Hi Sergei, Sending an updated version, according to our discussion on the phone. On 05/19/2015 01:49 PM, Sergey Vojtovich wrote:

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