Hello Vladislav!
Here is my comments about new version of your patch:
* Vladislav Vaintroub <vvaintroub@stripped> [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 <errno.h>
>
> +#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
