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 <s...@mariadb.org>
> committer: Sergey Vojtovich <s...@mariadb.org>
> 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<THD*>(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<THD*>(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 secur...@mariadb.org
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
