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) {