maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Sergei,

thanks for your comments, answers inline.

On Tue, Dec 10, 2013 at 10:14:32PM +0100, Sergei Golubchik wrote:
> Hi, Sergey!
> 
> Did you benchmark the improvement you get with this patch?
Yes, my results against recent 10.0 are in jira task description.
Also please find Axel's results (MDEV-4956 + MDEV-5388) attached.

>From my results please note that mutex wait time dropped almost 2x.

>From Axel's results please note performance regression between 10.0.4
and 10.0.5.

With 10.0.4 we've seen much better performance improvement. I suppose that
10.0.5 regression doesn't permit to go over certain threshold.

I can benchmark 10.0.4 + MDEV-4956 + MDEV-5388 again if you like.

> On Dec 05, Sergey Vojtovich wrote:
> > At lp:maria/10.0
> > 
> > ------------------------------------------------------------
> > revno: 3915
> > revision-id: svoj@xxxxxxxxxxx-20131205084430-7rt735tbanw0i6uq
> > parent: svoj@xxxxxxxxxxx-20130827121233-b9mguuxctnjlq5wd
> > committer: Sergey Vojtovich <svoj@xxxxxxxxxxx>
> > branch nick: 10.0
> > timestamp: Thu 2013-12-05 12:44:30 +0400
> > message:
> >   MDEV-5388 - Reduce usage of LOCK_open: unused_tables
> >   
> >   Removed unused_tables, find LRU object by timestamp instead.
> 
> > === modified file 'sql/table_cache.cc'
> > --- a/sql/table_cache.cc	2013-08-27 12:12:33 +0000
> > +++ b/sql/table_cache.cc	2013-12-05 08:44:30 +0000
> > @@ -324,20 +248,44 @@ void tc_add_table(THD *thd, TABLE *table
> >    DBUG_ASSERT(table->in_use == thd);
> >    mysql_mutex_lock(&LOCK_open);
> >    table->s->tdc.all_tables.push_front(table);
> > -  tc_count++;
> >    /* If we have too many TABLE instances around, try to get rid of them */
> > -  if (tc_count > tc_size && unused_tables)
> > +  if (tc_count == tc_size)
> 
> I'd rather put (tc_count >= tc_size), looks safer.
(tc_count > tc_size) means all instances are used and there is nothing to evict.
tc_release_table() guards this assertion.

Anyway I changed it to ">=" in the next patch (re tc_count). This assertion is
not valid there anymore.

> 
> >    {
> > -    TABLE *purge_table= unused_tables;
> > -    tc_remove_table(purge_table);
> > -    mysql_rwlock_rdlock(&LOCK_flush);
> > +    TDC_iterator tdc_it;
> >      mysql_mutex_unlock(&LOCK_open);
> > -    intern_close_table(purge_table);
> > -    mysql_rwlock_unlock(&LOCK_flush);
> > -    check_unused(thd);
> > +
> > +    tdc_it.init();
> > +    mysql_mutex_lock(&LOCK_open);
> > +    if (tc_count == tc_size)
> 
> and here
> 
> > +    {
> > +      TABLE *purge_table= 0;
> > +      TABLE_SHARE *share;
> 
> I think a status variable for this would be nice. So that one could
> make an informed decision about the desired size of the cache.
Agree, added to my todo.

> > +      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;
> > +      }
> > +      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);
> > +        check_unused(thd);
> > +        return;
> > +      }
> > +    }
> > +    tdc_it.deinit();
> 
> Okay, so you only purge one table at time? Since purge is kind of an
> expensive operation, it's better to amortize the cost of it by purging
> many tables at once.
With current algorithm it is impossible to get multiple tables for purge here,
explanation above.

Thanks,
Sergey

Attachment: sysbench45c.ods
Description: application/vnd.oasis.opendocument.spreadsheet