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