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@mariadb.org-20131205084430-7rt735tbanw0i6uq parent: svoj@mariadb.org-20130827121233-b9mguuxctnjlq5wd committer: Sergey Vojtovich <svoj@mariadb.org> 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