maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Sergei,

On Fri, May 08, 2015 at 06:52:19PM +0200, Sergei Golubchik wrote:
> Hi, Sergey!
> 
> On Apr 22, svoj@xxxxxxxxxxx wrote:
> > revision-id: 8a0f3310275f7b4fa445f907140f677910e18999
> > parent(s): 7feee74dd30c96bd50d1c90e4ce3b06a656b17a5
> > committer: Sergey Vojtovich
> > branch nick: mariadb
> > timestamp: 2015-04-22 14:18:51 +0400
> > message:
> > 
> > MDEV-7943 - pthread_getspecific() takes 0.76% in OLTP RO
> > 
> > Avoid calling current_thd from thd_kill_level(). This reduces number of
> > pthread_getspecific() calls from 776 to 354.
> > 
> > Also thd_kill_level(NULL) is not permitted anymore: this saves one condition.
> > 
> > ---
> >  plugin/semisync/semisync_master.cc |  4 ++--
> >  sql/sql_class.cc                   | 20 +++++++++-----------
> >  2 files changed, 11 insertions(+), 13 deletions(-)
> > 
> > diff --git a/plugin/semisync/semisync_master.cc b/plugin/semisync/semisync_master.cc
> > index c88c162..b1f7fbd 100644
> > --- a/plugin/semisync/semisync_master.cc
> > +++ b/plugin/semisync/semisync_master.cc
> > @@ -635,7 +635,7 @@ int ReplSemiSyncMaster::commitTrx(const char* trx_wait_binlog_name,
> >                              (int)is_on());
> >      }
> >  
> > -    while (is_on() && !thd_killed(NULL))
> > +    while (is_on() && !thd_killed(current_thd))
> 
> thd_killed() is a function of the kill_statement service
> (see include/mysql/service_kill_statement.h). It is created for plugins
> to use. But current_thd is not. Plugins generally have no access to it.
Speaking of general sanity: there's no other code that does thd_killed(NULL).
Speaking of semisync plugin sanity: it does lots of direct server function calls
already, including current_thd.

Do you think it is worth to preserve this thd_killed(NULL)?
I believe it is quite easy to extend replication plugin interface so that THD
is passed through. But is it worth it?

> 
> >      {
> >        if (reply_file_name_inited_)
> >        {
> > diff --git a/sql/sql_class.cc b/sql/sql_class.cc
> > index f273974..d32b14d 100644
> > --- a/sql/sql_class.cc
> > +++ b/sql/sql_class.cc
> > @@ -4208,20 +4208,18 @@ void THD::restore_backup_open_tables_state(Open_tables_backup *backup)
> >  */
> >  extern "C" enum thd_kill_levels thd_kill_level(const MYSQL_THD thd)
> >  {
> > -  THD* current= current_thd;
> > -
> > -  if (!thd)
> > -    thd= current;
> > -
> > -  if (thd == current)
> 
> I wonder if there's a cheaper way to check "if THD is current_thd".
> 
> E.g. THD stores the "end of stack" value. If it'll also store "beginning
> of stack", one can easily check whether a local variable is within these
> limits, which would mean the THD is current.
Hmm... why? We only need to check this when we have APC requests, which is
not a hot path.

> 
> > -  {
> > -    Apc_target *apc_target= (Apc_target*)&thd->apc_target;
> > -    if (apc_target->have_apc_requests())
> > -        apc_target->process_apc_requests(); 
> > -  }
> > +  DBUG_ASSERT(thd);
> >  
> >    if (likely(thd->killed == NOT_KILLED))
> > +  {
> > +    Apc_target *apc_target= (Apc_target*) &thd->apc_target;
> > +    if (unlikely(apc_target->have_apc_requests()))
> > +    {
> > +      if (thd == current_thd)
> > +        apc_target->process_apc_requests();
> > +    }
> >      return THD_IS_NOT_KILLED;
> > +  }
> 
> Why did you put process_apc_requests() under thd->killed == NOT_KILLED ?
Mostly to be inline with THD::check_killed(). We anyway process APC requests
when statement ends (see Apc_target::disable()).

Thanks,
Sergey