maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

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