10 Dec
2013
10 Dec
'13
6:24 a.m.
Hi Sergei, thanks for your review, answers inline. On Mon, Dec 09, 2013 at 10:30:43PM +0100, Sergei Golubchik wrote: > Hi, Sergey! > > Looks pretty much ok. See few comments/questions below. > > On Oct 24, Sergey Vojtovich wrote: > > ------------------------------------------------------------ > > revno: 3807 > > revision-id: svoj@mariadb.org-20130827121233-xh1uyhgfwbhedqyf > > parent: jplindst@mariadb.org-20130823060357-pww92qxla7o8iir7 > > committer: Sergey Vojtovich <svoj@mariadb.org> > > branch nick: 10.0 > > timestamp: Tue 2013-08-27 16:12:33 +0400 > > message: > > MDEV-4956 - Reduce usage of LOCK_open: TABLE_SHARE::tdc.used_tables > > > > - tc_acquire_table and tc_release_table do not access > > TABLE_SHARE::tdc.used_tables anymore > > ok > > > - in tc_acquire_table(): release LOCK_tdc after we relase LOCK_open > > (saves a few CPU cycles in critical section) > > This is a bit counter-intuitive. Your change is to hold the lock more > than necessary. Does it give a measurable performance improvement? The reasons are: - yes, there was something like ~2% performance improvement, though I have no exact numbers now - it is rdlock(LOCK_tdc), which is not that much contested - in subsequent patches tc_acquire_table() becomes so simple, so one can't say LOCK_tdc is held more than necessary > > > - in tc_release_table(): if we reached table cache threshold, evict > > to-be-released table without moving it to unused_tables. unused_tables > > must be empty at this point (added assertion). > > Why unused_tables must be empty at this point? There're two guardians looking after table cache threshold: tc_add_table() and tc_release_table(). First one tc_add_table(): here we may overrun table cache threshold only if there is nothing to evict (=all tables are used). In tc_release_table() we just need to check this overrun. But just recently I thought of one case when the above is broken: SET table_open_cache= table_open_cache - X; I plan to fix the above in coming patches by flushing table cache when it's size is reduced. Assertion is already removed in subsequent patch. > > > === modified file 'sql/sql_base.cc' > > --- a/sql/sql_base.cc 2013-08-14 08:48:50 +0000 > > +++ b/sql/sql_base.cc 2013-08-27 12:12:33 +0000 > > @@ -370,7 +372,7 @@ void free_io_cache(TABLE *table) > > > > void kill_delayed_threads_for_table(TABLE_SHARE *share) > > { > > - TABLE_SHARE::TABLE_list::Iterator it(share->tdc.used_tables); > > + TABLE_SHARE::All_share_tables_list::Iterator it(share->tdc.all_tables); > > In this function, I'd suggest to add some flag to be able to skip the > mutex and the loop altogether - as in the absolute majority of cases > there will be no delayed insert thread to kill. > > Either use a global flag to know if there's a delayed thread running, > or create a per-share flag. It should be called only for FLUSH TABLE <table>. And I plan to make tdc.all_tables protected by per-share lock. Said the above, is it worth to bother? Anyway it will be pretty simple and short fix because there is global delayed_insert_threads counter. Thanks, Sergey