From: Tor Didriksen
Date: January 25 2011 4:29pm
Subject: Re: bzr commit into mysql-next-mr-bugfixing branch (tor.didriksen:3245)
 WL#5257
List-Archive: http://lists.mysql.com/commits/129594
Message-Id: <4D3EFA60.50308@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit

On 2011-01-25 10:48, Guilhem Bichot wrote:
> 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
>> +

my main argument is readability.
you can of course say something like

inline const char *close_bracket(bool v)
{
   return brackets[2*v];
}

If benchmarking shows that this is superior.
BTW: Can you try switch/case on the bool as well?

If (2*bool) makes you unhappy, you can even have two different arrays.

-- didrik
>> +
>>  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;
>>
>>
>>
>> ------------------------------------------------------------------------
>>
>>
>
>