Re: [Tarantool-patches] [PATCH 1/2] box: speed up tuple_field_map_create

[Serge Petrenko <sergepetrenko@tarantool.org>]

02.12.2020 01:04, Vladislav Shpilevoy пишет:
> Thanks for the investigation!
>
>>> Then we probably also could gain some perf by splitting these functions
>>> into 2 versions with validate true and validate false. Because we always
>>> know it at compile time. Would be cool to use templates for this, but not
>>> sure if we can simply change tuple.c to tuple.cc only for that.
>>
>> You mean like that?
>>
>> tuple_field_map_create(...) {
>>
>> tuple_field_map_create_impl(..., true, ...);
>>
>> }
>>
>> tuple_field_map_create_unchecked(...) {
>>
>> tuple_field_mp_create_impl(..., false, ...);
>>
>> }
>>
>> I tried this approach, it didn't give anything. tuple_field_map_create time 2.15 s.
> No, I mean create 2 versions of tuple_field_map_create. It leads
> to some code duplication, and we probably don't want to do that
> this way. But if it will give notable perf increase, we may decide
> to consider templates.


I see. I thought the compiler would do the same if it saw an inline function

with explicit boolean argument.


>
>> Is this possible that compiler already evaluates `validate`?
> I guess it can, but we can not be sure it will always do so.
>
> Consider this diff. Super ugly, but should reveal if validate
> matters. After all, we have at least 4 validate checks here. The
> diff replaces it with 1.


Thanks for all the suggestions!

Tested this diff. My numbers must be off somewhere in the previous letter,
because tuple_field_map_create() took ~2.15 seconds back then.

This must be because I took a 19 second profile, instead of a 15 second 
one, I guess.
Anyway, all the comparisons from the previous letter were valid.

During this test tuple_field_map_create() took ~  1.6 seconds. With and 
without
the diff applied. I double-checked it this time. It is a 15 second profile.
So looks like splitting by validate doesn't help much.


>
> ====================
> diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
> index 6c9b2a255..5ee0c4484 100644
> --- a/src/box/tuple_format.c
> +++ b/src/box/tuple_format.c
> @@ -859,12 +859,38 @@ tuple_format_required_fields_validate(struct tuple_format *format,
>   
>   static int
>   tuple_field_map_create_plain(struct tuple_format *format, const char *tuple,
> -			     bool validate, struct field_map_builder *builder)
> +			     struct field_map_builder *builder)
> +{
> +	const char *pos = tuple;
> +	uint32_t defined_field_count = mp_decode_array(&pos);
> +	defined_field_count = MIN(defined_field_count,
> +				  tuple_format_field_count(format));
> +	if (unlikely(defined_field_count == 0))
> +		return 0;
> +
> +	struct tuple_field *field;
> +	struct json_token **token = format->fields.root.children;
> +	for (uint32_t i = 0; i < defined_field_count; i++, token++, mp_next(&pos)) {
> +		field = json_tree_entry(*token, struct tuple_field, token);
> +		if (field->offset_slot != TUPLE_OFFSET_SLOT_NIL &&
> +		    field_map_builder_set_slot(builder, field->offset_slot,
> +					       pos - tuple, MULTIKEY_NONE,
> +					       0, NULL) != 0) {
> +			return -1;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int
> +tuple_field_map_create_and_validate_plain(struct tuple_format *format,
> +					  const char *tuple,
> +					  struct field_map_builder *builder)
>   {
>   	struct region *region = &fiber()->gc;
>   	const char *pos = tuple;
>   	uint32_t defined_field_count = mp_decode_array(&pos);
> -	if (validate && format->exact_field_count > 0 &&
> +	if (format->exact_field_count > 0 &&
>   	    format->exact_field_count != defined_field_count) {
>   		diag_set(ClientError, ER_EXACT_FIELD_COUNT,
>   			 (unsigned) defined_field_count,
> @@ -881,28 +907,22 @@ tuple_field_map_create_plain(struct tuple_format *format, const char *tuple,
>   		required_fields = format->required_fields;
>   		goto end;
>   	}
> -
> -	if (validate) {
> -		required_fields = region_alloc(region, required_fields_sz);
> -		memcpy(required_fields, format->required_fields,
> -		       required_fields_sz);
> -	}
> +	required_fields = region_alloc(region, required_fields_sz);
> +	memcpy(required_fields, format->required_fields,
> +	       required_fields_sz);
>   
>   	struct tuple_field *field;
>   	struct json_token **token = format->fields.root.children;
>   	for (uint32_t i = 0; i < defined_field_count; i++, token++, mp_next(&pos)) {
>   		field = json_tree_entry(*token, struct tuple_field, token);
> -		if (validate) {
> -			bool nullable = tuple_field_is_nullable(field);
> -			if(!field_mp_type_is_compatible(field->type, pos,
> -							nullable)) {
> -				diag_set(ClientError, ER_FIELD_TYPE,
> -					 tuple_field_path(field),
> -					 field_type_strs[field->type]);
> -				return -1;
> -			}
> -			bit_clear(required_fields, field->id);
> +		bool nullable = tuple_field_is_nullable(field);
> +		if(!field_mp_type_is_compatible(field->type, pos, nullable)) {
> +			diag_set(ClientError, ER_FIELD_TYPE,
> +				 tuple_field_path(field),
> +				 field_type_strs[field->type]);
> +			return -1;
>   		}
> +		bit_clear(required_fields, field->id);
>   		if (field->offset_slot != TUPLE_OFFSET_SLOT_NIL &&
>   		    field_map_builder_set_slot(builder, field->offset_slot,
>   					       pos - tuple, MULTIKEY_NONE,
> @@ -910,12 +930,9 @@ tuple_field_map_create_plain(struct tuple_format *format, const char *tuple,
>   			return -1;
>   		}
>   	}
> -
>   end:
> -	return validate ?
> -	       tuple_format_required_fields_validate(format, required_fields,
> -						     required_fields_sz) :
> -	       0;
> +	return tuple_format_required_fields_validate(format, required_fields,
> +						     required_fields_sz);
>   }
>   
>   /** @sa declaration for details. */
> @@ -935,8 +952,11 @@ tuple_field_map_create(struct tuple_format *format, const char *tuple,
>   	 * tuple field traversal may be simplified.
>   	 */
>   	if (format->fields_depth == 1) {
> -		return tuple_field_map_create_plain(format, tuple, validate,
> -						    builder);
> +		if (validate) {
> +			return tuple_field_map_create_and_validate_plain(
> +				format, tuple, builder);
> +		}
> +		return tuple_field_map_create_plain(format, tuple, builder);
>   	}
>   
>   	uint32_t field_count;

-- 
Serge Petrenko