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 _______________________________________________ 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