maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Alexander,

looks good, just a few minor suggestions inline.

Some measures on my computer:
add_key_fields                       0.20% -> out of radar
Item_func_between::add_key_fields          -> 0.05%
Item_equal::add_key_fields                 -> 0.03%

Item_equal::select_optimize          0.01% -> out of radar
Item_func_between::select_optimize   0.00% -> out of radar

Item_func_eq::functype               0.04% -> 0.03%
Item_equal::functype                 0.04% -> 0.02%
Item_func_between::functype          0.03% -> 0.02%
Item_func::functype                  0.00% -> 0.00%

Item_func::type                      0.14% -> 0.11%
Item_field::type                     0.08% -> 0.08%
Item_int::type                       0.04% -> 0.03%
Item_sum::type                       0.02% -> 0.01%

Total saving: 0.22%

On Fri, May 15, 2015 at 06:58:38PM +0400, Alexander Barkov wrote:
> Hi Sergey,
> 
> 
> Please review the next iteration for MDEV-7950.
> 
> 
> This one splits the function add_key_fields() into a method in Item.
> This change removes about 3 virtual calls item->type(), as well as some
> virtual calls item_func->functype(), and adds one virtual call
> item->add_key_fields() instead.
> 
> Thanks.
...skip...

> diff --git a/sql/item_geofunc.h b/sql/item_geofunc.h
> index 2dbb2cd..b8cbed6 100644
> --- a/sql/item_geofunc.h
> +++ b/sql/item_geofunc.h
> @@ -404,7 +404,6 @@ class Item_func_isempty: public Item_bool_func
>  public:
>    Item_func_isempty(Item *a): Item_bool_func(a) {}
>    longlong val_int();
> -  optimize_type select_optimize() const { return OPTIMIZE_NONE; }
>    const char *func_name() const { return "st_isempty"; }
>    void fix_length_and_dec() { maybe_null= 1; }
>  };
Hmm... does select_optimize() make sense at all now? I mean won't code become
simpler if we move it's logic to appropriate callers?

> diff --git a/sql/sql_select.cc b/sql/sql_select.cc
> index 83bf487..947592a 100644
> --- a/sql/sql_select.cc
> +++ b/sql/sql_select.cc
> @@ -4602,240 +4603,283 @@ is_local_field (Item *field)
...skip...

> +void
> +Item_bool_func::add_key_fields(JOIN *join, KEY_FIELD **key_fields,
> +                               uint *and_level, table_map usable_tables,
> +                               SARGABLE_PARAM **sargables)
> +{
> +  /* If item is of type 'field op field/constant' add it to key_fields */
> +  switch (select_optimize()) {
> +  case Item_func::OPTIMIZE_NONE:
> +    break;
>    case Item_func::OPTIMIZE_OP:
>    {
> -    bool equal_func=(cond_func->functype() == Item_func::EQ_FUNC ||
> -		     cond_func->functype() == Item_func::EQUAL_FUNC);
> +    bool equal_func= (functype() == Item_func::EQ_FUNC ||
> +		      functype() == Item_func::EQUAL_FUNC);
>  
> -    if (is_local_field (cond_func->arguments()[0]))
> +    if (is_local_field(args[0]))
>      {
> -      add_key_equal_fields(join, key_fields, *and_level, cond_func,
> -                           (Item_field*) (cond_func->arguments()[0])->
> -                           real_item(),
> -		           equal_func,
> -                           cond_func->arguments()+1, 1, usable_tables,
> -                           sargables);
> +      add_key_equal_fields(join, key_fields, *and_level, this,
> +                           (Item_field*) args[0]->real_item(), equal_func,
> +                           args + 1, 1, usable_tables, sargables);
>      }
> -    if (is_local_field (cond_func->arguments()[1]) &&
> -	cond_func->functype() != Item_func::LIKE_FUNC)
> +    if (is_local_field(args[1]))
>      {
> -      add_key_equal_fields(join, key_fields, *and_level, cond_func, 
> -                           (Item_field*) (cond_func->arguments()[1])->
> -                           real_item(),
> -		           equal_func,
> -                           cond_func->arguments(),1,usable_tables,
> -                           sargables);
> +      add_key_equal_fields(join, key_fields, *and_level, this, 
> +                           (Item_field*) args[1]->real_item(), equal_func,
> +                           args, 1, usable_tables, sargables);
>      }
>      break;
>    }
> +  case Item_func::OPTIMIZE_KEY:
>    case Item_func::OPTIMIZE_NULL:
> -    /* column_name IS [NOT] NULL */
> -    if (is_local_field (cond_func->arguments()[0]) &&
> -	!(cond_func->used_tables() & OUTER_REF_TABLE_BIT))
> -    {
> -      Item *tmp=new Item_null;
> -      if (unlikely(!tmp))                       // Should never be true
> -	return;
> -      add_key_equal_fields(join, key_fields, *and_level, cond_func,
> -                           (Item_field*) (cond_func->arguments()[0])->
> -                           real_item(),
> -                           cond_func->functype() == Item_func::ISNULL_FUNC,
> -			   &tmp, 1, usable_tables, sargables);
> -    }
> -    break;
>    case Item_func::OPTIMIZE_EQUAL:
> -    Item_equal *item_equal= (Item_equal *) cond;
> -    Item *const_item= item_equal->get_const();
> -    Item_equal_fields_iterator it(*item_equal);
> -    if (const_item)
> +    DBUG_ASSERT(0);
> +    break;
> +  }
> +}
> +
> +
I'd better do:
if (select_optimize() == Item_func::OPTIMIZE_OP)
{
  ...
  return;
}
DBUG_ASSERT(select_optimize() == Item_func::OPTIMIZE_NONE);

Also OPTIMIZE_OP is only set by Item_bool_func2 and Item_func_spatial_rel. Why
not to create methods for them instead?

Thanks,
Sergey