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

[Nikita Pettik <korablev@tarantool.org>]

On 04 Dec 12:52, 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

LGTM

> ---
>  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);

Nit: these lines a bit break 80 border. I'd fix this.