maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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