From: Dmitry Lenev Date: October 4 2010 8:59am Subject: Re: bzr commit into mysql-5.5-bugteam branch (vvaintroub:3220) Bug#56405 Bug#56585 List-Archive: http://lists.mysql.com/commits/119798 Message-Id: <20101004085931.GA25938@mockturtle> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Hello Vladislav! Here is my comments about new version of your patch: * Vladislav Vaintroub [10/10/04 04:32]: > #At file:///H:/bzr-new/mysql-5.5-bugteam/ based on revid:alexander.nozdrin@stripped > > 3220 Vladislav Vaintroub 2010-10-04 > A follow-up to the patch for bug #56405 "Deadlock in the MDL deadlock > detector". This patch addresses performance regression in OLTP_RO/MyISAM > test on Windows introduced by the fix for bug #56405. Thus it makes > original patch acceptable as a solution for bug #56585 "Slowdown of > readonly sysbench benchmarks (e.g point_select) on Windows 5.5". > > With this patch, MySQL will use native Windows condition variables and > reader-writer locks if they are supported by the OS. > > This speeds up MyISAM and the effect comes mostly from using native > rwlocks. Native conditions improve scalability with higher number of > concurrent users in other situations, e.g for prlocks. > > Benchmark numbers for this patch as measured on Win2008R2 quad > core machine are attached to the bug report. > ( direct link http://bugs.mysql.com/file.php?id=15883 ) > > Note that currently we require at least Windows7/WS2008R2 for > reader-writer locks, even though native rwlock is available also on Vista. > Reason is that "trylock" APIs are missing on Vista, and trylock is used in > the server (in a single place in query cache). > > While this patch could have been written differently, to enable the native > rwlock optimization also on Vista/WS2008 (e.g using native locks everywhere > but portable implementation in query cache), this would come at the > expense of the code clarity, as it would introduce a new "try-able" rwlock > type, to handle Vista case. > > Another way to improve performance for the special case > (OLTP_RO/MYISAM/Vista) would be to eliminate "trylock" usage from server, > but this is outside of the scope here. OK. If there is a significant interest in bringing rwlock improvement to Vista we can implement one of approaches that you have outlined above in a separate patch. > === modified file 'include/my_pthread.h' > --- a/include/my_pthread.h 2010-09-29 12:09:07 +0000 > +++ b/include/my_pthread.h 2010-10-04 00:27:24 +0000 > @@ -48,19 +48,30 @@ typedef struct st_pthread_link { > struct st_pthread_link *next; > } pthread_link; > > -typedef struct { > - uint32 waiting; > - CRITICAL_SECTION lock_waiting; > - > - enum { > - SIGNAL= 0, > - BROADCAST= 1, > - MAX_EVENTS= 2 > - } EVENTS; > - > - HANDLE events[MAX_EVENTS]; > - HANDLE broadcast_block_event; > + /** > + Implementation of Windows condition variables. > + We use native conditions on Vista and later, and fallback to own > + implementation on earlier OS version. > + */ > +typedef union I think the above comment is mis-aligned. Please check with our Coding Guidelines. ... > @@ -679,6 +690,47 @@ extern int rw_pr_destroy(rw_pr_lock_t *) > > > #ifdef NEED_MY_RW_LOCK > + > +#ifdef _WIN32 > + > +/** > + Implementation of Windows rwlock. > + > + We use native (slim) rwlocks on Win7 and later, and fallback to portable > + implementation on earlier Windows. > + > + slim rwlock are also available on Vista/WS2008, but we do not use it > + ("trylock" APIs are missing on Vista) > +*/ > +typedef union > +{ > + /* Native rwlock (is_srwlock == TRUE) */ > + struct > + { > + SRWLOCK srwlock; /* native reader writer lock */ > + BOOL have_exclusive_srwlock; /* used for unlock */ > + }; > + > + /* > + Portable implementation (is_srwlock == FALSE) > + Fields are identical with Unix my_rw_lock_t fields. > + */ > + struct > + { > + pthread_mutex_t lock; /* lock for structure */ > + pthread_cond_t readers; /* waiting readers */ > + pthread_cond_t writers; /* waiting writers */ > + int state; /* -1:writer,0:free,>0:readers */ > + int waiters; /* number of waiting writers */ > +#ifdef SAFE_MUTEX > + pthread_t write_thread; > +#endif > + }; > +} my_rw_lock_t; > + > + > +#else /* _WIN32 */ > + I was a bit surpised to see that you have decided that you need copy the whole my_rw_lock_t structure for Windows instead of having something like: typedef union { #ifdef _WIN32 /* Native rwlock (is_srwlock == TRUE) */ struct { SRWLOCK srwlock; /* native reader writer lock */ BOOL have_exclusive_srwlock; /* used for unlock */ }; #endif /* Portable implementation (is_srwlock == FALSE) Fields are identical with Unix my_rw_lock_t fields. */ struct { pthread_mutex_t lock; /* lock for structure */ ... } } my_rw_lock_t; Especially since both Windows-specific definition and generic portable implementation are still handled by the same calls. Could you please explain your reasons for doing this? ... > /* > On systems which don't support native read/write locks we have > to use own implementation. > @@ -694,6 +746,8 @@ typedef struct st_my_rw_lock_t { > #endif > } my_rw_lock_t; > > +#endif /*! _WIN32 */ > + > extern int my_rw_init(my_rw_lock_t *); > extern int my_rw_destroy(my_rw_lock_t *); > extern int my_rw_rdlock(my_rw_lock_t *); > ... > @@ -177,6 +244,87 @@ int pthread_cond_broadcast(pthread_cond_ > } > > > +/* > + Posix API functions. Just choose between native and legacy implementation. > +*/ > + > +int pthread_cond_init(pthread_cond_t *cond, const pthread_condattr_t *attr) > +{ > + /* > + Once initialization is used here rather than in my_init(), to > + 1) avoid my_init() pitfalls- undefined order in which initialization should > + run > + 2) be potentially useful C++ (in static constructors that run before main()) > + 3) just to simplify the API. > + Also, the overhead is of my_pthread_once is very small. > + */ ^^ Redundant "is" ? ... > === modified file 'mysys/thr_rwlock.c' > --- a/mysys/thr_rwlock.c 2010-09-29 12:09:07 +0000 > +++ b/mysys/thr_rwlock.c 2010-10-04 00:27:24 +0000 > @@ -20,6 +20,118 @@ > #if defined(NEED_MY_RW_LOCK) > #include > > +#ifdef _WIN32 > + > +static BOOL have_srwlock = TRUE; Ouch... IMO the above looks wrong as it means that native rwlocks are going to be used even on pre-Vista systems. I wonder if you have tried to run server built with your patch on any pre-Vista system. If you have not, please do! (And according to our Coding Guidelines there should be no space between 'have_srwlock' and '='). ... > +/** > + Check for presence of Windows slim reader writer lock function. > + Load function pointers. > +*/ > +static void check_srwlock_availability(void) There should be an empty line between function and its description per Coding Guidelines (I am sorry for being such a stickler :)) > +{ > + HMODULE module= GetModuleHandle("kernel32"); > + > + my_InitializeSRWLock= (srw_func) GetProcAddress(module, > + "InitializeSRWLock"); > + my_AcquireSRWLockExclusive= (srw_func) GetProcAddress(module, > + "AcquireSRWLockExclusive"); > + my_AcquireSRWLockShared= (srw_func) GetProcAddress(module, > + "AcquireSRWLockShared"); > + my_ReleaseSRWLockExclusive= (srw_func) GetProcAddress(module, > + "ReleaseSRWLockExclusive"); > + my_ReleaseSRWLockShared= (srw_func) GetProcAddress(module, > + "ReleaseSRWLockShared"); > + my_TryAcquireSRWLockExclusive= (srw_bool_func) GetProcAddress(module, > + "TryAcquireSRWLockExclusive"); > + my_TryAcquireSRWLockShared= (srw_bool_func) GetProcAddress(module, > + "TryAcquireSRWLockShared"); > + > + /* > + We currently require TryAcquireSRWLockExclusive. This API is missing on > + Vista, this means SRWLock are only used starting with Win7. > + > + If "trylock" usage for rwlocks is eliminated from server codebase (it is used > + in a single place currently, in query cache), then SRWLock can be enabled on > + Vista too. In this case condition below needs to be changed to e.g check > + for my_InitializeSRWLock. > + */ > + > + if (my_TryAcquireSRWLockExclusive) > + have_srwlock= TRUE; > + > +} I think it is OK to push you patch after considering/fixing the above issues (especially issue with have_srwlock initialization). -- Dmitry Lenev, Software Developer Oracle Development SPB/MySQL, www.mysql.com Are you MySQL certified? http://www.mysql.com/certification