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: [email protected] > > parent: [email protected] > > committer: Sergey Vojtovich <[email protected]> > > 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 _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
