Re: [Maria-developers] 3c6f17a130b: Allocate Session_sysvars_tracker statically

Hi, Sergey! Please, see few questions below On Mar 25, Sergey Vojtovich wrote:

revision-id: 3c6f17a130b (mariadb-10.3.12-93-g3c6f17a130b) parent(s): db9854b7d48 author: Sergey Vojtovich committer: Sergey Vojtovich timestamp: 2019-03-19 23:40:07 +0400 message:

Allocate Session_sysvars_tracker statically

One less new/delete per connection.

Removed m_mem_flag since most allocs are thread specific. The only exception are allocs performed during initialization.

Removed State_tracker and Session_tracker constructors as they don't make sense anymore.

Rather than having session_tracker.deinit(), fixed memory accounting so that it gets initialized before any other constructor and deinitialized after all destructors done. Cons: makes THD a few pointers larger :(

Part of MDEV-14984 - regression in connect performance

diff --git a/sql/sql_class.h b/sql/sql_class.h index 69e6f58f854..64d772e5d17 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -2145,13 +2145,34 @@ struct wait_for_commit

extern "C" void my_message_sql(uint error, const char *str, myf MyFlags);

+ +/** + A wrapper around thread_count. + + It must be specified as a first base class of THD, so that increment is + done before any other THD constructors and decrement - after any other THD + destructors. + + Destructor unblocks close_conneciton() if there are no more THD's left. +*/ +class THD_count +{ + THD *orig_thd; + THD_count(); + ~THD_count(); + friend class THD; +};

1. Why a separate class, what's wrong with having it in THD? No downcasts, no risk that someone would try to use it separately. 2. orig_thd looks very suspicious. You preserve the old THD* pointer for the whole duration of this THD lifetime and restore it at the end. How can you know that the "orig_thd" is still valid? Why do you need to do it anyway?

+ + /** @class THD For each client connection we create a separate thread with THD serving as a thread/connection descriptor */

-class THD :public Statement, +class THD: public THD_count, /* this must be first */

an assert would be better than "/* must be first */". Like, assert(&orig_thd == this) or something.

+ public Statement, + /* This is to track items changed during execution of a prepared statement/stored procedure. It's created by @@ -2176,19 +2197,6 @@ class THD :public Statement, inline bool is_conventional() const { DBUG_ASSERT(0); return Statement::is_conventional(); }

- void dec_thread_count(void) - { - DBUG_ASSERT(thread_count > 0); - thread_safe_decrement32(&thread_count); - signal_thd_deleted(); - } - - - void inc_thread_count(void) - { - thread_safe_increment32(&thread_count); - } - public: MDL_context mdl_context;

diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 9c9fa228e2a..afe4123a52c 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -597,8 +597,46 @@ extern "C" void thd_kill_timeout(THD* thd) thd->awake(KILL_TIMEOUT); }

-THD::THD(my_thread_id id, bool is_wsrep_applier, bool skip_global_sys_var_lock) - :Statement(&main_lex, &main_mem_root, STMT_CONVENTIONAL_EXECUTION, + +THD_count::THD_count() +{ + THD *thd= static_cast(this); + thread_safe_increment32(&thread_count); + /* + We set THR_THD to temporally point to this THD to register all the + variables that allocates memory for this THD + */ + orig_thd= current_thd; + set_current_thd(thd); + thd->status_var.local_memory_used= sizeof(THD); + thd->status_var.max_local_memory_used= thd->status_var.local_memory_used; + thd->status_var.global_memory_used= 0; + thd->variables.max_mem_used= global_system_variables.max_mem_used; +} + + +THD_count::~THD_count() +{ + THD *thd= static_cast(this); + if (thd->status_var.local_memory_used != 0) + { + DBUG_PRINT("error", ("memory_used: %lld", + thd->status_var.local_memory_used)); + SAFEMALLOC_REPORT_MEMORY(thd->thread_id); + DBUG_ASSERT(thd->status_var.local_memory_used == 0 || + !debug_assert_on_not_freed_memory); + } + update_global_memory_status(thd->status_var.global_memory_used); + set_current_thd(orig_thd); + DBUG_ASSERT(thread_count > 0); + thread_safe_decrement32(&thread_count); + signal_thd_deleted(); +} + + +THD::THD(my_thread_id id, bool is_wsrep_applier, bool skip_global_sys_var_lock): + THD_count(), + Statement(&main_lex, &main_mem_root, STMT_CONVENTIONAL_EXECUTION, /* statement id */ 0), rli_fake(0), rgi_fake(0), rgi_slave(NULL), protocol_text(this), protocol_binary(this), @@ -654,15 +692,6 @@ THD::THD(my_thread_id id, bool is_wsrep_applier, bool skip_global_sys_var_lock) ulong tmp; bzero(&variables, sizeof(variables));

- /* - We set THR_THD to temporally point to this THD to register all the - variables that allocates memory for this THD - */ - THD *old_THR_THD= current_thd; - set_current_thd(this); - status_var.local_memory_used= sizeof(THD); - status_var.max_local_memory_used= status_var.local_memory_used; - status_var.global_memory_used= 0; variables.pseudo_thread_id= thread_id; variables.max_mem_used= global_system_variables.max_mem_used; main_da.init(); @@ -849,8 +878,7 @@ THD::THD(my_thread_id id, bool is_wsrep_applier, bool skip_global_sys_var_lock) save_prep_leaf_list= FALSE; org_charset= 0; /* Restore THR_THD */ - set_current_thd(old_THR_THD); - inc_thread_count(); + set_current_thd(orig_thd); }

@@ -1589,7 +1617,6 @@ void THD::reset_for_reuse()

THD::~THD() { - THD *orig_thd= current_thd; THD_CHECK_SENTRY(this); DBUG_ENTER("~THD()"); /* Check that we have already called thd->unlink() */ @@ -1601,7 +1628,11 @@ THD::~THD() In error cases, thd may not be current thd. We have to fix this so that memory allocation counting is done correctly */ - set_current_thd(this); + orig_thd= current_thd; + if (orig_thd == this) + orig_thd= 0; + else + set_current_thd(this); if (!status_in_global) add_status_to_global();

@@ -1655,22 +1686,6 @@ THD::~THD() lf_hash_put_pins(xid_hash_pins); /* Ensure everything is freed */ status_var.local_memory_used-= sizeof(THD); - - /* trick to make happy memory accounting system */ -#ifndef EMBEDDED_LIBRARY - session_tracker.deinit(); -#endif //EMBEDDED_LIBRARY - - if (status_var.local_memory_used != 0) - { - DBUG_PRINT("error", ("memory_used: %lld", status_var.local_memory_used)); - SAFEMALLOC_REPORT_MEMORY(thread_id); - DBUG_ASSERT(status_var.local_memory_used == 0 || - !debug_assert_on_not_freed_memory); - } - update_global_memory_status(status_var.global_memory_used); - set_current_thd(orig_thd == this ? 0 : orig_thd); - dec_thread_count(); DBUG_VOID_RETURN; }

Regards, Sergei Chief Architect MariaDB and security@mariadb.org