From 19468389c7648fe1d49ccdd07a12f32eeffbb7e4 Mon Sep 17 00:00:00 2001 From: zhang-lujie <443275448@qq.com> Date: Mon, 11 Oct 2021 14:44:14 +0800 Subject: [PATCH 1/2] Optimize performance of count thread running. --- sql/event_scheduler.cc | 2 -- sql/mysqld_thd_manager.cc | 29 +++++++++++++++++++++++ sql/mysqld_thd_manager.h | 15 ++++-------- sql/sql_parse.cc | 3 --- storage/perfschema/pfs_variable.cc | 38 ++++++++++++++++++++++++++++++ unittest/gunit/thd_manager-t.cc | 8 ------- 6 files changed, 72 insertions(+), 23 deletions(-) diff --git a/sql/event_scheduler.cc b/sql/event_scheduler.cc index f7182867f586..b3c52c7d17ad 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 a477953b6cc1..b99b57f969f0 100644 --- a/sql/mysqld_thd_manager.cc +++ b/sql/mysqld_thd_manager.cc @@ -315,6 +315,35 @@ 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) {} + 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(); +} + +int Global_THD_manager::get_num_thread_running() { + return atomic_num_thread_running; +} + 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 bf04e6b623f6..d50f28d85d93 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(); /** Retrieves thread created statistic variable. diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index d27e36bc9b11..e235ba43eac1 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -1547,7 +1547,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++; @@ -2195,8 +2194,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 af1ad71a7494..c51f578bf1d1 100644 --- a/storage/perfschema/pfs_variable.cc +++ b/storage/perfschema/pfs_variable.cc @@ -1156,6 +1156,9 @@ int PFS_status_variable_cache::do_materialize_global(void) { false, /* threads */ true, /* THDs */ &visitor); + + 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. @@ -1199,6 +1202,17 @@ int PFS_status_variable_cache::do_materialize_all(THD *unsafe_thd) { init_show_var_array(OPT_SESSION, false); } + /* + It 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 we can find that LOCK_thd_list + should be locked before LOCK_thd_data. In this function, + LOCK_thd_data is locked in get_THD. If we count num in function + get_num_thread_running, the lock order is incorrect, which may + lead to dead lock. So we count num in this location. + */ + 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) { /* @@ -1248,6 +1262,17 @@ int PFS_status_variable_cache::do_materialize_session(THD *unsafe_thd) { init_show_var_array(OPT_SESSION, true); } + /* + It 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 we can find that LOCK_thd_list + should be locked before LOCK_thd_data. In this function, + LOCK_thd_data is locked in get_THD. If we count num in function + get_num_thread_running, the lock order is incorrect, which may + lead to dead lock. So we count num in this location. + */ + 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) { /* @@ -1291,6 +1316,17 @@ int PFS_status_variable_cache::do_materialize_session(PFS_thread *pfs_thread) { /* The SHOW_VAR array must be initialized externally. */ DBUG_ASSERT(m_initialized); + /* + It 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 we can find that LOCK_thd_list + should be locked before LOCK_thd_data. In this function, + LOCK_thd_data is locked in get_THD. If we count num in function + get_num_thread_running, the lock order is incorrect, which may + lead to dead lock. So we count num in this location. + */ + 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) { /* @@ -1342,6 +1378,8 @@ int PFS_status_variable_cache::do_materialize_client(PFS_client *pfs_client) { */ m_sum_client_status(pfs_client, &status_totals); + 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 22e947c6eafc..689d808d31ad 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(); From 39f768e2a7dda4d352f213f3d18ccda0a42c3398 Mon Sep 17 00:00:00 2001 From: zhang-lujie <443275448@qq.com> Date: Fri, 15 Oct 2021 15:49:47 +0800 Subject: [PATCH 2/2] Optimize performance of count thread running, modify code and add notes. --- sql/mysqld_thd_manager.cc | 4 -- sql/mysqld_thd_manager.h | 2 +- storage/perfschema/pfs_variable.cc | 73 +++++++++++++++++++++--------- 3 files changed, 53 insertions(+), 26 deletions(-) diff --git a/sql/mysqld_thd_manager.cc b/sql/mysqld_thd_manager.cc index b99b57f969f0..3a4fab45fc7b 100644 --- a/sql/mysqld_thd_manager.cc +++ b/sql/mysqld_thd_manager.cc @@ -340,10 +340,6 @@ void Global_THD_manager::count_num_thread_running() { atomic_num_thread_running = count_thread_running.get_count(); } -int Global_THD_manager::get_num_thread_running() { - return atomic_num_thread_running; -} - 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 d50f28d85d93..5be19f548668 100644 --- a/sql/mysqld_thd_manager.h +++ b/sql/mysqld_thd_manager.h @@ -156,7 +156,7 @@ class Global_THD_manager { Retrieves thread running statistic variable. @return int Returns the total number of threads currently running */ - int get_num_thread_running(); + int get_num_thread_running() const { return atomic_num_thread_running; } /** Retrieves thread created statistic variable. diff --git a/storage/perfschema/pfs_variable.cc b/storage/perfschema/pfs_variable.cc index c51f578bf1d1..d2754ca97627 100644 --- a/storage/perfschema/pfs_variable.cc +++ b/storage/perfschema/pfs_variable.cc @@ -1157,6 +1157,14 @@ int PFS_status_variable_cache::do_materialize_global(void) { 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(); /* @@ -1203,13 +1211,18 @@ int PFS_status_variable_cache::do_materialize_all(THD *unsafe_thd) { } /* - It 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 we can find that LOCK_thd_list - should be locked before LOCK_thd_data. In this function, - LOCK_thd_data is locked in get_THD. If we count num in function - get_num_thread_running, the lock order is incorrect, which may - lead to dead lock. So we count num in this location. + 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(); @@ -1263,13 +1276,18 @@ int PFS_status_variable_cache::do_materialize_session(THD *unsafe_thd) { } /* - It 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 we can find that LOCK_thd_list - should be locked before LOCK_thd_data. In this function, - LOCK_thd_data is locked in get_THD. If we count num in function - get_num_thread_running, the lock order is incorrect, which may - lead to dead lock. So we count num in this location. + 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(); @@ -1317,13 +1335,18 @@ int PFS_status_variable_cache::do_materialize_session(PFS_thread *pfs_thread) { DBUG_ASSERT(m_initialized); /* - It 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 we can find that LOCK_thd_list - should be locked before LOCK_thd_data. In this function, - LOCK_thd_data is locked in get_THD. If we count num in function - get_num_thread_running, the lock order is incorrect, which may - lead to dead lock. So we count num in this location. + 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(); @@ -1378,6 +1401,14 @@ 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(); /*