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

[Serge Petrenko <sergepetrenko@tarantool.org>]

02.12.2020 01:03, Vladislav Shpilevoy пишет:
> Thanks for the patch!
>
> Please, add a whitespace after 'box:' in the commit title.


Thanks for the review!

Fixed.

>
> 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.

Ok. Tried the diff below. Tested 2 times. 1st time 2:13, 2nd time 2:06.

Average 2:09.5. Same as without the diff.


==================================

diff --git a/src/box/field_map.h b/src/box/field_map.h
index d8ef726a1..6e33e888d 100644
--- a/src/box/field_map.h
+++ b/src/box/field_map.h
@@ -172,6 +172,17 @@ field_map_get_offset(const uint32_t *field_map, 
int32_t offset_slot,
         return offset;
  }

+static inline uint32_t
+field_map_get_offset_plain(const uint32_t *field_map, int32_t offset_slot)
+{
+       /*
+        * Can not access field_map as a normal uint32 array
+        * because its alignment may be < 4 bytes. Need to use
+        * unaligned store-load operations explicitly.
+        */
+       uint32_t offset = load_u32(&field_map[offset_slot]);
+       return offset;
+}
  /**
   * Initialize field_map_builder.
   *
diff --git a/src/box/tuple.h b/src/box/tuple.h
index fd373fdbf..c79b0dd87 100644
--- a/src/box/tuple.h
+++ b/src/box/tuple.h
@@ -710,7 +710,7 @@ 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_plain(field_map, offset_slot);
                 if (offset == 0)
                         return NULL;
                 tuple += offset;

>
>> +		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;
>>   }
>>   
>>   /**

-- 
Serge Petrenko