Hi, Monty! On Jan 23, Michael Widenius wrote:

revision-id: 51fa0272febd782674b3d44828718e805c44c1a8 parent(s): ab440b0fb7302d707ba0ba41382bf911404db1cb committer: Michael Widenius branch nick: maria-10.1 timestamp: 2015-01-23 14:32:53 +0200 message:

Improve performance for calculating memory allocation

See my comments below:

diff --git a/include/mysql/plugin.h b/include/mysql/plugin.h index 16b0c6e..9ae0fe8 100644 --- a/include/mysql/plugin.h +++ b/include/mysql/plugin.h @@ -190,6 +196,7 @@ struct st_mysql_show_var {

#define SHOW_VAR_FUNC_BUFF_SIZE (256 * sizeof(void*)) typedef int (*mysql_show_var_func)(MYSQL_THD, struct st_mysql_show_var*, char *); +typedef int (*mysql_show_var_extended_func)(MYSQL_THD, struct st_mysql_show_var*, enum enum_var_type, char *);

I'd put scope the last making mysql_show_var_extended_func backward compatible with mysql_show_var_func. this makes some code simpler, see below.

/* diff --git a/sql/sql_show.cc b/sql/sql_show.cc index 044b60d..8eed7b9 100644 --- a/sql/sql_show.cc +++ b/sql/sql_show.cc @@ -3115,9 +3115,16 @@ static bool show_status_array(THD *thd, const char *wild, if var->type is SHOW_FUNC or SHOW_SIMPLE_FUNC, call the function. Repeat as necessary, if new var is again one of the above */ - for (var=variables; var->type == SHOW_FUNC || - var->type == SHOW_SIMPLE_FUNC; var= &tmp) - ((mysql_show_var_func)(var->value))(thd, &tmp, buff); + for (var=variables; + var->type == SHOW_FUNC || var->type == SHOW_SIMPLE_FUNC || + var->type == SHOW_EXTENDED_FUNC; + var= &tmp) + { + if (var->type == SHOW_EXTENDED_FUNC) + ((mysql_show_var_extended_func)(var->value))(thd, &tmp, scope, buff); + else + ((mysql_show_var_func)(var->value))(thd, &tmp, buff);

You won't need this if() when scope is the last argument. In fact I'm still thinking whether it would be a good idea to add scope to mysql_show_var_func and SHOW_FUNC, without introducing SHOW_EXTENDED_FUNC at all. This wouldn't break any existing plugin, as it's perfectly binary compatible extension. Also, see below.

+ }

SHOW_TYPE show_type=var->type; if (show_type == SHOW_ARRAY) diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 0d49407..d200af6 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -1747,6 +1749,16 @@ void add_to_status(STATUS_VAR *to_var, STATUS_VAR *from_var) to_var->binlog_bytes_written+= from_var->binlog_bytes_written; to_var->cpu_time+= from_var->cpu_time; to_var->busy_time+= from_var->busy_time; + to_var->local_memory_used+= from_var->local_memory_used;

why? it's not additive, right? and global_status_var.local_memory_used is never shown anyway.

+ + /* + Update global_memory_used. We have to do this with atomic_add as the + global value can change outside of LOCK_status. + */ + // workaround for gcc 4.2.4-1ubuntu4 -fPIE (from DEB_BUILD_HARDENING=1) + int64 volatile * volatile ptr= &to_var->global_memory_used;

I'd rather remove this workaround. 4.2.4 is ancient, and quite probably this bug doesn't happen with MY_MEMORY_ORDER_RELAXED at all

+ my_atomic_add64_explicit(ptr, from_var->global_memory_used, + MY_MEMORY_ORDER_RELAXED); }

/* @@ -1784,6 +1796,11 @@ void add_diff_to_status(STATUS_VAR *to_var, STATUS_VAR *from_var, dec_var->binlog_bytes_written; to_var->cpu_time+= from_var->cpu_time - dec_var->cpu_time; to_var->busy_time+= from_var->busy_time - dec_var->busy_time; + + /* + We don't need to accumulate memory_used as these are not after + this function is called.

Sorry, I cannot parse that :(

+ */ }

#define SECONDS_TO_WAIT_FOR_KILL 2 diff --git a/sql/mysqld.cc b/sql/mysqld.cc index ea53e47..c4e1dc3 100644 --- a/sql/mysqld.cc +++ b/sql/mysqld.cc @@ -3972,19 +3972,24 @@ static void my_malloc_size_cb_func(long long size, my_bool is_thread_specific) if (thd) { DBUG_PRINT("info", ("memory_used: %lld size: %lld", - (longlong) thd->status_var.memory_used, size)); + (longlong) thd->status_var.local_memory_used, + size)); - thd->status_var.memory_used+= size; + thd->status_var.local_memory_used+= size; - DBUG_ASSERT((longlong) thd->status_var.memory_used >= 0); + DBUG_ASSERT((longlong) thd->status_var.local_memory_used >= 0); } } } + else if (likely(thd)) + thd->status_var.global_memory_used+= size;

Hmm, ok. So, you think that current_thd is cheaper than updating a global variable?

+ else + { // workaround for gcc 4.2.4-1ubuntu4 -fPIE (from DEB_BUILD_HARDENING=1) - int64 volatile * volatile ptr=&global_status_var.memory_used; + int64 volatile * volatile ptr=&global_status_var.global_memory_used;

I would've removed this workaround too

my_atomic_add64_explicit(ptr, size, MY_MEMORY_ORDER_RELAXED); + } } }

- static int init_common_variables() { umask(((~my_umask) & 0666)); @@ -8028,6 +8033,21 @@ static int show_default_keycache(THD *thd, SHOW_VAR *var, char *buff) return 0; }

+ +static int show_memory_used(THD *thd, SHOW_VAR *var, enum enum_var_type scope, + char *buff) +{ + var->type= SHOW_LONGLONG; + var->value= buff; + if (scope == OPT_GLOBAL) + *(longlong*) buff= (global_status_var.local_memory_used + + global_status_var.global_memory_used); + else + *(longlong*) buff= thd->status_var.local_memory_used; + return 0;

I'm surprised you haven't added SHOW_EXTENDED_SIMPLE_FUNC :) That would've make a very confusing name though. May be we should indeed add scope as the last argument to mysql_show_var_func and reuse both SHOW_SIMPLE_FUNC and SHOW_FUNC? I'm starting to think it's a good thing to do. Makes the API simpler and the number of choices to understand - smaller. If you do that - please don't forget to increment MARIA_PLUGIN_INTERFACE_VERSION.

+} + + #ifndef DBUG_OFF static int debug_status_func(THD *thd, SHOW_VAR *var, char *buff) {

Regards, Sergei