MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:September 15 2010 10:53am
Subject:Re: bzr commit into mysql-5.5-bugfixing branch (vvaintroub:3207)
Bug#56585
View as plain text  
Hello Vlad!

Here are my comments about new version of your patch:

* Vladislav Vaintroub <vvaintroub@stripped> [10/09/12 00:36]:
> #At file:///H:/bzr-new/55bf/ based on
> revid:tor.didriksen@stripped
> 
>  3207 Vladislav Vaintroub	2010-09-11
>       Bug#56585 : Slowdown on Windows in readonly  benchmark.
>       
>             
>       Yet another take on this problem:
>       implement rwlock using 2 critical sections and event.
>       This implementation is cheaper than  general one on based on condition 
>       variables,because it has less overhead compared to general one.
>             
>       For writer-mostly scenarios, the lock is equivalent to critical section,
>       with just Enter/LeaveCriticalSection per rwlock/rwunlock pair.
>             
>       For concurent reader scenarios, the overhead is the same as in general 
>       implementation (critical section enter/leave pair for both lock and unlock).
>       plus there is extra overhead to wait or release the writer for the first
> entering
>       and last existing reader).
>             
>       Compared to implementation on other platforms:
>       1) reader and writer critical sections are split (so there is less 
>            contention)
>       2) Less critical section lock/unlock operations  for the writer.
>       
>             
>       Performance-wise this lock seems to bring a lot, outperforming
>       even native reader-writer locks, as in the table below, under
>       column ("no fix" is 5.5,  "srwlock" is native Vista RW lock, thislock
>        is implementation in this patch)
>             
>       -------------------------------------------------
>        benchmark      | no fix     srwlock  thislock  
>        -----------------------------------------------
>       simple_ranges | 14051    17996   18459
>        -----------------------------------------------
>        point_select    | 20576     29120   29309

1) Great!!! IMO it makes sense to mention in this comment at least
   for which number of concurrent connections you have got such an
   improvement.

   Also I think that we should ask QA people to produce more thorough
   comparison of server performance with and without your patch in *ALL*
   sysbench's tests for different number of connections for both InnoDB
   and MyISAM.
   This will show us full picture and ideally should confirm that your
   patch does not introduces significant performance degradation in any
   other test (I am particularly suspicious about OLTP_RW/MyISAM test).
   Note that to exclude influence of other changes on the test we should
   ensure that the only difference between two versions of server to be
   tested is your patch.

>             
>       Besides,this lock implements correctly (in Dmitri's sense of it)
>       "prefer readers", e.g writer does not enter or block newly incoming
>       readers and waits until count of all (pending and active readers goes
>       down to 0).
>             
>       Also, this lock is recursive, for both readers and writers.
> 
>     modified:
>       include/my_pthread.h
>       mysys/thr_rwlock.c
> === modified file 'include/my_pthread.h'
> --- a/include/my_pthread.h	2010-08-10 21:12:01 +0000
> +++ b/include/my_pthread.h	2010-09-11 20:35:24 +0000
> @@ -632,6 +632,25 @@ extern int rw_pr_init(rw_pr_lock_t *);
>  /* Otherwise we have to use our own implementation of read/write locks. */
>  #define NEED_MY_RW_LOCK 1
>  struct st_my_rw_lock_t;
> +
> +#ifdef _WIN32
> +typedef struct _win_prlock
> +{
> +  CRITICAL_SECTION reader_cs;
> +  CRITICAL_SECTION writer_cs;
> +  volatile LONG reader_count; /*readers (all, pending and active) */
> +  int  writer_recursion_count; /* recursion count for writer lock */
> +  HANDLE allow_writer;
> +}rw_pr_lock_t;

2) IMO it no longer makes sense to require our custom my_rw_lock_t
   for implementing of rw_pr_lock_t on Windows (it is required for
   other reasons but it is a different matter).
   So I propose to change the above system of ifdefs in the following
   way:

#if defined(HAVE_PTHREAD_RWLOCK_RDLOCK) &&
defined(HAVE_PTHREAD_RWLOCKATTR_SETKIND_NP)
// Here go Linux-specific defines.
#elif defined(_WIN32)
// Here go your Windows-specific declarations.
#else
/* Otherwise we have to use our own implementation of read/write locks. */
#define NEED_MY_RW_LOCK 1
// Here goes generic implementation.
#endif /* defined(HAVE_PTHREAD_RWLOCK_RDLOCK) &&
defined(HAVE_PTHREAD_RWLOCKATTR_SETKIND_NP) */

3) IMO it would be nice to have short comment explaining why do
   we have a separate rw_pr_lock_t implementation on Windows
   (because it provides much better performance than universal
   one-size-fits-all implementation).

4) Is there some special reason for starting structure name with underscore?
   AFAIU we try to avoid this for function names, structure members and
   variables. IMO it is better to use something like st_rw_pr_lock_t
   instead.

5) You have committed separate patch making reader_count non-volatile
   int. So I am not commenting about volatile and LONG.

6) Please use Doxygen style for comments describing structure
   members. Also I would have added comments describing other
   members than reader_count and writer_recursion_count.

7) Although our Coding Style doesn't exactly prescribes spacing for this
   case let us be consistent with spacing in the rest of code and have
   ' ' between '}' and 'rw_pr_lock_t'.

> +
> +extern int rw_pr_init(rw_pr_lock_t *);
> +extern int rw_pr_rdlock(rw_pr_lock_t *);
> +extern int rw_pr_wrlock(rw_pr_lock_t *);
> +extern int rw_pr_tryrdlock(rw_pr_lock_t *);
> +extern int rw_pr_trywrlock(rw_pr_lock_t *);
> +extern int rw_pr_unlock(rw_pr_lock_t *);
> +extern int rw_pr_destroy(rw_pr_lock_t *);

8) What about rw_pr_lock_assert_write_owner(A)/not_write_owner(A) macros?
   I think it makes sense to have defined even on Windows. Probably it is
   OK for them to do nothing (as it happens on Linux).

> +#else
>  #define rw_pr_lock_t my_rw_lock_t
>  extern int rw_pr_init(struct st_my_rw_lock_t *);
>  #define rw_pr_rdlock(A) my_rw_rdlock((A))
> @@ -643,7 +662,7 @@ extern int rw_pr_init(struct st_my_rw_lo
>  #define rw_pr_lock_assert_write_owner(A) my_rw_lock_assert_write_owner((A))
>  #define rw_pr_lock_assert_not_write_owner(A) my_rw_lock_assert_not_write_owner((A))
>  #endif /* defined(HAVE_PTHREAD_RWLOCK_RDLOCK) &&
> defined(HAVE_PTHREAD_RWLOCKATTR_SETKIND_NP) */
> -
> +#endif
>  
>  #ifdef NEED_MY_RW_LOCK
>  /*
>
> === modified file 'mysys/thr_rwlock.c'
> --- a/mysys/thr_rwlock.c	2010-08-12 13:50:23 +0000
> +++ b/mysys/thr_rwlock.c	2010-09-11 20:35:24 +0000


9) Since we are doing significant changes to this file and to
   my_pthread.h maybe it makes sense to update copyright notice
   in them? What do you think?

> @@ -192,12 +192,13 @@ int my_rw_unlock(my_rw_lock_t *rwp)
>    return(0);
>  }
>  
> -
> +#ifndef _WIN32
>  int rw_pr_init(struct st_my_rw_lock_t *rwlock)
>  {
>    my_bool prefer_readers_attr= TRUE;
>    return my_rw_init(rwlock, &prefer_readers_attr);
>  }
> +#endif
>  
>  #else
> 

10) I think it is better to disentangle defines controlling our custom
    generic rwlock and custom Windows-specific rw_pr_lock implementations
    in this file as well. How about re-organizing ifdefs in it in the
    following way:

#if defined(THREAD)
#if defined(NEED_MY_RW_LOCK)

// Our generic rwlock implementation goes here.

#endif /* defined(NEED_MY_RW_LOCK) */

#if defined(HAVE_PTHREAD_RWLOCK_RDLOCK) &&
defined(HAVE_PTHREAD_RWLOCKATTR_SETKIND_NP)

// Here go Linux-specific rw_pr_init.

#elif defined(_WIN32)

// Here goes your Windows-specific implementation of rw_pr_lock.

#else

// Here goes rw_pr_init for case when rw_pr_lock is based on generic rwlock.

#endif /* defined(HAVE_PTHREAD_RWLOCK_RDLOCK) &&
defined(HAVE_PTHREAD_RWLOCKATTR_SETKIND_NP) */

#endif /* defined(THREAD) */

?

> @@ -218,4 +219,123 @@ int rw_pr_init(rw_pr_lock_t *rwlock)
>  }
>  
>  #endif /* defined(NEED_MY_RW_LOCK) */
> +
> +#ifdef _WIN32
> +int rw_pr_init(rw_pr_lock_t *rwl)
> +{
> +  InitializeCriticalSection(&rwl->reader_cs);
> +  InitializeCriticalSection(&rwl->writer_cs);
> +  rwl->allow_writer= CreateEvent(NULL, TRUE, TRUE, NULL);
> +  rwl->reader_count= 0;
> +  rwl->writer_recursion_count= 0;
> +  if(!rwl->allow_writer)
> +    return ENOMEM;
> +  return 0;
> +}

11) According to our Coding Style there should be a space between 'if'
    and '('. Please adhere to this rule here and in other places.

> +

12) Our Coding Style says that functions should be separated by two
    empty lines. Please adhere to this rule here and in other places.

> +int rw_pr_rdlock(rw_pr_lock_t *rwl)
> +{
> +  EnterCriticalSection(&rwl->reader_cs);
> +  rwl->reader_count++;
> +  if (rwl->reader_count == 1)
> +  {
> +    /* 
> +      First reader waits for possible writer to complete
> +      (during this time it blocks other readers that are waiting on reader_cs)
> +      After that, it will disallows other writers until all readers finish
> +      via ResetEvent().
> +    */
> +    EnterCriticalSection(&rwl->writer_cs);
> +    ResetEvent(rwl->allow_writer);
> +    LeaveCriticalSection(&rwl->writer_cs);
> +  }
> +  LeaveCriticalSection(&rwl->reader_cs);
> +  return 0;
> +}

13) IMO we need to have a really good comment somewhere describing how
    your rw_pr_lock works as a whole since it is non-trivial.
    I think right before rw_pr_init is an apropriate place for this.
    We can have detailed discussion about such comment on IRC...

> +
> +
> +int rw_pr_wrlock(rw_pr_lock_t *rwl)
> +{
> +  EnterCriticalSection(&rwl->writer_cs);
> +  if(rwl->reader_count > 0)
> +    WaitForSingleObject(rwl->allow_writer, INFINITE);

14) Here goes the most tricky part of your patch which definitely
    deserves a comment. This comment should explain why it is OK
    to read reader_count here under protection of writer_cs even
    though it is changed under protection of reader_cs.
    IMO in such comment it makes sense to look at all possible
    situations in which reader_count value which was read here
    does not correspond to "reality" due to memory visibility
    rules or races and show that this won't cause any problems,
    or simply won't happen in cases when it can cause problems.
    Again I am ready to discuss details on IRC...

> +  rwl->writer_recursion_count++;
> +  return 0;
> +}
> +
> +
> +int rw_pr_unlock(rw_pr_lock_t *rwl)
> +{
> +  if(rwl->writer_recursion_count > 0)
> +  {
> +    /* Unlock writer */
> +    rwl->writer_recursion_count--;
> +    LeaveCriticalSection(&rwl->writer_cs);
> +  }
> +  else
> +  {
> +    EnterCriticalSection(&rwl->reader_cs);
> +    rwl->reader_count--;
> +    if(rwl->reader_count == 0)
> +    {
> +      /* 
> +        Last reader exits, allow writers to run
> +      */
> +        SetEvent(rwl->allow_writer);

15) I think alignment of the above code is wrong according to
    Coding Style. Please fix it.

> +    }
> +    LeaveCriticalSection(&rwl->reader_cs);
> +  }
> +  return 0;
> +}
> +
> +int rw_pr_tryrdlock(rw_pr_lock_t *rwl)
> +{
> +  EnterCriticalSection(&rwl->reader_cs);

16) Hmm... I think the above should be TryEnterCriticalSection
    as well. Otherwise rw_pr_tryrdlock() is going to be blocked
    if lock is wr-locked and some other thread requested rd-lock
    using rw_pr_rdlock().

> +  rwl->reader_count++;
> +  if (rwl->reader_count == 1)
> +  {
> +    /* Wait for possible writer to complete, disallow other writers */
> +    if(TryEnterCriticalSection(&rwl->writer_cs))
> +    {
> +      ResetEvent(rwl->allow_writer);
> +      LeaveCriticalSection(&rwl->writer_cs);
> +    }
> +    else
> +    {
> +      rwl->reader_count--;
> +      LeaveCriticalSection(&rwl->reader_cs);
> +      return EBUSY;
> +    }
> +  }
> +  LeaveCriticalSection(&rwl->reader_cs);
> +  return 0;
> +}
> +
> +int rw_pr_trywrlock(rw_pr_lock_t *rwl)
> +{
> +  if(!TryEnterCriticalSection(&rwl->writer_cs))
> +    return EBUSY;
> +
> +   if(rwl->reader_count > 0)
> +   {
> +      if(WaitForSingleObject(rwl->allow_writer, 0) != WAIT_OBJECT_0)
> +      {
> +        LeaveCriticalSection(&rwl->writer_cs);
> +        return EBUSY;
> +      }
> +   }
> +
> +  rwl->writer_recursion_count++;
> +  return 0;
> +}
> +
> +int rw_pr_destroy(rw_pr_lock_t *rwl)
> +{
> +  DeleteCriticalSection(&rwl->reader_cs);
> +  DeleteCriticalSection(&rwl->writer_cs);
> +  CloseHandle(rwl->allow_writer);
> +  return 0;
> +}
> +#endif /* _WIN32 */
> +
>  #endif /* defined(THREAD) */
>

17) Taking into account that the above code is fairly complex chances
    that we have missed some kind of race in it are pretty high (the
    fact that this is the third version of the patch supports this
    theory). Hence I think it is crucial to have this code covered by
    stress/unit tests.
    Particularly I think that we should instrument this code with debug
    asserts checking its invariants and then try to perform random or
    almost random operations on this rwlock in many concurrent threads
    on a multi-core system.
    This test is not necessary have to be part of our test suite
    especially since to be somewhat sure in our code we have to run it
    for a few hours, nevertheless we should run it before pushing this
    patch into the tree.


Thanks a lot for working on this!

Please don't hesitate to discuss the above comments with me!

-- 
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-bugfixing branch (vvaintroub:3207) Bug#56585Vladislav Vaintroub11 Sep
Re: bzr commit into mysql-5.5-bugfixing branch (vvaintroub:3207)Bug#56585Dmitry Lenev15 Sep
  • RE: bzr commit into mysql-5.5-bugfixing branch (vvaintroub:3207) Bug#56585Vladislav Vaintroub22 Sep