Re: Please test new NSLock implementation!

[Fred Kiefer]

Hi David,

I am sending my reply back to the mailing list as well, hope that this
is ok for you.


David Chisnall schrieb:
> 
> On 2 Sep 2009, at 21:05, Fred Kiefer wrote:
> 
>> This is not really a bug, but the new lock code exposes the use of
>> pthread.h in the header file NSLock.h. I would prefer to see this
>> reverted back, like the old code did it, using macros in the .m file to
>> convert the mutex and condition to the correct type.
>> It looks like you changed this on purpose. Do you have any specific
>> reason of handling this differently?
> 
> The old version used objc_mutex_t, which was a void*.  A mutex is
> typically either one or two words, depending on the implementation. 
> Using malloc for this is very wasteful, both in terms of speed, cache
> usage, and memory footprint.  The objc_mutex_alloc() function was doing
> malloc(sizeof(pthread_mutex_t)); on some platforms (e.g. FreeBSD)
> pthread_mutex_t is a pointer to some other structure, so we were tying
> up three cache lines for a two word data structure.  This is far from
> ideal.

Not sure if I getting this correctly but on my 64-bit Linux system I
have # define __SIZEOF_PTHREAD_MUTEX_T 40 and 24 for a 32-bit system.
We are rather talking about 12 to 20 words here not one or two.

What do others think, is it worthwhile to hide these implementation
details via another indirection or not? I am still in favour of using an
opaque data type here. On systems like FreeBSD where pthread_mutex_t is
itself a pointer we could use that directly and on other systems we have
one additional malloc and free call per mutex. And where the structure
fits into your one or two words we could even put the value into the
ivar directly.

> We could just put some padding in the header, have something in the
> configure script the put sizeof(pthread_mutex_t) in a macro and then
> allocate this many chars + padding, but I don't see it as any cleaner.
> 
>> I think it is better to keep
>> implementation details as the usage of a specific thread implementation
>> local to GNUstep, this would also make it a lot easier to provide
>> different implementations in the .m file, if we ever need it.
> 
> I agree in theory, but in practice there is no clean way of doing this. 
> Fortunately, we have no requirement that headers match between platforms...
> 
>> But the
>> biggest benefit surely is that we keep user code free of our own
>> dependencies.
> 
> Yes, that would be ideal.  Suggestions welcome.
> 
>> Fred
>>
>> PS: There is a semicolon after the broadcast method in the .m file, this
>> may cause problems with the documentation generation.
> 
> Fixed.

Thank you.

>> PPS: Shouldn't NSConditionLock unlock twice in unlockWithCondition: as
>> the old code did?
> 
> No, but it shouldn't be locking there because -unlockWithCondition:
> should only be called when the condition lock is already locked by the
> caller.  I've now fixed this.  I've also added a test for
> NSConditionLock which implements a producer-consumer model and throws
> ten thousand objects between threads, which now passes.  Note that
> NSConditionLock now uses NSCondition (as does Apple's implementation),
> so this test also tests NSCondition.
> 
> David

Either way, thank you for fixing this.