maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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 <svoj@xxxxxxxxxxx>
> committer: Sergey Vojtovich <svoj@xxxxxxxxxxx>
> 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 security@xxxxxxxxxxx