maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

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