List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:January 26 2011 10:56am
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 25.01.2011 17:29:
> 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];
> }

Ok, did this (actually, 2+v).

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

switch/case: 1.3 seconds.
array: 1.2 secs.
ternary operator: 1.6 secs.
> 
> If (2*bool) makes you unhappy, you can even have two different arrays.

Kept one single array.
This will be visible in a soon coming commit.
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