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