List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:January 25 2011 9:48am
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (tor.didriksen:3245)
WL#5257
View as plain text  
Hello,

Tor Didriksen a écrit, Le 12.01.2011 09:33:
> #At file:///export/home/didrik/repo/next-mr-opt-backporting-wl4800/ based on
> revid:tor.didriksen@stripped
> 
>  3245 Tor Didriksen	2011-01-12
>       WL#5257 Review comments.
>      @ sql/opt_trace.cc
>         use inline functions rather than arrays.
>         more readable, and faster (no cache misses)

Using two-cell arrays was precisely an attempt to avoid if()s of
   return requires_key ? "object" : "array";
because if() are said to be slow, when they are wrongly predicted.

I wrote this simple program, which isn't representative of the complex 
mysqld system, alas, but tries to compare the ternary operator versus 
the array.

#ifndef WITHIF
int val[]= {-10, 11};
#endif
int main()
{
   int y= 0;
   for (int i= 0; i<1000000000; i++)
   {
#ifdef WITHIF
     y+= (i%2) ? 11 : -10;
#else
     y+= val[i%2];
#endif
   }
   return y;
}
Compiling with g++ -O3, with the ternary operator the program runs in 
1.6 sec; with the array it runs in 1.2 secs.

Looking at code, types[] isn't very useful anyway, it's just to be able 
to print
"starting array"
"starting object"
in DBUG. A generic
"starting struct"
would not be much worse, because the type can usually be inferred from 
the surrounding DBUG printouts.
So I vote for just deleting types[].

Remains brackets[]. I'd like to keep it as an array, if possible...

>      @ sql/opt_trace.h
>         requires_key should be bool, rather than int8

It used to be bool. But Jorgen didn't like usage of bool as an array 
subscript so I changed to int8 to make him happy :-). If I have a 
choice, I'd prefer to go back to bool, which is guaranteed by the 
standard to evaluate to 0/1.

>         remove the static arrays brackets[] and types[], no need to expose this in
> the header file.

Agree to at least making them static in opt_trace.cc.

>     modified:
>       sql/opt_trace.cc
>       sql/opt_trace.h
> === modified file 'sql/opt_trace.cc'
> --- a/sql/opt_trace.cc	2011-01-11 08:15:46 +0000
> +++ b/sql/opt_trace.cc	2011-01-12 08:32:58 +0000
> @@ -31,10 +31,25 @@
>  
>  /* Opt_trace_struct class */
>  
> -const char Opt_trace_struct::brackets[]= { '[', '{', ']', '}' };
> -const char *Opt_trace_struct::types[]= { "array", "object" };
>  bool Opt_trace_struct::dbug_assert_on_syntax_error= true;
>  
> +
> +namespace {
> +inline const char *object_type(bool requires_key)
> +{
> +  return requires_key ? "object" : "array";
> +}
> +inline const char *open_bracket(bool requires_key)
> +{
> +  return requires_key ? "{" : "[";
> +}
> +inline const char *close_bracket(bool requires_key)
> +{
> +  return requires_key ? "}" : "]";
> +}
> +} // namespace
> +
> +
>  void Opt_trace_struct::syntax_error(const char *key)
>  {
>    DBUG_PRINT("opt_trace", ("syntax error key: %s", key));
> @@ -66,7 +81,7 @@ void Opt_trace_struct::do_construct(Opt_
>    saved_key= key;
>    requires_key= requires_key_arg;
>  
> -  DBUG_PRINT("opt_trace", ("%s: starting %s", key, types[requires_key]));
> +  DBUG_PRINT("opt_trace", ("%s: starting %s", key, object_type(requires_key)));
>    stmt= stmt_arg;
>    parent= parent_arg;
>    DBUG_ASSERT(parent == NULL || stmt == parent->stmt);
> @@ -133,7 +148,7 @@ void Opt_trace_struct::do_construct(Opt_
>    stmt->current_struct= this;
>    if (!stmt->support_I_S)
>      return;
> -  stmt->buffer.append(brackets[requires_key]);
> +  stmt->buffer.append(open_bracket(requires_key));
>    stmt->push();
>  }
>  
> @@ -179,7 +194,8 @@ void Opt_trace_struct::add_struct(const 
>  
>  void Opt_trace_struct::do_destruct()
>  {
> -  DBUG_PRINT("opt_trace", ("%s: ending %s", saved_key, types[requires_key]));
> +  DBUG_PRINT("opt_trace", ("%s: ending %s",
> +                           saved_key, object_type(requires_key)));
>    DBUG_ASSERT(started);
>    /*
>      Note:
> @@ -196,7 +212,7 @@ void Opt_trace_struct::do_destruct()
>    {
>      stmt->pop();
>      stmt->next_line();
> -    stmt->buffer.append(brackets[requires_key + 2]);
> +    stmt->buffer.append(close_bracket(requires_key));
>      if (stmt->ctx->end_marker && saved_key != NULL)
>      {
>        stmt->buffer.append(STRING_WITH_LEN(" /* "));
> 
> === modified file 'sql/opt_trace.h'
> --- a/sql/opt_trace.h	2011-01-11 08:15:46 +0000
> +++ b/sql/opt_trace.h	2011-01-12 08:32:58 +0000
> @@ -1148,7 +1148,7 @@ C++ has member functions, not methods.
>    */
>    // Hand-coded typechecking is generally a bad idea,
>    // but since you need the information in the base CTOR/DTOR: OK.
> -  int8 requires_key;
> +  bool requires_key;
>    Opt_trace_stmt *stmt;  ///< Trace owning the structure
>    /** Parent structure ("outer structure" of the structure) */
>    Opt_trace_struct *parent;
> @@ -1170,10 +1170,6 @@ C++ has member functions, not methods.
>    */
>    bool save_stmt_support_I_S;
>  
> -  /** opening and closing symbols for arrays ([])and objects ({}) */
> -  static const char brackets[];
> -  /** human-readable names of structure types */
> -  static const char* types[];
>  public:
>    /**
>       Whether a JSON syntax error should cause an assertion in debug binaries;
> 
> 
> 
> ------------------------------------------------------------------------
> 
> 


-- 
Mr. Guilhem Bichot <guilhem.bichot@stripped>
Oracle / MySQL / Optimizer team, Lead Software Engineer
Bordeaux, France
www.oracle.com / www.mysql.com

Thread
bzr commit into mysql-next-mr-bugfixing branch (tor.didriksen:3245) WL#5257Tor Didriksen12 Jan
  • Re: bzr commit into mysql-next-mr-bugfixing branch (tor.didriksen:3245)WL#5257Guilhem Bichot25 Jan
    • Re: bzr commit into mysql-next-mr-bugfixing branch (tor.didriksen:3245)WL#5257Tor Didriksen25 Jan
      • Re: bzr commit into mysql-next-mr-bugfixing branch (tor.didriksen:3245)WL#5257Guilhem Bichot26 Jan