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.
