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@mariadb.org-20140130123642-wsysj31e1wwlqz0o parent: psergey@askmonty.org-20140120123657-g1z4g11xmsdiw2dc committer: Sergey Vojtovich <svoj@mariadb.org> 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