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

[Serge Petrenko <sergepetrenko@tarantool.org>]

10.12.2020 20:25, Nikita Pettik пишет:
> 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.


Ok, here:


diff --git a/src/box/tuple.h b/src/box/tuple.h
index fd373fdbf..e4267a4ec 100644
--- a/src/box/tuple.h
+++ b/src/box/tuple.h
@@ -710,7 +710,8 @@ tuple_field_raw(struct tuple_format *format, const 
char *tuple,
                 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);
+               offset = field_map_get_offset(field_map, offset_slot,
+                                             MULTIKEY_NONE);
                 if (offset == 0)
                         return NULL;
                 tuple += offset;


I wasn't sure how to fix the

`struct json_token *token = format->fields.root.children[field_no];`

line, so I left it as is.

>
-- 
Serge Petrenko