List:Commits« Previous MessageNext Message »
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
View as plain text  
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
Thread
bzr commit into mysql-5.5-bugteam branch (vvaintroub:3220) Bug#56405Bug#56585Vladislav Vaintroub4 Oct
Re: bzr commit into mysql-5.5-bugteam branch (vvaintroub:3220)Bug#56405 Bug#56585Dmitry Lenev4 Oct
  • RE: bzr commit into mysql-5.5-bugteam branch (vvaintroub:3220) Bug#56405 Bug#56585Vladislav Vaintroub4 Oct