maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

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@xxxxxxxxxxx-20130827121233-xh1uyhgfwbhedqyf
> > parent: jplindst@xxxxxxxxxxx-20130823060357-pww92qxla7o8iir7
> > committer: Sergey Vojtovich <svoj@xxxxxxxxxxx>
> > 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