Re: bzr commit into mysql-5.1-bugteam branch (epotemkin:3200) Bug#34384

[Sergei Golubchik]

Hi, Evgeny!

On Nov 06, Evgeny Potemkin wrote:
>  3200 Evgeny Potemkin	2009-11-06
>       Bug#34384: Slow down on constant conversion.
>       When values of different types are compared they're converted to a type that
>       allows correct comparison. This conversion is done for each comparison and
>       takes some time. When a constant is being compared it's possible to cache the
>       value after conversion to speedup comparison.

Looks good, thanks!

ok to push
after fixing a couple of minor issues below.
it would be also nice to mention in a changeset comment
the speedup you observed (10% ?).

> === modified file 'sql/item.h'
> --- a/sql/item.h	2009-08-28 10:55:59 +0000
> +++ b/sql/item.h	2009-11-06 12:25:00 +0000
> @@ -2891,14 +2895,25 @@ protected:
>    Field *cached_field;
>    enum enum_field_types cached_field_type;
>  public:
> -  Item_cache(): 
> -    example(0), used_table_map(0), cached_field(0),
> cached_field_type(MYSQL_TYPE_STRING) 
> +  /*
> +    TRUE <=> cache value on the store() call.
> +    In some cases it's not possible to get value of the item to be cached at
> +    the moment of cached being created & initialized. This flag tells
> Item_cache
> +    only to save the item to be cached without actually caching it.
> +    On the first call of val_xxx function the value of the item will be
> +    retrieved, cached and returned to the caller.
> +  */
> +  bool value_cached;

1. good, I didn't like the name in the previous patch 'value_caching',
this one is much better.

2. you can make 'value_cached' private now, it doesn't need to be
visible outside of the class anymore.

> +  Item_cache():
> +    example(0), used_table_map(0), cached_field(0),
> cached_field_type(MYSQL_TYPE_STRING),
> +    value_cached(0)
> === modified file 'sql/item_cmpfunc.cc'
> --- a/sql/item_cmpfunc.cc	2009-10-05 05:27:36 +0000
> +++ b/sql/item_cmpfunc.cc	2009-11-06 12:25:00 +0000
> @@ -914,10 +916,47 @@ int Arg_comparator::set_cmp_func(Item_bo
>        return 1;
>    }
>  
> +  a= cache_converted_constant(thd, a, &a_cache, type);
> +  b= cache_converted_constant(thd, b, &b_cache, type);
>    return set_compare_func(owner_arg, type);
>  }
>  
>  
> +/**
> +  Convert and cache a constant.
> +
> +  @param value      [in]  An item to cache
> +  @param cache_item [out] Placeholder for the cache item
> +  @param type       [in]  Comparison type
> +
> +  @details
> +    When given item is a constant and its type differs from comparison type
> +    then cache its value to avoid type conversion of this constant on each
> +    evaluation. In this case the value is cached and the reference to the cache
> +    is returned.
> +    Original value is returned otherwise.
> +
> +  @return cache item or original value.
> +*/
> +
> +Item** Arg_comparator::cache_converted_constant(THD *thd, Item **value,
> +                                                Item **cache_item,
> +                                                Item_result type)
> +{
> +  /* Don't need cache if doing context analysis only. */
> +  if (!thd->is_context_analysis_only() &&
> +      owner->functype() != Item_func::LIKE_FUNC &&

As discussed on IRC - you don't need to filter LIKE_FUNC out here.

> +      (*value)->const_item() && type != (*value)->result_type())
> +  {

Regards / Mit vielen Grüßen,
Sergei

-- 
   __  ___     ___ ____  __
  /  |/  /_ __/ __/ __ \/ /   Sergei Golubchik <serg@stripped>
 / /|_/ / // /\ \/ /_/ / /__  Principal Software Engineer/Server Architect
/_/  /_/\_, /___/\___\_\___/  Sun Microsystems GmbH, HRB München 161028
       <___/                  Sonnenallee 1, 85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schroeder, Wolfgang Engels, Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin Häring