New subject: [Maria-developers] [svoj@mariadb.org: [Commits] Rev 3807: MDEV-4956 - Reduce usage of LOCK_open: TABLE_SHARE::tdc.used_tables in lp:maria/10.0]

9 Dec 2013

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 
> > 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 . 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

					 0

				 0

            Reply

                Sign in to reply online

            Use email software

                       Sergey Vojtovich

                4:05 p.m.

          New subject: [Maria-developers] [svoj@mariadb.org: [Commits] Rev 3807: MDEV-4956 - Reduce usage of LOCK_open: TABLE_SHARE::tdc.used_tables in lp:maria/10.0]

      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 
> > > 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 

    . 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

					 0

				 0

            Reply

                Sign in to reply online

            Use email software

                    Sergei Golubchik

                4:49 p.m.

          New subject: [Maria-developers] [svoj@mariadb.org: [Commits] Rev 3807: MDEV-4956 - Reduce usage of LOCK_open: TABLE_SHARE::tdc.used_tables in lp:maria/10.0]

      Hi, Sergey!

On Dec 10, Sergey Vojtovich wrote:
...
- in subsequent patches tc_acquire_table() becomes so simple, so one can't say
  LOCK_tdc is held more than necessary
Are these subsequent patches ready? Can I see them?
Or did you mean the next version of this patch?

Regards,
Sergei

					 0

				 0

            Reply

                Sign in to reply online

            Use email software

                       Sergey Vojtovich

                5 p.m.

          New subject: [Maria-developers] [svoj@mariadb.org: [Commits] Rev 3807: MDEV-4956 - Reduce usage of LOCK_open: TABLE_SHARE::tdc.used_tables in lp:maria/10.0]

      Hi Sergei,

yes, these patches are at commits@:
MDEV-5388 - Reduce usage of LOCK_open: unused_tables
MDEV-5403 - Reduce usage of LOCK_open: tc_count

Thanks,
Sergey

On Tue, Dec 10, 2013 at 04:49:02PM +0100, Sergei Golubchik wrote:
...
Hi, Sergey!
On Dec 10, Sergey Vojtovich wrote:
...
- in subsequent patches tc_acquire_table() becomes so simple, so one can't say
  LOCK_tdc is held more than necessary
Are these subsequent patches ready? Can I see them?
Or did you mean the next version of this patch?
Regards,
Sergei

					 0

				 0

            Reply

                Sign in to reply online

            Use email software

        3933

          Age (days ago)

        3934

          Last active (days ago)

            List overview

         Download

        4 comments

        2 participants

                Add to favorites

                Remove from favorites

        tags 

        participants (2)

                    Sergei Golubchik

                    Sergey Vojtovich

            Powered by HyperKitty version 1.3.9.

Re: [Maria-developers] [svoj@mariadb.org: [Commits] Rev 3807: MDEV-4956 - Reduce usage of LOCK_open: TABLE_SHARE::tdc.used_tables in lp:maria/10.0]

Sergei Golubchik

Sergey Vojtovich

Sergey Vojtovich

Sergei Golubchik

Sergey Vojtovich

tags

participants (2)