maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Sergei,

comments inline.

On Fri, Jan 31, 2014 at 12:46:22PM +0100, Sergei Golubchik wrote:
> Hi, Sergey!
> 
> Please, see below.
> 
> On Jan 30, Sergey Vojtovich wrote:
> > revno: 3965
> > revision-id: svoj@xxxxxxxxxxx-20140130123642-wsysj31e1wwlqz0o
> > parent: psergey@xxxxxxxxxxxx-20140120123657-g1z4g11xmsdiw2dc
> > committer: Sergey Vojtovich <svoj@xxxxxxxxxxx>
> > branch nick: 10.0
> > timestamp: Thu 2014-01-30 16:36:42 +0400
> > message:
> >   MDEV-5403 - Reduce usage of LOCK_open: tc_count
> >   
> >   Lock-free tc_count.
> > === modified file 'sql/table_cache.cc'
> > --- a/sql/table_cache.cc	2013-12-12 17:49:14 +0000
> > +++ b/sql/table_cache.cc	2014-01-30 12:36:42 +0000
> > @@ -173,7 +175,9 @@ void tc_purge(void)
> >      {
> >        share->tdc.all_tables.remove(table);
> >        purge_tables.push_front(table);
> > -      tc_count--;
> > +      my_atomic_rwlock_wrlock(&LOCK_tdc_atomics);
> > +      my_atomic_add32(&tc_count, -1);
> > +      my_atomic_rwlock_wrunlock(&LOCK_tdc_atomics);
> 
> Okay, so in this new code tc_count is always equal or greater than the
> actual number of tables in the cache, right?
> (because you first remove the table from the cache, then decrement the
> counter)
> 
> >      }
> >    }
> >    tdc_it.deinit();
> > @@ -245,47 +249,52 @@ static void check_unused(THD *thd)
> >  
> >  void tc_add_table(THD *thd, TABLE *table)
> >  {
> > +  bool need_purge;
> >    DBUG_ASSERT(table->in_use == thd);
> >    mysql_mutex_lock(&LOCK_open);
> >    table->s->tdc.all_tables.push_front(table);
> > +  mysql_mutex_unlock(&LOCK_open);
> > +
> >    /* If we have too many TABLE instances around, try to get rid of them */
> > -  if (tc_count == tc_size)
> > +  my_atomic_rwlock_wrlock(&LOCK_tdc_atomics);
> > +  need_purge= my_atomic_add32(&tc_count, 1) >= (int32) tc_size;
> 
> Hmm, but here you increment after adding a table.
> 
> I'd suggest to keep tc_count consistently either always no smaller than
> the number of tables in the cache (that is, decrement after removing,
> increment before adding), or always no larger than that (derement before
> removing, increment after adding).
A very valid point which I was going to think about. I believe we should keep
it no larger than the number of tables in the cache: it may give somewhat
less evictions. Just fixed it in a patch which I'm working on now.

> > +  my_atomic_rwlock_wrunlock(&LOCK_tdc_atomics);
> > +
> > +  if (need_purge)
> >    {
> > +    TABLE *purge_table= 0;
> > +    TABLE_SHARE *share;
> >      TDC_iterator tdc_it;
> > -    mysql_mutex_unlock(&LOCK_open);
> >  
> >      tdc_it.init();
> >      mysql_mutex_lock(&LOCK_open);
> > -    if (tc_count == tc_size)
> > -    {
> > -      TABLE *purge_table= 0;
> > -      TABLE_SHARE *share;
> >        while ((share= tdc_it.next()))
> >        {
> >          TABLE_SHARE::TABLE_list::Iterator it(share->tdc.free_tables);
> >          TABLE *entry;
> >          while ((entry= it++))
> >            if (!purge_table || entry->tc_time < purge_table->tc_time)
> >              purge_table= entry;
> >        }
> > +    tdc_it.deinit();
> > +
> >        if (purge_table)
> >        {
> > -        tdc_it.deinit();
> >          purge_table->s->tdc.free_tables.remove(purge_table);
> >          purge_table->s->tdc.all_tables.remove(purge_table);
> >          mysql_rwlock_rdlock(&LOCK_flush);
> >          mysql_mutex_unlock(&LOCK_open);
> >          intern_close_table(purge_table);
> >          mysql_rwlock_unlock(&LOCK_flush);
> > +
> > +      my_atomic_rwlock_wrlock(&LOCK_tdc_atomics);
> > +      my_atomic_add32(&tc_count, -1);
> > +      my_atomic_rwlock_wrunlock(&LOCK_tdc_atomics);
> >          check_unused(thd);
> > -        return;
> >        }
> > -    }
> > -    tdc_it.deinit();
> > -  }
> > -  /* Nothing to evict, increment tc_count. */
> > -  tc_count++;
> > +    else
> >    mysql_mutex_unlock(&LOCK_open);
> > +  }
> >  }
> >  
> >  
> > @@ -357,25 +366,35 @@ bool tc_release_table(TABLE *table)
> >    DBUG_ASSERT(table->in_use);
> >    DBUG_ASSERT(table->file);
> >  
> > +  if (table->needs_reopen() || tc_records() > tc_size)
> > +  {
> > +    mysql_mutex_lock(&LOCK_open);
> > +    table->in_use= 0;
> > +    goto purge;
> > +  }
> 
> Why did you move this up?
Move what?

> 
> >    table->tc_time= my_interval_timer();
> >  
> >    mysql_mutex_lock(&LOCK_open);
> >    table->in_use= 0;
> > -  if (table->s->has_old_version() || table->needs_reopen() || tc_count > tc_size)
> > -  {
> > -    tc_count--;
> > +  if (table->s->has_old_version())
> > +    goto purge;
> > +  /* 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);
> > +  check_unused(thd);
> > +  return false;
> > +
> > +purge:
> > +  my_atomic_rwlock_wrlock(&LOCK_tdc_atomics);
> > +  my_atomic_add32(&tc_count, -1);
> > +  my_atomic_rwlock_wrunlock(&LOCK_tdc_atomics);
> >      table->s->tdc.all_tables.remove(table);
> 
> and now you decrement before removal
> 
> >      mysql_rwlock_rdlock(&LOCK_flush);
> >      mysql_mutex_unlock(&LOCK_open);
> >      intern_close_table(table);
> >      mysql_rwlock_unlock(&LOCK_flush);
> >      return true;
> > -  }
> > -  /* 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);
> > -  check_unused(thd);
> > -  return false;
> >  }
> >  
> >  
> Regards,
> Sergei

Thanks,
Sergey