Re: [Tarantool-patches] [PATCH v2 2/2] box:refactor tuple_field_raw to omit path checks

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

Please, add a whitespace after 'box:' in the commit title.

The patch looks good. See one another possible opt below.
But the patch already is fine, and the opt is dubious - you
can ignore it if you think all is good already.

On 30.11.2020 12:14, Serge Petrenko wrote:
> tuple_field_raw is an alias to tuple_field_raw_by_path with zero path.
> This involves multiple path != NULL checks which aren't needed for tuple
> field access by field number. The checks make this function rather slow
> compared to its 1.10 counterpart (see results below).
> 
> In order to fix perf problems when JSON path indices aren't involved,
> factor out the part of tuple_field_raw_by_path which is responsible for
> direct field access by number and place it in tuple_field_raw.
> 
> This patch was tested by snapshot recovery part involving secondary
> index building for a 1.5G snapshot with
> one space and one secondary index over 4 integer and one string field.
> Comparison table is below:
> 
>     Version    | time(seconds)  | Change relative to 1.10
> ---------------|----------------|------------------------
> 1.10           |      2:24      |           -/-
> 2.x(unpatched) |      3:03      |          + 27%
> 2.x (patched)  |      2:10      |          - 10%
> 
> Numbers below show cumulative time spent in tuple_compare_slowpath,
> for 1.10 / 2.x(unpatched) / 2.x(patched) for 15, 19 and 14 second
> profiles respectively: 13.9 / 17.8 / 12.5.
> 
> tuple_field_raw() isn't measured directly, since it's inlined, but all
> its uses come from tuple_compare_slowpath.
> 
> As the results show, we manage to be even faster, than 1.10 used to be
> in this test. This must be due to tuple comparison hints, which are
> present only in 2.x.
> 
> Closes #4774
> ---
>  src/box/tuple.h | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/tuple.h b/src/box/tuple.h
> index 755aee506..fd373fdbf 100644
> --- a/src/box/tuple.h
> +++ b/src/box/tuple.h
> @@ -697,8 +697,33 @@ static inline const char *
>  tuple_field_raw(struct tuple_format *format, const char *tuple,
>  		const uint32_t *field_map, uint32_t field_no)
>  {
> -	return tuple_field_raw_by_path(format, tuple, field_map, field_no,
> -				       NULL, 0, NULL, MULTIKEY_NONE);
> +	if (likely(field_no < format->index_field_count)) {
> +		int32_t offset_slot;
> +		uint32_t offset = 0;
> +		struct tuple_field *field;
> +		if (field_no == 0) {
> +			mp_decode_array(&tuple);
> +			return tuple;
> +		}
> +		struct json_token *token = format->fields.root.children[field_no];
> +		field = json_tree_entry(token, struct tuple_field, token);
> +		offset_slot = field->offset_slot;
> +		if (offset_slot == TUPLE_OFFSET_SLOT_NIL)
> +			goto parse;
> +		offset = field_map_get_offset(field_map, offset_slot, MULTIKEY_NONE);

What if we would remove multikey argument from field_map_get_offset,
and introduce a new function field_map_get_offset_mk?

field_map_get_offset would do a plain load_u32 without any ifs.
field_map_get_offset_mk would check multikey like now, but we won't
use it here.

> +		if (offset == 0)
> +			return NULL;> +		tuple += offset;
> +	} else {
> +parse:
> +		ERROR_INJECT(ERRINJ_TUPLE_FIELD, return NULL);
> +		uint32_t field_count = mp_decode_array(&tuple);
> +		if (unlikely(field_no >= field_count))
> +			return NULL;
> +		for ( ; field_no > 0; field_no--)
> +			mp_next(&tuple);
> +	}
> +	return tuple;
>  }
>  
>  /**