Hi Alexander, ok to push.
Thanks, Sergey On Wed, May 13, 2015 at 02:22:22PM +0400, Alexander Barkov wrote: > Hi Sergey, > > I'm sending a new version, which takes into account your review > suggestions and our skype discussion. > > Thanks. > > On 05/08/2015 12:27 PM, Sergey Vojtovich wrote: > >Hi Alexander, > > > >following profiler reports for build_equal_items* in your and mine patches. > >For OLTP RO they're equal. I guess Item_func::build_equal_items() was not > >inlined due to recursion? > > > >Bar's patch > >----------- > >0.06% Item_func_eq::build_equal_items_and_cond() > >0.05% build_equal_items() > >0.01% Item_func::build_equal_items() > >0.00% Item_func::build_equal_items_and_cond() > > > >Svoj's patch > >------------ > >0.06% build_equal_items() > >0.05% Item_func_eq::build_equal_items_and_cond() > >0.01% Item_func::build_equal_items() > >0.00% Item::build_equal_items_and_cond() > > > >Regards, > >Sergey > > > >On Thu, May 07, 2015 at 01:58:28PM +0400, Sergey Vojtovich wrote: > >>Hi Alexander, > >> > >>comments inline. > >> > >>On Wed, May 06, 2015 at 07:51:18PM +0400, Alexander Barkov wrote: > >>>Hi Sergey, > >>> > >>>Please review a patch for the next step for MDEV-7950 > >>> > >>>(one small thing at a time, to avoid huge unclear patches) > >>> > >>>Thanks. > >> > >>>diff --git a/sql/item.h b/sql/item.h > >>>index 043605a..a665d23 100644 > >>>--- a/sql/item.h > >>>+++ b/sql/item.h > >>>@@ -1133,6 +1133,13 @@ class Item: public Type_std_attributes > >>> update_used_tables(); > >>> return this; > >>> } > >>>+ virtual COND *build_equal_items_and_cond(THD *thd, COND_EQUAL > >>>*inherited, > >>>+ bool link_item_fields, > >>>+ COND_EQUAL **cond_equal_ref) > >>>+ { > >>>+ *cond_equal_ref= NULL; > >>>+ return Item::build_equal_items(thd, inherited, link_item_fields); > >>>+ } > >>> /* > >>> Checks whether the item is: > >>> - a simple equality (field=field_item or field=constant_item), or > >>cond_equal_ref is initialized to 0 by caller, why duplicate initialization > >>here > >>and everywhere else? > >> > >>When you write XXX::build_equal_items() you force static call indeed, but > >>not > >>inlining, which we're aiming for. That is non-inlined static call as such > >>won't > >>save us much compared to virtual call. > >> > >>I think our aim should be to keep build_equal_items_and_cond() virtual and > >>have > >>build_equal_items() inlined into the former. So we mostly rely on compiler's > >>optimizer. > >> > >>I _believe_ it should be clever enough to optimize the following as > >>described > >>above: > >>class Item > >>{ > >> ... > >> virtual build_equal_items_and_cond() > >> { return build_equal_items(); } > >> ... > >>} > >> > >>This should be easy to test by adding "inline" to build_equal_items() > >>declaration and compiling with -Winline. > >> > >>Makes sense? > >> > >>...skip... > >> > >>>index 53f249d..0c279ed 100644 > >>>--- a/sql/item_cmpfunc.h > >>>+++ b/sql/item_cmpfunc.h > >>>@@ -1989,6 +2002,11 @@ class COND_EQUAL: public Sql_alloc > >>> { > >>> upper_levels= 0; > >>> } > >>>+ COND_EQUAL(Item_equal *item_equal) > >>>+ :upper_levels(0) > >>>+ { > >>>+ current_level.push_back(item_equal); > >>>+ } > >>> void copy(COND_EQUAL &cond_equal) > >>> { > >>> max_members= cond_equal.max_members; > >>If possible, here and everywhere else please use > >>push_back(item_equal, thd->mem_root). This will save us one > >>pthread_getspecific() call. > >> > >>>@@ -1998,6 +2016,8 @@ class COND_EQUAL: public Sql_alloc > >>> else > >>> current_level= cond_equal.current_level; > >>> } > >>>+ Item_equal *build_item_equal_from_current_level(THD *thd, > >>>+ COND_EQUAL *inherited); > >>> }; > >>> > >>> > >>>@@ -2106,8 +2126,23 @@ class Item_cond_and :public Item_cond > >>> void mark_as_condition_AND_part(TABLE_LIST *embedding); > >>> virtual uint exists2in_reserved_items() { return list.elements; }; > >>> bool walk_top_and(Item_processor processor, uchar *arg); > >>>+ void build_new_level(THD *thd, COND_EQUAL *cond_equal); > >>> COND *build_equal_items(THD *thd, COND_EQUAL *inherited, > >>> bool link_item_fields); > >>Didn't get the need for build_item_equal_from_current_level() and > >>build_new_level(). Is it just some minor refactoring? > >> > >>...skip... > >>>diff --git a/sql/item_func.h b/sql/item_func.h > >>>index 0d57c2b..01e1527 100644 > >>>--- a/sql/item_func.h > >>>+++ b/sql/item_func.h > >>>@@ -133,6 +133,15 @@ class Item_func :public Item_func_or_sum, public > >>>Used_tables_and_const_cache > >>> } > >>> COND *build_equal_items(THD *thd, COND_EQUAL *inherited, > >>> bool link_item_fields); > >>>+ COND *build_equal_items_and_cond(THD *thd, COND_EQUAL *inherited, > >>>+ bool link_item_fields, > >>>+ COND_EQUAL **cond_equal_ref) > >>>+ { > >>>+ *cond_equal_ref= NULL; > >>>+ COND *cond= build_equal_items(thd, inherited, link_item_fields); > >>>+ DBUG_ASSERT(cond == this); > >>>+ return cond; > >>>+ } > >>> bool eq(const Item *item, bool binary_cmp) const; > >>> virtual optimize_type select_optimize() const { return OPTIMIZE_NONE; } > >>> virtual bool have_rev_func() const { return 0; } > >>The only difference from base build_equal_items_and_cond() is just > >>DBUG_ASSERT(). > >>I think assert can be moved into base implementation with more complex > >>condition > >>too (assuming we'll decide to keep only base implementation). > >> > >>...skip... > >> > >>>diff --git a/sql/sql_select.cc b/sql/sql_select.cc > >>>index 95778f0..7658a24 100644 > >>>--- a/sql/sql_select.cc > >>>+++ b/sql/sql_select.cc > >>>@@ -13053,23 +13127,14 @@ static COND *build_equal_items(JOIN *join, COND > >>>*cond, > >>> > >>> if (cond) > >>> { > >>>- cond= cond->build_equal_items(thd, inherited, link_equal_fields); > >>>- if (cond->type() == Item::COND_ITEM && > >>>- ((Item_cond*) cond)->functype() == Item_func::COND_AND_FUNC) > >>>- cond_equal= &((Item_cond_and*) cond)->m_cond_equal; > >>>- > >>>- else if (cond->type() == Item::FUNC_ITEM && > >>>- ((Item_cond*) cond)->functype() == > >>>Item_func::MULT_EQUAL_FUNC) > >>>+ cond= cond->build_equal_items_and_cond(thd, inherited, > >>>link_equal_fields, > >>>+ &cond_equal); > >>>+ if (cond_equal) > >>> { > >>>- cond_equal= new COND_EQUAL; > >>>- cond_equal->current_level.push_back((Item_equal *) cond); > >>>+ cond_equal->upper_levels= inherited; > >>>+ inherited= cond_equal; > >>> } > >>> } > >>>- if (cond_equal) > >>>- { > >>>- cond_equal->upper_levels= inherited; > >>>- inherited= cond_equal; > >>>- } > >>> *cond_equal_ref= cond_equal; > >>> > >>> if (join_list && !ignore_on_conds) > >>I believe this *cond_equal_ref= cond_equal is probably not needed. > >>Initialize > >>cond_equal_ref directly and remove cond_equal variable. > >> > >>Regards, > >>Sergey > diff --git a/sql/item.h b/sql/item.h > index 043605a..017d348 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -1128,9 +1128,11 @@ class Item: public Type_std_attributes > void print_value(String *); > virtual void update_used_tables() {} > virtual COND *build_equal_items(THD *thd, COND_EQUAL *inherited, > - bool link_item_fields) > + bool link_item_fields, > + COND_EQUAL **cond_equal_ref) > { > update_used_tables(); > + DBUG_ASSERT(!cond_equal_ref || !cond_equal_ref[0]); > return this; > } > /* > @@ -2335,7 +2337,8 @@ class Item_field :public Item_ident > update_table_bitmaps(); > } > COND *build_equal_items(THD *thd, COND_EQUAL *inherited, > - bool link_item_fields) > + bool link_item_fields, > + COND_EQUAL **cond_equal_ref) > { > /* > normilize_cond() replaced all conditions of type > @@ -2351,7 +2354,8 @@ class Item_field :public Item_ident > SELECT * FROM t1 WHERE DEFAULT(a); > */ > DBUG_ASSERT(type() != FIELD_ITEM); > - return Item_ident::build_equal_items(thd, inherited, link_item_fields); > + return Item_ident::build_equal_items(thd, inherited, link_item_fields, > + cond_equal_ref); > } > bool is_result_field() { return false; } > void set_result_field(Field *field) {} > @@ -3593,7 +3597,8 @@ class Item_ref :public Item_ident > table_map used_tables() const; > void update_used_tables(); > COND *build_equal_items(THD *thd, COND_EQUAL *inherited, > - bool link_item_fields) > + bool link_item_fields, > + COND_EQUAL **cond_equal_ref) > { > /* > normilize_cond() replaced all conditions of type > @@ -3604,7 +3609,8 @@ class Item_ref :public Item_ident > already be replaced. No Item_ref referencing to Item_field are > possible. > */ > DBUG_ASSERT(real_type() != FIELD_ITEM); > - return Item_ident::build_equal_items(thd, inherited, link_item_fields); > + return Item_ident::build_equal_items(thd, inherited, link_item_fields, > + cond_equal_ref); > } > bool const_item() const > { > diff --git a/sql/item_cmpfunc.h b/sql/item_cmpfunc.h > index 53f249d..0a3bf41 100644 > --- a/sql/item_cmpfunc.h > +++ b/sql/item_cmpfunc.h > @@ -563,7 +563,8 @@ class Item_func_eq :public Item_bool_rowready_func2 > const char *func_name() const { return "="; } > Item *negated_item(); > COND *build_equal_items(THD *thd, COND_EQUAL *inherited, > - bool link_item_fields); > + bool link_item_fields, > + COND_EQUAL **cond_equal_ref); > bool check_equality(THD *thd, COND_EQUAL *cond, List<Item> *eq_list); > /* > - If this equality is created from the subquery's IN-equality: > @@ -1772,7 +1773,8 @@ class Item_cond :public Item_bool_func > used_tables_and_const_cache_update_and_join(list); > } > COND *build_equal_items(THD *thd, COND_EQUAL *inherited, > - bool link_item_fields); > + bool link_item_fields, > + COND_EQUAL **cond_equal_ref); > virtual void print(String *str, enum_query_type query_type); > void split_sum_func(THD *thd, Item **ref_pointer_array, List<Item> > &fields); > friend int setup_conds(THD *thd, TABLE_LIST *tables, TABLE_LIST *leaves, > @@ -1960,6 +1962,9 @@ class Item_equal: public Item_bool_func > void fix_length_and_dec(); > bool fix_fields(THD *thd, Item **ref); > void update_used_tables(); > + COND *build_equal_items(THD *thd, COND_EQUAL *inherited, > + bool link_item_fields, > + COND_EQUAL **cond_equal_ref); > bool walk(Item_processor processor, bool walk_subquery, uchar *arg); > Item *transform(Item_transformer transformer, uchar *arg); > virtual void print(String *str, enum_query_type query_type); > @@ -1989,6 +1994,11 @@ class COND_EQUAL: public Sql_alloc > { > upper_levels= 0; > } > + COND_EQUAL(Item_equal *item, MEM_ROOT *mem_root) > + :upper_levels(0) > + { > + current_level.push_back(item, mem_root); > + } > void copy(COND_EQUAL &cond_equal) > { > max_members= cond_equal.max_members; > @@ -2107,7 +2117,8 @@ class Item_cond_and :public Item_cond > virtual uint exists2in_reserved_items() { return list.elements; }; > bool walk_top_and(Item_processor processor, uchar *arg); > COND *build_equal_items(THD *thd, COND_EQUAL *inherited, > - bool link_item_fields); > + bool link_item_fields, > + COND_EQUAL **cond_equal_ref); > }; > > inline bool is_cond_and(Item *item) > diff --git a/sql/item_func.h b/sql/item_func.h > index 0d57c2b..a5ddb24 100644 > --- a/sql/item_func.h > +++ b/sql/item_func.h > @@ -132,7 +132,8 @@ class Item_func :public Item_func_or_sum, public > Used_tables_and_const_cache > used_tables_and_const_cache_update_and_join(arg_count, args); > } > COND *build_equal_items(THD *thd, COND_EQUAL *inherited, > - bool link_item_fields); > + bool link_item_fields, > + COND_EQUAL **cond_equal_ref); > bool eq(const Item *item, bool binary_cmp) const; > virtual optimize_type select_optimize() const { return OPTIMIZE_NONE; } > virtual bool have_rev_func() const { return 0; } > diff --git a/sql/item_sum.h b/sql/item_sum.h > index b540b44..056c623 100644 > --- a/sql/item_sum.h > +++ b/sql/item_sum.h > @@ -445,7 +445,8 @@ class Item_sum :public Item_func_or_sum > table_map used_tables() const { return used_tables_cache; } > void update_used_tables (); > COND *build_equal_items(THD *thd, COND_EQUAL *inherited, > - bool link_item_fields) > + bool link_item_fields, > + COND_EQUAL **cond_equal_ref) > { > /* > Item_sum (and derivants) of the original WHERE/HAVING clauses > @@ -453,7 +454,8 @@ class Item_sum :public Item_func_or_sum > build_equal_items() is called. See Item::split_sum_func2(). > */ > DBUG_ASSERT(0); > - return Item::build_equal_items(thd, inherited, link_item_fields); > + return Item::build_equal_items(thd, inherited, link_item_fields, > + cond_equal_ref); > } > bool is_null() { return null_value; } > void make_const () > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 95778f0..2cc0173 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -12773,7 +12773,8 @@ bool Item_func_eq::check_equality(THD *thd, > COND_EQUAL *cond_equal, > > COND *Item_cond_and::build_equal_items(THD *thd, > COND_EQUAL *inherited, > - bool link_item_fields) > + bool link_item_fields, > + COND_EQUAL **cond_equal_ref) > { > Item_equal *item_equal; > COND_EQUAL cond_equal; > @@ -12785,6 +12786,7 @@ COND *Item_cond_and::build_equal_items(THD *thd, > List_iterator<Item> li(*args); > Item *item; > > + DBUG_ASSERT(!cond_equal_ref || !cond_equal_ref[0]); > /* > Retrieve all conjuncts of this level detecting the equality > that are subject to substitution by multiple equality items and > @@ -12809,7 +12811,7 @@ COND *Item_cond_and::build_equal_items(THD *thd, > if (!args->elements && > !cond_equal.current_level.elements && > !eq_list.elements) > - return new Item_int((longlong) 1, 1); > + return new (thd->mem_root) Item_int((longlong) 1, 1); > > List_iterator_fast<Item_equal> it(cond_equal.current_level); > while ((item_equal= it++)) > @@ -12833,7 +12835,7 @@ COND *Item_cond_and::build_equal_items(THD *thd, > while ((item= li++)) > { > Item *new_item; > - if ((new_item= item->build_equal_items(thd, inherited, FALSE)) > + if ((new_item= item->build_equal_items(thd, inherited, false, NULL)) > != item) > { > /* This replacement happens only for standalone equalities */ > @@ -12848,19 +12850,23 @@ COND *Item_cond_and::build_equal_items(THD *thd, > args->append(&eq_list); > args->append((List<Item> *)&cond_equal.current_level); > update_used_tables(); > + if (cond_equal_ref) > + *cond_equal_ref= &m_cond_equal; > return this; > } > > > COND *Item_cond::build_equal_items(THD *thd, > COND_EQUAL *inherited, > - bool link_item_fields) > + bool link_item_fields, > + COND_EQUAL **cond_equal_ref) > { > List<Item> *args= argument_list(); > > List_iterator<Item> li(*args); > Item *item; > > + DBUG_ASSERT(!cond_equal_ref || !cond_equal_ref[0]); > /* > Make replacement of equality predicates for lower levels > of the condition expression. > @@ -12870,7 +12876,7 @@ COND *Item_cond::build_equal_items(THD *thd, > while ((item= li++)) > { > Item *new_item; > - if ((new_item= item->build_equal_items(thd, inherited, FALSE)) > + if ((new_item= item->build_equal_items(thd, inherited, false, NULL)) > != item) > { > /* This replacement happens only for standalone equalities */ > @@ -12889,11 +12895,14 @@ COND *Item_cond::build_equal_items(THD *thd, > > COND *Item_func_eq::build_equal_items(THD *thd, > COND_EQUAL *inherited, > - bool link_item_fields) > + bool link_item_fields, > + COND_EQUAL **cond_equal_ref) > { > COND_EQUAL cond_equal; > cond_equal.upper_levels= inherited; > List<Item> eq_list; > + > + DBUG_ASSERT(!cond_equal_ref || !cond_equal_ref[0]); > /* > If an equality predicate forms the whole and level, > we call it standalone equality and it's processed here. > @@ -12909,7 +12918,7 @@ COND *Item_func_eq::build_equal_items(THD *thd, > Item_equal *item_equal; > int n= cond_equal.current_level.elements + eq_list.elements; > if (n == 0) > - return new Item_int((longlong) 1,1); > + return new (thd->mem_root) Item_int((longlong) 1,1); > else if (n == 1) > { > if ((item_equal= cond_equal.current_level.pop())) > @@ -12919,10 +12928,14 @@ COND *Item_func_eq::build_equal_items(THD *thd, > set_if_bigger(thd->lex->current_select->max_equal_elems, > item_equal->n_field_items()); > item_equal->upper_levels= inherited; > + if (cond_equal_ref) > + *cond_equal_ref= new (thd->mem_root) COND_EQUAL(item_equal, > + thd->mem_root); > return item_equal; > } > Item *res= eq_list.pop(); > res->update_used_tables(); > + DBUG_ASSERT(res->type() == FUNC_ITEM); > return res; > } > else > @@ -12946,15 +12959,19 @@ COND *Item_func_eq::build_equal_items(THD *thd, > cond_equal.current_level= and_cond->m_cond_equal.current_level; > args->append((List<Item> *)&cond_equal.current_level); > and_cond->update_used_tables(); > + if (cond_equal_ref) > + *cond_equal_ref= &and_cond->m_cond_equal; > return and_cond; > } > } > - return Item_func::build_equal_items(thd, inherited, link_item_fields); > + return Item_func::build_equal_items(thd, inherited, link_item_fields, > + cond_equal_ref); > } > > > COND *Item_func::build_equal_items(THD *thd, COND_EQUAL *inherited, > - bool link_item_fields) > + bool link_item_fields, > + COND_EQUAL **cond_equal_ref) > { > /* > For each field reference in cond, not from equal item predicates, > @@ -12968,6 +12985,20 @@ COND *Item_func::build_equal_items(THD *thd, > COND_EQUAL *inherited, > &Item::equal_fields_propagator, > (uchar *) inherited); > cond->update_used_tables(); > + DBUG_ASSERT(cond == this); > + DBUG_ASSERT(!cond_equal_ref || !cond_equal_ref[0]); > + return cond; > +} > + > + > +COND *Item_equal::build_equal_items(THD *thd, COND_EQUAL *inherited, > + bool link_item_fields, > + COND_EQUAL **cond_equal_ref) > +{ > + COND *cond= Item_func::build_equal_items(thd, inherited, link_item_fields, > + cond_equal_ref); > + if (cond_equal_ref) > + *cond_equal_ref= new (thd->mem_root) COND_EQUAL(this, thd->mem_root); > return cond; > } > > @@ -13049,28 +13080,19 @@ static COND *build_equal_items(JOIN *join, COND > *cond, > bool link_equal_fields) > { > THD *thd= join->thd; > - COND_EQUAL *cond_equal= 0; > + > + *cond_equal_ref= NULL; > > if (cond) > { > - cond= cond->build_equal_items(thd, inherited, link_equal_fields); > - if (cond->type() == Item::COND_ITEM && > - ((Item_cond*) cond)->functype() == Item_func::COND_AND_FUNC) > - cond_equal= &((Item_cond_and*) cond)->m_cond_equal; > - > - else if (cond->type() == Item::FUNC_ITEM && > - ((Item_cond*) cond)->functype() == Item_func::MULT_EQUAL_FUNC) > + cond= cond->build_equal_items(thd, inherited, link_equal_fields, > + cond_equal_ref); > + if (*cond_equal_ref) > { > - cond_equal= new COND_EQUAL; > - cond_equal->current_level.push_back((Item_equal *) cond); > + (*cond_equal_ref)->upper_levels= inherited; > + inherited= *cond_equal_ref; > } > } > - if (cond_equal) > - { > - cond_equal->upper_levels= inherited; > - inherited= cond_equal; > - } > - *cond_equal_ref= cond_equal; > > if (join_list && !ignore_on_conds) > { _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp