maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Sergei,

thanks for the review. Answers inline.

On Wed, Mar 19, 2014 at 02:47:43PM +0100, Sergei Golubchik wrote:
> Hi, Sergey!
> 
> On Mar 15, Sergey Vojtovich wrote:
> > At lp:maria/10.0
> > 
> > ------------------------------------------------------------
> > revno: 4053
> > revision-id: svoj@xxxxxxxxxxx-20140315185301-ce4r6br023co80dm
> > parent: sanja@xxxxxxxxxxxxxxxx-20140314073116-nklb0jkjd4t1w40n
> > committer: Sergey Vojtovich <svoj@xxxxxxxxxxx>
> > branch nick: 10.0
> > timestamp: Sat 2014-03-15 22:53:01 +0400
> > message:
> >   MDEV-5864 - Reduce usage of LOCK_open: TABLE_SHARE::tdc.free_tables
> >   
> >   Let TABLE_SHARE::tdc.free_tables, TABLE_SHARE::tdc.all_tables,
> >   TABLE_SHARE::tdc.flushed and corresponding invariants be protected by
> >   per-share TABLE_SHARE::tdc.LOCK_table_share instead of global LOCK_open.
> 
> This looks pretty good to me!
> A couple of comments, see below.
> 
> > === modified file 'sql/sql_plist.h'
> > --- a/sql/sql_plist.h	2013-11-20 11:05:39 +0000
> > +++ b/sql/sql_plist.h	2014-03-15 18:53:01 +0000
> > @@ -142,6 +142,14 @@ class I_P_List : public C, public I
> >  
> >      return result;
> >    }
> > +  inline T *back()
> > +  {
> > +    T *result= m_first;
> > +    if (result)
> > +      while (*B::next_ptr(result))
> > +        result= *B::next_ptr(result);
> > +    return result;
> > +  }
> 
> I think this is wrong. The correct way would be to use I_P_List<> with
> the I_P_List_fast_push_back policy, then you could use
> have get_last() method to get the last element in the list.
> May be as
> 
>     inline T *back()
>     {
>       return *I::get_last();
>     }
> 
I was considering I_P_List_fast_push_back, but I had reasons not to use it:
1. It'll have to maitain m_last (last element pointer), which involves extra
   shared memory location write. Even though there are good chances for m_head
   and m_last to share the same cache line. This maintenance will happen on
   a hot path.

2. I'm aiming to replace I_P_List with single-linked list (updated atomically).
   I can remove this ugly back() implementation when I'm done, or alternatively
   I can move it now to table_cache.cc.

3. We nerver use back() on a hot path, I believe there is not much meaning to
   sacrifice hot path performance.


> >    void swap(I_P_List<T, B, C> &rhs)
> >    {
> >      swap_variables(T *, m_first, rhs.m_first);
> > 
> > === modified file 'sql/table_cache.cc'
> > --- a/sql/table_cache.cc	2014-03-06 12:19:12 +0000
> > +++ b/sql/table_cache.cc	2014-03-15 18:53:01 +0000
> > @@ -70,13 +70,6 @@ static int32 tc_count; /**< Number of TA
> >  
> >  
> >  /**
> > -  Protects TABLE_SHARE::tdc.free_tables, TABLE_SHARE::tdc.all_tables.
> > -*/
> > -
> > -mysql_mutex_t LOCK_open;
> 
> So, you've now removed LOCK_open completely? Cool!
Yes, there is no more global mutex on a hot path. It means different tables
won't interfere on a hot path at all. At list with regards to mutexes, there
is still quite a few work before we'll make it scale well.

Originally I was aiming to keep LOCK_open for protection of
TABLE_SHARE::tdc.all_tables. But it came to be easier just to protect all_tables
by LOCK_table_share due to invariants between free_tables and all_tables.


> 
> > -
> > -
> > -/**
> >    Protects unused shares list.
> >  
> >    TABLE_SHARE::tdc.prev
> > @@ -360,12 +378,15 @@ bool tc_release_table(TABLE *table)
> >    table->in_use= 0;
> >    /* Add table to the list of unused TABLE objects for this share. */
> >    table->s->tdc.free_tables.push_front(table);
> > -  mysql_mutex_unlock(&LOCK_open);
> > +  mysql_mutex_unlock(&table->s->tdc.LOCK_table_share);
> >    return false;
> >  
> >  purge:
> > +  /* Wait for MDL deadlock detector */
> > +  while (table->s->tdc.all_tables_refs)
> > +    mysql_cond_wait(&table->s->tdc.COND_release, &table->s->tdc.LOCK_table_share);
> 
> you're using it many times, I'd put it in a small static inline
> function, like
> 
>  static wait_for_mdl_deadlock_detector(TABLE *table)
>  {
>    mysql_mutex_assert_owner(&table->s->tdc.LOCK_table_share);
>    while (table->s->tdc.all_tables_refs)
>      mysql_cond_wait(&table->s->tdc.COND_release, &table->s->tdc.LOCK_table_share);
>  }
I agree. Will make it as you suggest. Hope some day I'll move it to
tc_remove_table() and there will be no need for this extra function.

Thanks,
Sergey