Re: [Maria-developers] Rev 3915: MDEV-5388 - Reduce usage of LOCK_open: unused_tables
Hi, Sergey! Did you benchmark the improvement you get with this patch? 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.
{ - 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.
+ 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.
} - else - mysql_mutex_unlock(&LOCK_open); + /* Nothing to evict, increment tc_count. */ + tc_count++; + mysql_mutex_unlock(&LOCK_open); }
2 Regards, Sergei
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
Hi, Sergey! On Dec 11, Sergey Vojtovich wrote:
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.
Ah, I see, thanks. Great!
Also please find Axel's results (MDEV-4956 + MDEV-5388) attached. From Axel's results please note performance regression between 10.0.4 and 10.0.5.
Why is that, do you know?
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.
But you said that in the next patch it becomes possible, or did I misunderstand? Regards, Sergei
Hi Sergei, On Wed, Dec 11, 2013 at 09:34:02PM +0100, Sergei Golubchik wrote:
Hi, Sergey!
On Dec 11, Sergey Vojtovich wrote:
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.
Ah, I see, thanks. Great!
Also please find Axel's results (MDEV-4956 + MDEV-5388) attached. From Axel's results please note performance regression between 10.0.4 and 10.0.5.
Why is that, do you know? Not yet, but I plan to investigate it next week.
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.
But you said that in the next patch it becomes possible, or did I misunderstand?
Yes, it becomes possible, but I'd say it is headache of the next patch. It becomes possible when two (or more) tc_add_table() executed concurrently: thr1: tc_count++ thr2: tc_count++ // Now we will try to evict 2 tables. thr1: <LOCK_open>start expensive eviction</LOCK_open> thr2: <LOCK_open>start expensive eviction</LOCK_open> But for now it is like: thr1: <LOCK_open>tc_count++; start expensive eviction</LOCK_open> thr2: <LOCK_open>tc_count++; start expensive eviction</LOCK_open> Regards, Sergey
Hi, Sergey! On Dec 12, Sergey Vojtovich wrote:
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. But you said that in the next patch it becomes possible, or did I misunderstand? Yes, it becomes possible, but I'd say it is headache of the next patch.
I mean something simpler, actually. Like, when a limit is reached, purge 10 (or 20, or 100, or 10%) tables at once. Regards, Sergei
Hi Sergei, yes, it may make sense. What if I do some testing and implement it along with MDEV-5403 or a separate task? Thanks, Sergey On Thu, Dec 12, 2013 at 11:23:57AM +0100, Sergei Golubchik wrote:
Hi, Sergey!
On Dec 12, Sergey Vojtovich wrote:
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. But you said that in the next patch it becomes possible, or did I misunderstand? Yes, it becomes possible, but I'd say it is headache of the next patch.
I mean something simpler, actually. Like, when a limit is reached, purge 10 (or 20, or 100, or 10%) tables at once.
Regards, Sergei
Hi, Sergey! On Dec 12, Sergey Vojtovich wrote:
yes, it may make sense. What if I do some testing and implement it along with MDEV-5403 or a separate task?
Sounds great!
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. I mean something simpler, actually. Like, when a limit is reached, purge 10 (or 20, or 100, or 10%) tables at once.
Regards, Sergei
participants (2)
-
Sergei Golubchik -
Sergey Vojtovich