List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:October 1 2010 7:12pm
Subject:Re: bzr commit into mysql-5.5-bugfixing branch (vvaintroub:3204)
View as plain text  
Hello Vlad!

* Vladislav Vaintroub <vladislav.vaintroub@stripped> [10/10/01 21:13]:
> > Here are my comments about your patch:
> > 
> > 5) You have told me that CONDITION_VARIABLE is a small object so
> >    it won't increase size of pthread_cond_t much. Still since it
> >    is never used together with the rest of structure maybe it
> >    makes sense to put it in union with the rest of the structure
> >    to save some space?
> 
> I'm not completely sure what you suggest. Should it look like that?
> 
> typedef  struct xxx {
> #ifdef _WIN32
> union 
> {
>   struct
>   {
>     Member;
>     Member;
>   }
>  struct
>  {
> #endif /* Win32 */
>   Member;
>   Member;
>  }
> #ifdef _WIN32
> }
> }
> #endif
> 
> (IMO, very ugly). 
> OR you prefer that
> 
> #ifdef _WIN32
> typedef struct xxx
> {
>   Union 
>   {
>       Struct
>      {
>        Member;
>        Member;
>      }
>      Struct 
>     {
>      Member;
>      Member;
>     }
> }zzz
> #else
> typedef  struct xxx
> {
>    Member;
>    Member;
> }zzz
> #endif
> 
> Also ugly but less than first example. I'm not really sure it complies like that with
> anonymous  members, I did not try.

I also think that the second example is nicer.

> 
> On the other hand. Maybe more space is not a bad thing after all, there is atomic
> stuff going on in this structures, and padding
> will put them in the different cache lines  and to false sharing 

Well... It is up to you. But I think both these structures are already big enough.

...

> > 14) As Davi I would have preferred if this one-time initialization
> >     would have happened somewhere in my_init() or my_basic_init().
> >     This would have allowed to avoid overhead of extra interlocked
> >     exchange/access to volatile variable for each pthread_cond_init.
> > 
> >     If you are concerned with a safety of doing this initialization in
> >     my_basic_init() we could add assert which will fail if someone tries
> >     to init condition variable before check_native_cond_availability()
> >     is called.
> >     In any case, if there are reasons to do this one time initialization
> >     using my_pthread_once() in this call they should be explained in the
> >     above comment.
> 
> 
> I think  is better from the modularity point of view, the code is concentrated in one
> file , and the module is usable without
> spreading it into different places which is my_init() . Given  the cost of
> pthread_once is minimal, the place where it used is not
> hot,  and a big plus is that the usage rwlock is safe and initialization-free.
> 
> If you ever have followed the problems  with  MY_PTHREAD_MUTEX_FAST (I recall Davi
> took a look at this) that crashed every couple of
> month on FreeBSD only, in DBUG only, and every time because initialization of it was
> moved from one place to another inside
> my_init(),  then you would probably understand the my concerns  better.  Rwlocks can
> become as popular as mutexes, and it will crash
> every couple of month on DBUG version only, and on Windows only, and only on specific
> Windows version:)
> 
> As it is in the patch , it is safe without adding assertions. Plus,  and it is safe
> against C++ static initialization. To illustrate
> C++ static initialization problem, here is an example. Somebody  create a  C++ class
> and to  puts some rwlocks into it . It is
> natural to have mysql_rwlock_init() in constructor. In some circumstances, it is also
> natural to have a static instance of this
> class. So a programmer compiled it on his Linux, or Solaris,  it worked well. He also
> made some strides towards portability and
> tested on his wife's XP - worked fine. He pushed - and his program hanged or crashed
> on Windows 2008. Now, what you would suggest to
> this developer? Is this reasonable to tell him to call my_init() in his constructor,
> just  for the sake of single (for him
> unimportant) platform, just because he used rwlocks?
> 
> As for access to volatile variable,  I'm not concerned. Performance has to be
> measured (well ,it was measured even), and I really
> tend to believe  we both  already spent more time writing this explanations, than it
> could be saved by  all future Windows
> installations of MySQL around the world, if I implemented you (and davi's )
> suggestion :)

OK. All of these are valid points.

Maybe it makes sense to mention them briefly in the comment ? :)

> > > @@ -155,6 +155,13 @@ int pthread_cancel(pthread_t thread)
> > >  int my_pthread_once(my_pthread_once_t *once_control,
> > >      void (*init_routine)(void))
> > >  {
> > > +  /*
> > > +    Do "dirty" read to find out if initialization is already done, to
> > > +    save an interlocked operation in common case.
> > > +  */
> > > +  if (*once_control == MY_PTHREAD_ONCE_DONE)
> > > +    return 0;
> > > +
> > >    LONG state= InterlockedCompareExchange(once_control,
> MY_PTHREAD_ONCE_INPROGRESS,
> > >                                            MY_PTHREAD_ONCE_INIT);
> > 
> > 15) I concur with Davi that this change should not be necessary if you
> >     accept our comment 14).
> 
> Not persuaded yet , but we can discuss this further.
> 
> >     If you still want to keep it, then IMO there is some additional work to do:
> > 
> >     This is so called Double Checked Locking pattern or rather anti-pattern.
> >     (See for example http://en.wikipedia.org/wiki/Double-checked_locking).
> >     Since it is known not to work in general case you have to explain (in the
> >     comment describing this optimization) why it works in your particular case.
> 
> >     E.g., because Visual C++ with version >=5.0 provides guarantee that read
> >     of volatile variable has Acquire semantics, so any reads of global/static
> >     memory which follow it won't be reordered to happen before volatile is
> >     read.
> 
> I think you make it too complicated. Volatile/atomics are used only to ensure this
> function is called exactly once, so disregardthem
> for a moment.
> 
> 
> Lets take a much simpler case, below, which hopefully illustrates the point.  It is
> just an example for "early exit", no atomics
> voodoo.
> 
> void  f()
> {
>   sleep(1);
> }
> 
> static int var = 0;
> 
> void my_func()
> {
>   If(var==1)
>    return; // EARLY RETURN
>   f();
>   var=1;
> }
> 
> I state that EARLY_RETURN does not happen before f() runs at least once.  No matter
> how many threads you let run in parallel.  You
> seem  to suggest my_func() can return before at least one f() is finished, and only
> volatile, atomics and memory ordering rules
> help to avoid this. If so, can you point where you see problem in this simple code
> first , and then we can go back to "real"
> pthread_once(), but it basically uses the same logic.

As discussed on IRC it is not about whether EARLY_RETURN before f() runs
at least once. The memory barrier is needed in order to ensure that code
which is executed outside of my_func() after EARLY_RETURN happens sees all
changes which were done by f() executed in other thread.

Since the above pthread_once() code heavily relies on "volatile" properties
which are specific to Visual C++, and would have not worked in e.g. GCC,
it needs commenting.

-- 
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:3204) Vladislav Vaintroub27 Sep
Re: bzr commit into mysql-5.5-bugfixing branch (vvaintroub:3204)Davi Arnaut1 Oct
Re: bzr commit into mysql-5.5-bugfixing branch (vvaintroub:3204)Dmitry Lenev1 Oct
  • RE: bzr commit into mysql-5.5-bugfixing branch (vvaintroub:3204)Vladislav Vaintroub1 Oct
    • Re: bzr commit into mysql-5.5-bugfixing branch (vvaintroub:3204)Dmitry Lenev1 Oct