List:Commits« Previous MessageNext Message »
From:Vladislav Vaintroub Date:September 24 2010 12:31pm
Subject:RE: bzr commit into mysql-5.5-runtime branch (Dmitry.Lenev:3142) Bug#56405 Bug#56715
View as plain text  
Hello Dmitry. Very nice work,

Some comments below.

> -----Original Message-----
> From: Dmitry Lenev [mailto:Dmitry.Lenev@stripped]
> Sent: Thursday, September 23, 2010 5:21 PM
> To: commits@stripped
> Subject: bzr commit into mysql-5.5-runtime branch (Dmitry.Lenev:3142) Bug#56405
> Bug#56715

[...]
> -/*
> -  Portable read-write locks which prefer readers.
> +/**
> +  Portable implementation of special type of read-write locks.
> 
> -  Required by some algorithms in order to provide correctness.
> +  These locks have two properties which are unusual for rwlocks:
> +  1) They "prefer readers" in the sense that they do not allow
> +     situations in which rwlock is rd-locked and there is a
> +     pending rd-lock which is blocked (e.g. due to pending
> +     request for wr-lock).
> +     This is a stronger guarantee than one which is provided for
> +     PTHREAD_RWLOCK_PREFER_READER_NP rwlocks in Linux.
> +     MDL subsystem deadlock detector relies on this property for
> +     its correctness.
> +  2) They are optimized for uncontended wr-lock/unlock case.
> +     This is scenario in which they are most oftenly used
> +     within MDL subsystem. Optimizing for it gives significant
> +     performance improvements in some of tests involving many
> +     connections.
>  */

Nice comment. As discussed on IRC, there are possibly different other assumptions about
how this lock is used in MDL context, e.g
mentioned race between destroy and other functions, e.g unlock. This is rather unusual,
normally application that is calling
"destroy" should make sure itself that no other API is being used. If it is not the case
here could you document exactly how it is
supposed to work (I admit I did not understand it fully). 
If it is grabbing the writer lock and destroy under writer lock, I'm afraid Windows
Application verifier will be unhappy with such
technique, as destroying critical sections(i.e pthread_mutex_t) that is locked is
considered  bad practice..


[...]
> +int rw_pr_tryrdlock(rw_pr_lock_t *rwlock)
> +{
> +  if (pthread_mutex_lock(&rwlock->lock))

Should not it be pthread_mutex_trylock ? Perhaps, we  do not need "try" APIs at all?

> +int rw_pr_trywrlock(rw_pr_lock_t *rwlock)
> +{
> +  if (pthread_mutex_lock(&rwlock->lock))

Same here.


> +int rw_pr_unlock(rw_pr_lock_t *rwlock)
> +{
> +  if (rwlock->active_writer)
> +  {
> +    /* We are unlocking wr-lock. */
> +#ifdef SAFE_MUTEX
> +    rwlock->writer_thread= 0;
> +#endif
> +    rwlock->active_writer= FALSE;
> +    if (rwlock->writers_waiting_readers)
> +    {
> +      /*
> +        Avoid expensive cond signal in case when
> +        there is no contention or it is wr-only.
> +      */
> +      pthread_cond_signal(&rwlock->no_active_readers);
> +    }
> +    pthread_mutex_unlock(&rwlock->lock);
> +  }
> +  else
> +  {
> +    /* We are unlocking rd-lock. */
> +    pthread_mutex_lock(&rwlock->lock);
> +    rwlock->active_readers--;
> +    if (rwlock->active_readers == 0 &&
> +        rwlock->writers_waiting_readers)
> +    {
> +      /*
> +        If we are last reader and there are waiting
> +        writers wake them up.
> +      */
> +      pthread_cond_signal(&rwlock->no_active_readers);
> +    }
> +    pthread_mutex_unlock(&rwlock->lock);
> +  }
> +  return 0;
> +}

I think it would be a good idea to move pthread_cond_signal  after the mutex unlock. I
believe it does not change correctness, it
could lead to spurious wakeups,  but it is not a problem at all (condition is reevaluated
in the wait loop).  I measured on Windows
(OLTP_RO MYISAM low concurrency), and measurements support this theory (that it is a good
idea). But as already discussed,
"destroy"  races  should be prevented/fixed  for that.

> +
>  #endif /* defined(THREAD) */


Thread
bzr commit into mysql-5.5-runtime branch (Dmitry.Lenev:3142) Bug#56405Bug#56715Dmitry Lenev23 Sep
  • RE: bzr commit into mysql-5.5-runtime branch (Dmitry.Lenev:3142) Bug#56405 Bug#56715Vladislav Vaintroub24 Sep