From 308077503221109044fe6a418036c7575745803a Mon Sep 17 00:00:00 2001 From: nbuxrr Date: Mon, 19 Sep 2022 11:57:10 +0800 Subject: [PATCH] mod: thread counter optimization --- sql/event_scheduler.cc | 2 - sql/mysqld_thd_manager.cc | 26 +++++++++++ sql/mysqld_thd_manager.h | 15 +++---- sql/sql_parse.cc | 3 -- storage/perfschema/pfs_variable.cc | 69 ++++++++++++++++++++++++++++++ unittest/gunit/thd_manager-t.cc | 8 ---- 6 files changed, 100 insertions(+), 23 deletions(-) diff --git a/sql/event_scheduler.cc b/sql/event_scheduler.cc index c8a9ec1eb434..e4a491f98cbc 100644 --- a/sql/event_scheduler.cc +++ b/sql/event_scheduler.cc @@ -170,7 +170,6 @@ bool post_init_event_thread(THD *thd) { Global_THD_manager *thd_manager = Global_THD_manager::get_instance(); thd_manager->add_thd(thd); - thd_manager->inc_thread_running(); return false; } @@ -190,7 +189,6 @@ void deinit_event_thread(THD *thd) { DBUG_PRINT("exit", ("Event thread finishing")); thd->release_resources(); thd_manager->remove_thd(thd); - thd_manager->dec_thread_running(); delete thd; } diff --git a/sql/mysqld_thd_manager.cc b/sql/mysqld_thd_manager.cc index 902fe2b2c610..56f7aab2610d 100644 --- a/sql/mysqld_thd_manager.cc +++ b/sql/mysqld_thd_manager.cc @@ -315,6 +315,32 @@ THD *Global_THD_manager::find_thd(Find_thd_with_id *func) { return nullptr; } +/** + This class implements callback for do_for_all_thd(). + It counts the total number of running threads + from global thread list. +*/ +class Count_thread_running : public Do_THD_Impl { + public: + Count_thread_running() : m_count(0) {} + ~Count_thread_running() {} + virtual void operator()(THD *thd) { + if (thd->get_command() != COM_SLEEP) { + m_count++; + } + } + int get_count() { return m_count; } + + private: + int m_count; +}; + +void Global_THD_manager::count_num_thread_running() { + Count_thread_running count_thread_running; + do_for_all_thd(&count_thread_running); + atomic_num_thread_running = count_thread_running.get_count(); +} + void inc_thread_created() { Global_THD_manager::get_instance()->inc_thread_created(); } diff --git a/sql/mysqld_thd_manager.h b/sql/mysqld_thd_manager.h index 046997db6852..70dcea1ef8a9 100644 --- a/sql/mysqld_thd_manager.h +++ b/sql/mysqld_thd_manager.h @@ -148,20 +148,15 @@ class Global_THD_manager { void remove_thd(THD *thd); /** - Retrieves thread running statistic variable. - @return int Returns the total number of threads currently running + Count thread running statistic variable. */ - int get_num_thread_running() const { return atomic_num_thread_running; } + void count_num_thread_running(); /** - Increments thread running statistic variable. - */ - void inc_thread_running() { atomic_num_thread_running++; } - - /** - Decrements thread running statistic variable. + Retrieves thread running statistic variable. + @return int Returns the total number of threads currently running */ - void dec_thread_running() { atomic_num_thread_running--; } + int get_num_thread_running() const { return atomic_num_thread_running; } /** Retrieves thread created statistic variable. diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 52f7f16431eb..e4952a63a206 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -1599,7 +1599,6 @@ bool dispatch_command(THD *thd, const COM_DATA *com_data, } thd->set_query_id(next_query_id()); thd->reset_rewritten_query(); - thd_manager->inc_thread_running(); if (!(server_command_flags[command] & CF_SKIP_QUESTIONS)) thd->status_var.questions++; @@ -2265,8 +2264,6 @@ bool dispatch_command(THD *thd, const COM_DATA *com_data, /* Prevent rewritten query from getting "stuck" in SHOW PROCESSLIST. */ thd->reset_rewritten_query(); - thd_manager->dec_thread_running(); - /* Freeing the memroot will leave the THD::work_part_info invalid. */ thd->work_part_info = nullptr; diff --git a/storage/perfschema/pfs_variable.cc b/storage/perfschema/pfs_variable.cc index d33b1a488899..15a9f41e87da 100644 --- a/storage/perfschema/pfs_variable.cc +++ b/storage/perfschema/pfs_variable.cc @@ -1157,6 +1157,17 @@ int PFS_status_variable_cache::do_materialize_global(void) { false, /* threads */ true, /* THDs */ &visitor); + + /* + Because of the reason described in + PFS_status_variable_cache::do_materialize_all(THD *unsafe_thd), + PFS_status_variable_cache::do_materialize_session(THD *unsafe_thd) and + PFS_status_variable_cache::do_materialize_session(PFS_thread *pfs_thread), + count_num_thread_running() cannot put together with + get_num_thread_running(), so count_num_thread_running() is put here. + */ + Global_THD_manager::get_instance()->count_num_thread_running(); + /* Build the status variable cache using the SHOW_VAR array as a reference. Use the status totals collected from all threads. @@ -1200,6 +1211,22 @@ int PFS_status_variable_cache::do_materialize_all(THD *unsafe_thd) { init_show_var_array(OPT_SESSION, false); } + /* + count_num_thread_running() counts the total number of running threads + from global thread list, using LOCK_thd_list to protect sharded + global thread list. In lock_order_dependencies.txt, the lock order + is that LOCK_thd_list must be locked before LOCK_thd_data. In this + function, LOCK_thd_data is already locked in get_THD(), Then manifest() + will call get_num_thread_running(). If get_num_thread_running() counts + and returns the num, the lock order will be incorrect, which may + lead to dead lock. To prevent this situation, get_num_thread_running() + is split into two part, one is still called get_num_thread_running() + which returns the num, the other is called count_num_thread_running() + which counts the num and should be called before get_THD() and + get_num_thread_running(). So count_num_thread_running() is put here. + */ + Global_THD_manager::get_instance()->count_num_thread_running(); + /* Get and lock a validated THD from the thread manager. */ if ((m_safe_thd = get_THD(unsafe_thd)) != nullptr) { /* @@ -1249,6 +1276,22 @@ int PFS_status_variable_cache::do_materialize_session(THD *unsafe_thd) { init_show_var_array(OPT_SESSION, true); } + /* + count_num_thread_running() counts the total number of running threads + from global thread list, using LOCK_thd_list to protect sharded + global thread list. In lock_order_dependencies.txt, the lock order + is that LOCK_thd_list must be locked before LOCK_thd_data. In this + function, LOCK_thd_data is already locked in get_THD(), Then manifest() + will call get_num_thread_running(). If get_num_thread_running() counts + and returns the num, the lock order will be incorrect, which may + lead to dead lock. To prevent this situation, get_num_thread_running() + is split into two part, one is still called get_num_thread_running() + which returns the num, the other is called count_num_thread_running() + which counts the num and should be called before get_THD() and + get_num_thread_running(). So count_num_thread_running() is put here. + */ + Global_THD_manager::get_instance()->count_num_thread_running(); + /* Get and lock a validated THD from the thread manager. */ if ((m_safe_thd = get_THD(unsafe_thd)) != nullptr) { /* @@ -1292,6 +1335,22 @@ int PFS_status_variable_cache::do_materialize_session(PFS_thread *pfs_thread) { /* The SHOW_VAR array must be initialized externally. */ assert(m_initialized); + /* + count_num_thread_running() counts the total number of running threads + from global thread list, using LOCK_thd_list to protect sharded + global thread list. In lock_order_dependencies.txt, the lock order + is that LOCK_thd_list must be locked before LOCK_thd_data. In this + function, LOCK_thd_data is already locked in get_THD(), Then manifest() + will call get_num_thread_running(). If get_num_thread_running() counts + and returns the num, the lock order will be incorrect, which may + lead to dead lock. To prevent this situation, get_num_thread_running() + is split into two part, one is still called get_num_thread_running() + which returns the num, the other is called count_num_thread_running() + which counts the num and should be called before get_THD() and + get_num_thread_running(). So count_num_thread_running() is put here. + */ + Global_THD_manager::get_instance()->count_num_thread_running(); + /* Get and lock a validated THD from the thread manager. */ if ((m_safe_thd = get_THD(pfs_thread)) != nullptr) { /* @@ -1343,6 +1402,16 @@ int PFS_status_variable_cache::do_materialize_client(PFS_client *pfs_client) { */ m_sum_client_status(pfs_client, &status_totals); + /* + Because of the reason described in + PFS_status_variable_cache::do_materialize_all(THD *unsafe_thd), + PFS_status_variable_cache::do_materialize_session(THD *unsafe_thd) and + PFS_status_variable_cache::do_materialize_session(PFS_thread *pfs_thread), + count_num_thread_running() cannot put together with + get_num_thread_running(), so count_num_thread_running() is put here. + */ + Global_THD_manager::get_instance()->count_num_thread_running(); + /* Build the status variable cache using the SHOW_VAR array as a reference and the status totals collected from threads associated with this client. diff --git a/unittest/gunit/thd_manager-t.cc b/unittest/gunit/thd_manager-t.cc index 1e2efa42011a..4c0aa1bdbafd 100644 --- a/unittest/gunit/thd_manager-t.cc +++ b/unittest/gunit/thd_manager-t.cc @@ -82,14 +82,6 @@ TEST_F(ThreadManagerTest, AddRemoveTHDWithGuard) { EXPECT_EQ(0U, thd_manager->get_thd_count()); } -TEST_F(ThreadManagerTest, IncDecThreadRunning) { - EXPECT_EQ(0, thd_manager->get_num_thread_running()); - thd_manager->inc_thread_running(); - EXPECT_EQ(1, thd_manager->get_num_thread_running()); - thd_manager->dec_thread_running(); - EXPECT_EQ(0, thd_manager->get_num_thread_running()); -} - TEST_F(ThreadManagerTest, IncThreadCreated) { EXPECT_EQ(0U, thd_manager->get_num_thread_created()); thd_manager->inc_thread_created();