[tarantool-patches] Re: [PATCH v1 2/5] box: introduce slot_cache in key_part

[Kirill Shcherbatov]

>> -	const struct key_part *part = key_def->parts;
>> +	struct key_part *part = (struct key_part *)key_def->parts;
> 1. Why?
Fixed.

> 2. These things were needed to get formats and tuples data once. Now you
> lookup format and raw data on each part in the cycle below. So it is
> slower in part_count times. See below the main comment.
> 3. As I said you verbally, when we were discussing how to implement
> comparators, you do not need to use slot cache, epochs and other
> complex JSON things to compare flat index keys, with no JSON paths. You
> should add a new template parameter to tuple_compare_slowpath like
> 'is_flat' or something, and if it is false, then use regular tuple_field
> or tuple_field_raw. If true, then try to use slot cache, epoch etc.
> Comparators are chosen once when key_def is created and template 'if'
> is compile time, so these 'if's will cost nothing.
> 
> Now tuple_field_by_part is much slower than tuple_field_raw - it makes
> additional lookup of a format (from a huge array of tuple formats),
> of tuple data (additional call), field map (one more call). Further
> both tuple_field_by_part and tuple_field_raw do 3 'if's and lookup
> in field_map. But tuple_field_raw also updates slot_cache and
> format_epoch - it is +2 assignments. So tuple_field_raw is faster.
> In this cycle in the summary it is in part_count times faster.
> 
> So please, do not use tuple_field_by_part for flat indexes. It has no
> profit for them.
> 
> All the same about key extraction and hash.
Done as you've suggested.

> 4. Tuple format should not depend on tuple. Vice versa. Please,
> consult other tuple_field methods how to solve this dependency
> cycle.
There is no such problem already.

> 5. Looks like you've ignored my point that a global format
> epoch will expire epoch in all formats when just a one
> space is altered. Please, do not. Epoch should be one per
> space.
I've introduced new function space_format_update_epoch that is called on alter.

> 6. Prefix flags with 'is_'.
Ok, fixed.

> 7. Why do you need it?
I've reeworked this function a lot as a part of following patch.
Here is wrapper that calls tuple_field_raw now in this patch.

> 8. Too small comment for such complex feature.
I've wrote a bigger comment.