Implementation of Monty's idea about clear_alloc_root() optimization and cleanup of work

with memory roots in THD/Statement/Item_arena.
Added assertions preventing memory allocation on bzero'ed MEM_ROOT since it is worked by 
pure luck and was very ineffective.


include/my_sys.h:
  Reimplementation of Monty's optimization of clear_alloc_root().
  Now clear_alloc_root() can be used only for detaching memory associated with MEM_ROOT
  (e.g. to avoid its freeing). It can not be used for MEM_ROOT initialization any longer 
  (it was bad idea anyway since memory allocation on such MEM_ROOT was very ineffective 
  and worked by pure luck).
  Introduced ALLOC_ROOT_MIN_BLOCK_SIZE constant.
mysys/my_alloc.c:
  Added description of init_alloc_root().
  Added assertions to alloc_root() and reset_root_defaults() so now they can only be used
  on previosly initialized MEM_ROOT. (It worked for bzeroed MEM_ROOT before but 
  by pure luck and very inefficiently). Calling free_root() on bzero'ed MEM_ROOT
  is still ok (we can't remove this easily because of static MEM_ROOTs).
  Also now using ALLOC_ROOT_MIN_BLOCK_SIZE constant inside these functions.
sql/opt_range.cc:
  Fixed get_quick_select_for_ref() function to not use bzero'ed MEM_ROOT for allocation.
  Also QUICK_RANGEs created in this function should be created in memory root of QUICK_SELECT.
sql/sql_class.cc:
  Implementation of Monty's idea about clear_alloc_root() optimization and cleanup of work 
  with memory roots in THD/Statement/Item_arena.
  Now we are always initing THD::transaction.mem_root and THD::mem_root in THD constructor 
  (without memory allocation and with minimal block size) and then later change their
  parameters in THD::init_for_queries() (this is partially because we can't allocate anything
  on bzero'ed memory roots anymore).
  Item_arena() constructor is now trivial and is used only then Item_arena is created as backup
  storage for other Item_arena (we use Item_arena(bool) now if it is part of Statement).
  Both trivial Item_arena constructor and destructor are now inline.
  Removed unneeded clear_alloc_root from Item_arena::restore_backup_item_arena().
sql/sql_class.h:
  Both trivial Item_arena constructor and destructor are now inline.
  Commented various Item_arena constructors.

5 files changed, 100 insertions, 33 deletions

diff --git a/include/my_sys.h b/include/my_sys.h
index ad1966ba67f..271e0ea0bcb 100644
--- a/include/my_sys.h
+++ b/include/my_sys.h
@@ -725,7 +725,8 @@ extern void my_free_lock(byte *ptr,myf flags);
 #define my_free_lock(A,B) my_free((A),(B))
 #endif
 #define alloc_root_inited(A) ((A)->min_malloc != 0)
-#define clear_alloc_root(A) bzero((void *) (A), sizeof(MEM_ROOT))
+#define ALLOC_ROOT_MIN_BLOCK_SIZE (MALLOC_OVERHEAD + sizeof(USED_MEM) + 8)
+#define clear_alloc_root(A) do { (A)->free= (A)->used= (A)->pre_alloc= 0; } while(0)
 extern void init_alloc_root(MEM_ROOT *mem_root, uint block_size,
 			    uint pre_alloc_size);
 extern gptr alloc_root(MEM_ROOT *mem_root,unsigned int Size);
diff --git a/mysys/my_alloc.c b/mysys/my_alloc.c
index c9784ddc9a0..f0bc62b10a0 100644
--- a/mysys/my_alloc.c
+++ b/mysys/my_alloc.c
@@ -22,6 +22,27 @@
 #undef EXTRA_DEBUG
 #define EXTRA_DEBUG
 
+
+/*
+  Initialize memory root
+
+  SYNOPSIS
+    init_alloc_root()
+      mem_root       - memory root to initialize
+      block_size     - size of chunks (blocks) used for memory allocation
+                       (It is external size of chunk i.e. it should include
+                        memory required for internal structures, thus it
+                        should be no less than ALLOC_ROOT_MIN_BLOCK_SIZE)
+      pre_alloc_size - if non-0, then size of block that should be
+                       pre-allocated during memory root initialization.
+
+  DESCRIPTION
+    This function prepares memory root for further use, sets initial size of
+    chunk for memory allocation and pre-allocates first block if specified.
+    Altough error can happen during execution of this function if pre_alloc_size
+    is non-0 it won't be reported. Instead it will be reported as error in first
+    alloc_root() on this memory root.
+*/
 void init_alloc_root(MEM_ROOT *mem_root, uint block_size,
 		     uint pre_alloc_size __attribute__((unused)))
 {
@@ -29,7 +50,7 @@ void init_alloc_root(MEM_ROOT *mem_root, uint block_size,
   DBUG_PRINT("enter",("root: 0x%lx", mem_root));
   mem_root->free= mem_root->used= mem_root->pre_alloc= 0;
   mem_root->min_malloc= 32;
-  mem_root->block_size= block_size-MALLOC_OVERHEAD-sizeof(USED_MEM)-8;
+  mem_root->block_size= block_size - ALLOC_ROOT_MIN_BLOCK_SIZE;
   mem_root->error_handler= 0;
   mem_root->block_num= 4;			/* We shift this with >>2 */
   mem_root->first_block_usage= 0;
@@ -54,9 +75,9 @@ void init_alloc_root(MEM_ROOT *mem_root, uint block_size,
   SYNOPSIS
     reset_root_defaults()
     mem_root        memory root to change defaults of
-    block_size      new value of block size. Must be 
-                    greater than ~68 bytes (the exact value depends on
-                    platform and compilation flags)
+    block_size      new value of block size. Must be greater or equal
+                    than ALLOC_ROOT_MIN_BLOCK_SIZE (this value is about
+                    68 bytes and depends on platform and compilation flags)
     pre_alloc_size  new size of preallocated block. If not zero,
                     must be equal to or greater than block size,
                     otherwise means 'no prealloc'.
@@ -70,7 +91,9 @@ void init_alloc_root(MEM_ROOT *mem_root, uint block_size,
 void reset_root_defaults(MEM_ROOT *mem_root, uint block_size,
                          uint pre_alloc_size __attribute__((unused)))
 {
-  mem_root->block_size= block_size-MALLOC_OVERHEAD-sizeof(USED_MEM)-8;
+  DBUG_ASSERT(alloc_root_inited(mem_root));
+
+  mem_root->block_size= block_size - ALLOC_ROOT_MIN_BLOCK_SIZE;
 #if !(defined(HAVE_purify) && defined(EXTRA_DEBUG))
   if (pre_alloc_size)
   {
@@ -123,6 +146,8 @@ gptr alloc_root(MEM_ROOT *mem_root,unsigned int Size)
   DBUG_ENTER("alloc_root");
   DBUG_PRINT("enter",("root: 0x%lx", mem_root));
 
+  DBUG_ASSERT(alloc_root_inited(mem_root));
+
   Size+=ALIGN_SIZE(sizeof(USED_MEM));
   if (!(next = (USED_MEM*) my_malloc(Size,MYF(MY_WME))))
   {
@@ -140,6 +165,8 @@ gptr alloc_root(MEM_ROOT *mem_root,unsigned int Size)
   reg1 USED_MEM *next= 0;
   reg2 USED_MEM **prev;
 
+  DBUG_ASSERT(alloc_root_inited(mem_root));
+
   Size= ALIGN_SIZE(Size);
   if ((*(prev= &mem_root->free)) != NULL)
   {
diff --git a/sql/opt_range.cc b/sql/opt_range.cc
index 27e8e9c11e7..9c5b0235767 100644
--- a/sql/opt_range.cc
+++ b/sql/opt_range.cc
@@ -2554,7 +2554,8 @@ static bool null_part_in_key(KEY_PART *key_part, const char *key, uint length)
 
 QUICK_SELECT *get_quick_select_for_ref(THD *thd, TABLE *table, TABLE_REF *ref)
 {
-  QUICK_SELECT *quick=new QUICK_SELECT(thd, table, ref->key, 1);
+  MEM_ROOT *old_root= my_pthread_getspecific_ptr(MEM_ROOT*, THR_MALLOC);
+  QUICK_SELECT *quick= new QUICK_SELECT(thd, table, ref->key);
   KEY *key_info = &table->key_info[ref->key];
   KEY_PART *key_part;
   QUICK_RANGE *range;
@@ -2566,7 +2567,7 @@ QUICK_SELECT *get_quick_select_for_ref(THD *thd, TABLE *table, TABLE_REF *ref)
   {
     if (thd->is_fatal_error)
       goto err;					// out of memory
-    return quick;				// empty range
+    goto ok;                                    // empty range
   }
 
   if (!(range= new QUICK_RANGE()))
@@ -2613,9 +2614,12 @@ QUICK_SELECT *get_quick_select_for_ref(THD *thd, TABLE *table, TABLE_REF *ref)
       goto err;
   }
 
+ok:
+  my_pthread_setspecific_ptr(THR_MALLOC, old_root);
   return quick;
 
 err:
+  my_pthread_setspecific_ptr(THR_MALLOC, old_root);
   delete quick;
   return 0;
 }
diff --git a/sql/sql_class.cc b/sql/sql_class.cc
index 16c0c206df3..2074f0f5d40 100644
--- a/sql/sql_class.cc
+++ b/sql/sql_class.cc
@@ -221,7 +221,6 @@ THD::THD()
 
   init();
   /* Initialize sub structures */
-  clear_alloc_root(&transaction.mem_root);
   init_alloc_root(&warn_root, WARN_ALLOC_BLOCK_SIZE, WARN_ALLOC_PREALLOC_SIZE);
   user_connect=(USER_CONN *)0;
   hash_init(&user_vars, &my_charset_bin, USER_VARS_HASH_SIZE, 0, 0,
@@ -258,6 +257,7 @@ THD::THD()
     transaction.trans_log.end_of_file= max_binlog_cache_size;
   }
 #endif
+  init_alloc_root(&transaction.mem_root, ALLOC_ROOT_MIN_BLOCK_SIZE, 0);
   {
     ulong tmp=sql_rnd_with_mutex();
     randominit(&rand, tmp + (ulong) &rand, tmp + (ulong) ::query_id);
@@ -303,12 +303,12 @@ void THD::init(void)
 void THD::init_for_queries()
 {
   ha_enable_transaction(this,TRUE);
-  init_sql_alloc(&mem_root,
-                 variables.query_alloc_block_size,
-                 variables.query_prealloc_size);
-  init_sql_alloc(&transaction.mem_root,         
-		 variables.trans_alloc_block_size, 
-		 variables.trans_prealloc_size);
+
+  reset_root_defaults(&mem_root, variables.query_alloc_block_size,
+                      variables.query_prealloc_size);
+  reset_root_defaults(&transaction.mem_root,
+                      variables.trans_alloc_block_size,
+                      variables.trans_prealloc_size);
 }
 
 
@@ -1331,6 +1331,17 @@ void select_dumpvar::cleanup()
 }
 
 
+/*
+  Create arena for already constructed THD.
+
+  SYNOPSYS
+    Item_arena()
+      thd - thread for which arena is created
+
+  DESCRIPTION
+    Create arena for already existing THD using its variables as parameters
+    for memory root initialization.
+*/
 Item_arena::Item_arena(THD* thd)
   :free_list(0),
   state(INITIALIZED)
@@ -1341,24 +1352,31 @@ Item_arena::Item_arena(THD* thd)
 }
 
 
-/* This constructor is called when Item_arena is a subobject of THD */
+/*
+  Create arena and optionally initialize memory root.
 
-Item_arena::Item_arena()
-  :free_list(0),
-  state(CONVENTIONAL_EXECUTION)
-{
-  clear_alloc_root(&mem_root);
-}
+  SYNOPSYS
+    Item_arena()
+      init_mem_root - whenever we need to initialize memory root
 
+  DESCRIPTION
+    Create arena and optionally initialize memory root with minimal
+    possible parameters.
 
+  NOTE
+    We use this constructor when arena is part of THD, but reinitialize
+    its memory root in THD::init_for_queries() before execution of real
+    statements.
+*/
 Item_arena::Item_arena(bool init_mem_root)
   :free_list(0),
-  state(INITIALIZED)
+  state(CONVENTIONAL_EXECUTION)
 {
   if (init_mem_root)
-    clear_alloc_root(&mem_root);
+    init_alloc_root(&mem_root, ALLOC_ROOT_MIN_BLOCK_SIZE, 0);
 }
 
+
 Item_arena::Type Item_arena::type() const
 {
   DBUG_ASSERT("Item_arena::type()" == "abstract");
@@ -1366,10 +1384,6 @@ Item_arena::Type Item_arena::type() const
 }
 
 
-Item_arena::~Item_arena()
-{}
-
-
 /*
   Statement functions 
 */
@@ -1393,7 +1407,8 @@ Statement::Statement(THD *thd)
 */
 
 Statement::Statement()
-  :id(0),
+  :Item_arena((bool)TRUE),
+  id(0),
   set_query_id(1),
   allow_sum_func(0),                            /* initialized later */
   lex(&main_lex),
@@ -1461,8 +1476,16 @@ void Item_arena::restore_backup_item_arena(Item_arena *set, Item_arena *backup)
 {
   set->set_item_arena(this);
   set_item_arena(backup);
-  // reset backup mem_root to avoid its freeing
-  init_alloc_root(&backup->mem_root, 0, 0);
+#ifdef NOT_NEEDED_NOW
+  /*
+    Reset backup mem_root to avoid its freeing.
+    Since Item_arena's mem_root is freed only when it is part of Statement
+    we need this only if we use some Statement's arena as backup storage.
+    But we do this only with THD::stmt_backup and this Statement is specially
+    handled in this respect. So this code is not really needed now.
+  */
+  clear_alloc_root(&backup->mem_root);
+#endif
 }
 
 void Item_arena::set_item_arena(Item_arena *set)
diff --git a/sql/sql_class.h b/sql/sql_class.h
index 387bba43cad..89bf2dde12e 100644
--- a/sql/sql_class.h
+++ b/sql/sql_class.h
@@ -441,11 +441,23 @@ public:
     STATEMENT, PREPARED_STATEMENT, STORED_PROCEDURE
   };
 
+  /*
+    This constructor is used only when Item_arena is created as
+    backup storage for another instance of Item_arena.
+  */
+  Item_arena() {};
+  /*
+    Create arena for already constructed THD using its variables as
+    parameters for memory root initialization.
+  */
   Item_arena(THD *thd);
-  Item_arena();
+  /*
+    Create arena and optionally init memory root with minimal values.
+    Particularly used if Item_arena is part of Statement.
+  */
   Item_arena(bool init_mem_root);
   virtual Type type() const;
-  virtual ~Item_arena();
+  virtual ~Item_arena() {};
 
   inline bool is_stmt_prepare() const { return (int)state < (int)PREPARED; }
   inline bool is_first_stmt_execute() const { return state == PREPARED; }