Hi Sergei,

On Tue, Dec 10, 2013 at 10:24:22AM +0400, Sergey Vojtovich wrote:
> 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.
Fixed in updated 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.
Fixed in updated patch.

Regards,
Sergey