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
