Re: [Maria-developers] 8a0f331: MDEV-7943 - pthread_getspecific() takes 0.76% in OLTP RO
Hi, Sergey! On Apr 22, svoj@mariadb.org 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.
{ 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.
- { - 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 ?
return thd->killed & KILL_HARD_BIT ? THD_ABORT_ASAP : THD_ABORT_SOFTLY; }
Regards, Sergei
Hi Sergei, On Fri, May 08, 2015 at 06:52:19PM +0200, Sergei Golubchik wrote:
Hi, Sergey!
On Apr 22, svoj@mariadb.org 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
Hi, Sergey! On May 13, Sergey Vojtovich wrote:
- 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?
There's no need to change semisync plugin, if it uses current_thd, then it can also use it for thd_killed(). But if you'll require thd_killed() always to take THD as an argument, then other plugins (that work strictly within plugin api limits) won't be able to check killed flag in places where THD isn't available. May be it's not a problem, though, I don't know. Regards, Sergei
Hi Sergei, On Wed, May 13, 2015 at 05:25:21PM +0200, Sergei Golubchik wrote: > Hi, Sergey! > > On May 13, Sergey Vojtovich wrote: > > > > > > > > - 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? > > There's no need to change semisync plugin, if it uses current_thd, then > it can also use it for thd_killed(). > > But if you'll require thd_killed() always to take THD as an argument, > then other plugins (that work strictly within plugin api limits) > won't be able to check killed flag in places where THD isn't available. > May be it's not a problem, though, I don't know. I can suggest 3 alternatives: - add thd_killed_current() (now or on demand?) - add current_thd() to API (now or on demand?) - keep if (!thd), even though it makes mostly useless branch per row Which one do you like most? Thanks, Sergey
Hi, Sergey! On May 13, Sergey Vojtovich wrote:
But if you'll require thd_killed() always to take THD as an argument, then other plugins (that work strictly within plugin api limits) won't be able to check killed flag in places where THD isn't available. May be it's not a problem, though, I don't know.
I can suggest 3 alternatives: - add thd_killed_current() (now or on demand?) - add current_thd() to API (now or on demand?) - keep if (!thd), even though it makes mostly useless branch per row
Which one do you like most?
"on demand" one. Let's go with your patch. But add a comment (near DBUG_ASSERT(thd) or in the function comment) that if we'll get a use case where a plugin needs to call thd_killed and where it has no thd available, we can either add thd_killed_current() or current_thd() or do if (!thd). Regards, Sergei
participants (2)
-
Sergei Golubchik -
Sergey Vojtovich