cvs commit: src/lib/libthr/thread thr_mutex.c src/lib/libkse/thread thr_mutex.c src/include pthread.h

[Kris Kennaway]

David Xu wrote:
> Kris Kennaway wrote:
>>>
>>> I object adding PTHREAD_MUTEX_ADAPTIVE_NP, because there is no
>>> corresponding PTHREAD_ADPATIVE_MUTEX_INITIALIZER, but normal
>>> pthread mutex has macro PTHREAD_MUTEX_INITIALIZER, so it is
>>> inconsistent, maybe adding pthread_mutexattr_setspin() etcs are better
>>> than this hack for our implementation, it is not portable though.
>>
>>
>> There is an PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP, which is again the 
>> name that existing applications expect for it.  The fact that this 
>> interface *already exists* in other operating systems and *is already 
>> used* by real applications overrides objections about one name choice 
>> vs another.  The best you can argue for is to use a different name 
>> with a compatibility #define, but I dont see the point of this.
>>
> 
> I didn't find any place in thr_mutex.c that you have set 
> PTHREAD_MUTEX_ADAPTIVE_NP type, or I missed something ?

PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP is just in <pthread.h>.

/*
  * Static initialization values.
  */
#define PTHREAD_MUTEX_INITIALIZER       NULL
#define PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP   NULL
#define PTHREAD_COND_INITIALIZER        NULL
#define PTHREAD_RWLOCK_INITIALIZER      NULL

>>> I remembered mysql uses this macro to initialize spin mutex, and you
>>> indead needs a patch to let it work
>>
>>
>> No, with the code I committed mysql detects and uses it out of the 
>> box, without requiring any patches.  It is easy to measure the 
>> resulting 30% performance improvement at high loads ;-)
>>
> see above, I didn't see any code set PTHREAD_MUTEX_ADAPTIVE_NP type.

The code is already in mysql for use with glibc.  It basically does

#ifdef PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP
/*
  * Use PTHREAD_MUTEX_ADAPTIVE_NP for the mutexes we know will benefit
  * from it
  */
...
#endif

so it just works.

> My code even needn't to recompile mysql and improve 40% performance. :-)

OK, but that's not good enough.  As I have mentioned, you should have 
serious concerns about performance loss in other cases with an approach 
that always spins in userland when acquiring any mutex.

>>  > in fact spin mutex in libthr is
>>
>>> arguable, normally you can use pthread_mutex_trylock() in application 
>>> to simulate spinning, adding such mutex type it in libthr just 
>>> simplified
>>> it, but it is not portable.
>>
>>
>> That is what the "_NP" indicates (although remember that this 
>> interface is compatible with glibc).
>>
>> Kris
>>
> I am waiting for others, since this is first time we have to add a new
> mutex type.

I don't understand what you are saying here.  The glibc interface is 
defined by glibc, so if we want to do something different then either 
compatibility interfaces are needed (so what was the point of being 
different other than "NIH"?), or we need to manually patch the 
applications to do it "our way".

Kris