List:Commits« Previous MessageNext Message »
From:Sergei Golubchik Date:October 14 2009 7:20pm
Subject:Re: bzr commit into mysql-5.1-bugteam branch (epotemkin:3147)
Bug#34384
View as plain text  
Hi, Evgeny!

On Oct 08, Evgeny Potemkin wrote:
> #At file:///work/bzr_trees/34384-bug-5.1-bugteam/ based on
> revid:kristofer.pettersson@stripped
> 
>  3147 Evgeny Potemkin	2009-10-08
>       Bug#34384: Slow down on constant conversion.

See below.

> === modified file 'sql/item_xmlfunc.cc'
> --- a/sql/item_xmlfunc.cc	2009-09-23 13:21:29 +0000
> +++ b/sql/item_xmlfunc.cc	2009-10-08 11:19:15 +0000
> @@ -942,6 +942,8 @@ static Item *create_comparator(MY_XPATH 
>      */
>  
>      Item *fake= new Item_string("", 0, xpath->cs);
> +    /* Don't cache fake because its value will be changed during comparison.*/
> +    fake->uncacheable= TRUE;

I wonder whether you need this 'uncacheable' property,
perhaps you can just mark the item non-const ?
like fake->set_used_tables(RAND_TABLE_BIT)

>      Item_nodeset_func *nodeset;
>      Item *scalar, *comp;
>      if (a->type() == Item::XPATH_NODESET)
> === modified file 'sql/item_cmpfunc.cc'
> --- a/sql/item_cmpfunc.cc	2009-10-05 05:27:36 +0000
> +++ b/sql/item_cmpfunc.cc	2009-10-08 11:19:15 +0000
> @@ -532,6 +532,13 @@ void Item_bool_func2::fix_length_and_dec
>    set_cmp_func();
>  }
>  
> +void Item_bool_func2::set_cmp_func()
> +{
> +  /* Don't need cache if doing context analysis only. */
> +  if (current_thd->is_context_analysis_only())
> +    tmp_arg[0]->uncacheable= tmp_arg[1]->uncacheable= TRUE;

you don't need 'uncacheable' flag for this either, you can check
thd->is_context_analysis_only() directly in cache_converted_constant()

> +  cmp.set_cmp_func(this, tmp_arg, tmp_arg+1);
> +}
>  
>  int Arg_comparator::set_compare_func(Item_bool_func2 *item, Item_result type)
>  {
> @@ -914,9 +921,42 @@ int Arg_comparator::set_cmp_func(Item_bo
>        return 1;
>    }
>  
> +  a= cache_converted_constant(a, &a_cache, type);
> +  b= cache_converted_constant(b, &b_cache, type);

ok, shouldn't you remove if()'s above ?
your function does all the conversion now

>    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(Item **value,
> +                                                Item **cache_item,
> +                                                Item_result type)
> +{
> +  if ((*value)->const_item() && !(*value)->uncacheable &&
> +      type != (*value)->result_type())
> +  {
> +    Item_cache *cache= Item_cache::get_cache(type, *value, TRUE);
> +    cache->store(*value);
> +    *cache_item= cache;
> +    return cache_item;
> +  }
> +  return value;
> +}
>  
>  void Arg_comparator::set_datetime_cmp_func(Item **a1, Item **b1)
>  {
> @@ -4372,6 +4412,11 @@ Item_func::optimize_type Item_func_like:
>  bool Item_func_like::fix_fields(THD *thd, Item **ref)
>  {
>    DBUG_ASSERT(fixed == 0);
> +
> +  /* LIKE doesn't compare arguments directly, thus no reason to cache them. */
> +  for (int i= 0; i < arg_count; i++)
> +    args[i]->uncacheable= TRUE;

why ?

> +
>    if (Item_bool_func2::fix_fields(thd, ref) ||
>        escape_item->fix_fields(thd, &escape_item))
>      return TRUE;
> === modified file 'sql/item.h'
> --- a/sql/item.h	2009-08-28 10:55:59 +0000
> +++ b/sql/item.h	2009-10-08 11:19:15 +0000
> @@ -523,6 +523,9 @@ public:
>                                             of its arguments is or contains a
>                                             subselect */
>    Item_result cmp_context;              /* Comparison context */
> +  bool uncacheable;               /* Whether item can be cached by
> +                                     Arg_comparator. See description of
> +                                     Arg_comparator::cache_converted_constant.*/

See above, I think it's unnecessary

>    // alloc & destruct is done as start of select using sql_alloc
>    Item();
>    /*
> @@ -2890,15 +2893,26 @@ 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 first val_xxx 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 late_caching;

Why wouldn't you do late caching *always* unconditionally ?
it should always work, shouldn't it ?

> +public:
> +  Item_cache(bool late_caching_arg= FALSE): 
> +    example(0), used_table_map(0), cached_field(0),
> cached_field_type(MYSQL_TYPE_STRING),
> +    late_caching(late_caching_arg)
>    {
>      fixed= 1; 
>      null_value= 1;
>    }
>    Item_cache(enum_field_types field_type_arg):
> -    example(0), used_table_map(0), cached_field(0),
> cached_field_type(field_type_arg)
> +    example(0), used_table_map(0), cached_field(0),
> cached_field_type(field_type_arg),
> +    late_caching(0)
>    {
>      fixed= 1;
>      null_value= 1;
> @@ -2933,6 +2949,8 @@ public:
>    {
>      return this == item;
>    }
> +  inline bool init_late_caching(Item *item);
> +  inline void cache_value();

eh, inline doesn't work that way. if it's inline compiler puts the code
of the function where it's used, instead of generating a function call.
but to do that it *needs to see the code*, kind of.

which means the method body must be in the .h file, not in .cc

>  };
>  
>  
> === modified file 'sql/item.cc'
> --- a/sql/item.cc	2009-09-29 02:23:38 +0000
> +++ b/sql/item.cc	2009-10-08 11:19:15 +0000
> @@ -6978,6 +7012,8 @@ void Item_cache_int::store(Item *item)
>  
>  void Item_cache_int::store(Item *item, longlong val_arg)
>  {
> +  if (init_late_caching(item))
> +    return;

I don't see how this could work. cache->store() is called with an
explicit value to cache, you cannot cache it late, as you
only remember the item, not the value, for the late caching.

furtermore, if the value is avaiulable, the cache obviously cannot be
late caching at all. DBUG_ASSERT(!late_caching) would be more appropriate
here.

>    value= val_arg;
>    null_value= item->null_value;
>    unsigned_flag= item->unsigned_flag;
> 

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
Thread
bzr commit into mysql-5.1-bugteam branch (epotemkin:3147) Bug#34384Evgeny Potemkin8 Oct
  • Re: bzr commit into mysql-5.1-bugteam branch (epotemkin:3147)Bug#34384Sergei Golubchik8 Oct
    • Re: bzr commit into mysql-5.1-bugteam branch (epotemkin:3147) Bug#34384Evgeny Potemkin9 Oct
      • Re: bzr commit into mysql-5.1-bugteam branch (epotemkin:3147) Bug#34384Roy Lyseng9 Oct
        • Re: bzr commit into mysql-5.1-bugteam branch (epotemkin:3147) Bug#34384Evgeny Potemkin13 Oct
  • Re: bzr commit into mysql-5.1-bugteam branch (epotemkin:3147) Bug#34384Øystein Grøvlen9 Oct
  • Re: bzr commit into mysql-5.1-bugteam branch (epotemkin:3147)Bug#34384Sergei Golubchik14 Oct