List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:April 13 2011 3:53pm
Subject:[Resend] bzr commit into mysql-trunk branch (guilhem.bichot:3290)
View as plain text  
[This commit e-mail is a repeat.]

#At file:///home/mysql_src/bzrrepos_new/mysql-next-mr-opt-backporting-wl4800-new2/ based
on revid:jorgen.loland@stripped

 3290 Guilhem Bichot	2011-04-12
      Proposal suggested by Jorgen: move content of Struct_desc into Opt_trace_struct.
      Opt_trace_stmt functions get the information which they need
      through new parameters to open_struct()/close_struct().
      There is only one "encapsulation breakage" (one new public setter), it's when
      Opt_trace_stmt::separator() has to inform Opt_trace_struct that it's not empty
      anymore. But this is minor and I could not find a way to clean this up.
      Partial_access_for_Opt_trace_stmt is deleted.

    modified:
      sql/opt_trace.cc
      sql/opt_trace.h
=== modified file 'sql/opt_trace.cc'
--- a/sql/opt_trace.cc	2011-04-05 07:34:13 +0000
+++ b/sql/opt_trace.cc	2011-04-12 13:40:12 +0000
@@ -75,15 +75,18 @@ public:
      may have been written to, will likely be invalid JSON.
   */
   bool open_struct(const char *key, Opt_trace_struct *ots,
-                   Opt_trace_context::feature_value feature,
-                   char opening_bracket);
+                   bool has_disabled_I_S, char opening_bracket);
   /**
-     When closing an Opt_trace_struct: adds the closing bracket and optionally
-     the key to the trace buffer, updates current_struct.
+     When closing an Opt_trace_struct:
+     - adds the closing bracket and optionally the key to the trace buffer
+     - re-enables I_S output if the dying structure had disabled it
+     - updates current_struct.
      @param  saved_key        key or NULL
+     @param  has_disabled_I_S whether structure had disabled I_S output
      @param  closing_bracket  closing bracket to use
   */
-  void close_struct(const char *saved_key, char closing_bracket);
+  void close_struct(const char *saved_key, bool has_disabled_I_S,
+                    char closing_bracket);
 
   /// Put optional comma, newline and indentation
   void separator();
@@ -167,28 +170,8 @@ private:
   Opt_trace_context *ctx;                       ///< context
   Opt_trace_struct *current_struct;             ///< current open structure
 
-  /**
-     Whether we added data (scalars, structures) to the current struct.
-     This is used and maintained only if sending to I_S.
-  */
-  bool current_struct_empty;
-  /// @sa stack_of_current_structs
-  struct Struct_desc
-  {
-    Opt_trace_struct *current_struct;
-    bool has_disabled_I_S;
-    bool current_struct_empty;
-  };
-  /**
-     Same logic as Opt_trace_context::stack_of_current_stmts. We need to
-     remember:
-     - each value of current_struct
-     - whether such structure caused tracing to be disabled in this statement
-     (happens when the structure belongs to a not-traced optimizer feature,
-     in accordance with the value of @@@@optimizer_trace_features)
-     - whether such structure was empty.
-  */
-  Dynamic_array<Struct_desc> stack_of_current_structs;
+  /// Same logic as Opt_trace_context::stack_of_current_stmts.
+  Dynamic_array<Opt_trace_struct *> stack_of_current_structs;
 
   /**
      When we temporarily disable I_S (because of Opt_trace_disable_I_S, or
@@ -287,29 +270,6 @@ private:
 };
 
 
-/**
-   This is a trick.
-   The problem is that Opt_trace_stmt needs access to
-   Opt_trace_struct::check_key(). We don't want to make Opt_trace_stmt a
-   friend of Opt_trace_struct, that would be giving Opt_trace_stmt access to
-   too many things. So we have to make check_key() public. This isn't good
-   either: function becomes accessible to includers of opt_trace.h.
-   Instead, we declare the class below as friend of Opt_trace_struct; this
-   class only calls check_key(), and Opt_trace_stmt is a friend of it. The
-   idea is that, of Opt_trace_struct, only check_key() is made accessible
-   to Opt_trace_stmt, and only to it, via the class below.
-   Let me know what option to choose, trick or public.
-*/
-class Partial_access_for_Opt_trace_stmt
-{
-private:
-  static const char *check_key(Opt_trace_struct *s, const char *key)
-  { return s->check_key(key); }
-
-  friend class Opt_trace_stmt;
-};
-
-
 // implementation of class Opt_trace_struct
 
 namespace {
@@ -339,7 +299,9 @@ void Opt_trace_struct::do_construct(Opt_
 #ifndef DBUG_OFF
   previous_key[0]= 0;
 #endif
-  if (likely(!stmt->open_struct(key, this, feature,
+  has_disabled_I_S= !ctx->feature_enabled(feature);
+  empty= true;
+  if (likely(!stmt->open_struct(key, this, has_disabled_I_S,
                                 opening_bracket(requires_key))))
     started= true;
 }
@@ -349,7 +311,8 @@ void Opt_trace_struct::do_destruct()
 {
   DBUG_PRINT("opt", ("%s: ending struct", saved_key));
   DBUG_ASSERT(started);
-  stmt->close_struct(saved_key, closing_bracket(requires_key));
+  stmt->close_struct(saved_key, has_disabled_I_S,
+                     closing_bracket(requires_key));
   started= false;
 }
 
@@ -561,13 +524,12 @@ void Opt_trace_stmt::set_query(const cha
 
 
 bool Opt_trace_stmt::open_struct(const char *key, Opt_trace_struct *ots,
-                                 Opt_trace_context::feature_value feature,
+                                 bool has_disabled_I_S,
                                  char opening_bracket)
 {
-  bool has_disabled_I_S= false;
   if (support_I_S == YES_FOR_THIS)
   {
-    if (!ctx->feature_enabled(feature))
+    if (has_disabled_I_S)
     {
 
       /*
@@ -589,13 +551,11 @@ bool Opt_trace_stmt::open_struct(const c
       if (unlikely(stack_of_values_of_support_I_S.append(support_I_S)))
         goto err;
       support_I_S= NO_FOR_THIS_AND_CHILDREN;
-      has_disabled_I_S= true;
     }
     else
     {
       if (current_struct != NULL)
-        key= Partial_access_for_Opt_trace_stmt::
-          check_key(current_struct, key);
+        key= current_struct->check_key(key);
       trace_buffer.prealloc();
       separator();
       if (key != NULL)
@@ -608,10 +568,9 @@ bool Opt_trace_stmt::open_struct(const c
     }
   }
   {
-    Struct_desc d= { current_struct, has_disabled_I_S, current_struct_empty};
     DBUG_EXECUTE_IF("opt_trace_oom_in_open_struct",
                     DBUG_SET("+d,simulate_out_of_memory"););
-    const bool rc= stack_of_current_structs.append(d);
+    const bool rc= stack_of_current_structs.append(current_struct);
     /*
       If the append() above didn't trigger reallocation, we need to turn the
       symbol off by ourselves, or it could make an unrelated allocation
@@ -623,7 +582,6 @@ bool Opt_trace_stmt::open_struct(const c
       goto err;
   }
   current_struct= ots;
-  current_struct_empty= true;
   return false;
 err:
   return true;
@@ -631,12 +589,10 @@ err:
 
 
 void Opt_trace_stmt::close_struct(const char *saved_key,
+                                  bool has_disabled_I_S,
                                   char closing_bracket)
 {
-  Struct_desc d= stack_of_current_structs.pop();
-  current_struct= d.current_struct;
-  current_struct_empty= d.current_struct_empty;
-
+  current_struct= stack_of_current_structs.pop();
   if (support_I_S == YES_FOR_THIS)
   {
     next_line();
@@ -648,7 +604,7 @@ void Opt_trace_stmt::close_struct(const 
       trace_buffer.append(STRING_WITH_LEN(" */"));
     }
   }
-  if (d.has_disabled_I_S)
+  if (has_disabled_I_S)
     support_I_S= stack_of_values_of_support_I_S.pop();
 }
 
@@ -659,10 +615,9 @@ void Opt_trace_stmt::separator()
   // Put a comma first, if we have already written an object at this level.
   if (current_struct != NULL)
   {
-    if (!current_struct_empty)
+    if (!current_struct->set_not_empty())
       trace_buffer.append(',');
     next_line();
-    current_struct_empty= false;
   }
 }
 

=== modified file 'sql/opt_trace.h'
--- a/sql/opt_trace.h	2011-04-05 07:34:13 +0000
+++ b/sql/opt_trace.h	2011-04-12 13:40:12 +0000
@@ -716,8 +716,6 @@ private:
 };
 
 
-class Partial_access_for_Opt_trace_stmt;
-
 /**
    Object and array are both "structured data" and have lots in common, so the
    Opt_trace_struct is a base class for them.
@@ -982,6 +980,25 @@ public:
   */
   void end() { if (unlikely(started)) do_destruct(); }
 
+  /**
+    Informs this structure that we are adding data (scalars, structures) to
+    it.
+    This is used only if sending to I_S.
+    @returns whether the structure was empty so far.
+    @note this is reserved for use by Opt_trace_stmt.
+  */
+  bool set_not_empty()
+  {
+    const bool old_empty= empty;
+    empty= false;
+    return old_empty;
+  }
+  /**
+    Validates the key about to be added.
+    @note this is reserved for use by Opt_trace_stmt.
+  */
+  const char *check_key(const char *key);
+
 private:
 
   /// Full initialization. @sa Opt_trace_struct::Opt_trace_struct
@@ -1006,9 +1023,6 @@ private:
   Opt_trace_struct& do_add_null(const char *key);
   Opt_trace_struct& do_add_utf8_table(TABLE *tab);
 
-  const char *check_key(const char *key);  ///< Validates the key
-  friend class Partial_access_for_Opt_trace_stmt;
-
   Opt_trace_struct(const Opt_trace_struct&);            ///< not defined
   Opt_trace_struct& operator=(const Opt_trace_struct&); ///< not defined
 
@@ -1038,6 +1052,14 @@ private:
   */
   char previous_key[25];
 #endif
+
+  /**
+    Whether this structure caused tracing to be disabled in this statement
+    because belonging to a not-traced optimizer feature, in accordance with
+    the value of @@@@optimizer_trace_features.
+  */
+  bool has_disabled_I_S;
+  bool empty;                                   ///< @see is_empty()
 };
 
 


Attachment: [text/bzr-bundle] bzr/guilhem.bichot@oracle.com-20110412134012-4yv0l797yoz8qynu.bundle
Thread
[Resend] bzr commit into mysql-trunk branch (guilhem.bichot:3290) Guilhem Bichot13 Apr
  • Re: [Resend] bzr commit into mysql-trunk branch (guilhem.bichot:3290)Jorgen Loland14 Apr
    • Re: [Resend] bzr commit into mysql-trunk branch (guilhem.bichot:3290)Guilhem Bichot14 Apr