maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #08566
Re: Step#4: MDEV-7950 Item_func::type() takes 0.26% in OLTP RO
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)
> {
References
