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

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

Hi! Thanks for the patch!

Did you think of trying to flatten the first level of tuple_format.fields
tree into an array, like it was before JSON indexes? So we would have an
array of fields, and each one has a json-subtree in it. Which is just
unused in 99.999% of cases. It could restore even more perf maybe.

See 2 comments below.

> diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
> index 9b817d3cf..ad2f251b4 100644
> --- a/src/box/tuple_format.c
> +++ b/src/box/tuple_format.c
> @@ -852,6 +852,89 @@ tuple_format1_can_store_format2_tuples(struct tuple_format *format1,
>  	return true;
>  }
>  
> +static int
> +tuple_format_required_fields_validate(struct tuple_format *format,
> +				      void *required_fields,
> +				      uint32_t required_fields_sz);
> +
> +static int
> +tuple_field_map_create_plain(struct tuple_format *format, const char *tuple,
> +			     bool validate, struct field_map_builder *builder)
> +{
> +#define check_field_type(field, pos) ({					       \
> +	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;						       \
> +	}								       \
> +})
> +
> +	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  &&
> +	    format->exact_field_count != defined_field_count) {
> +		diag_set(ClientError, ER_EXACT_FIELD_COUNT,
> +			 (unsigned) defined_field_count,
> +			 (unsigned) format->exact_field_count);
> +		return -1;
> +	}
> +	defined_field_count = MIN(defined_field_count,
> +				  tuple_format_field_count(format));
> +
> +	void *required_fields = NULL;
> +	uint32_t required_fields_sz = bitmap_size(format->total_field_count);
> +	if (validate) {
> +		required_fields = region_alloc(region, required_fields_sz);
> +		memcpy(required_fields, format->required_fields,
> +		       required_fields_sz);
> +	}
> +
> +	if (unlikely(defined_field_count == 0))
> +		goto end;

1. If the count is 0, you anyway allocate something on the region. Better do the
check before allocating anything.

> +
> +	struct json_token token = {
> +		.type = JSON_TOKEN_NUM,
> +		.num = 0,
> +	};
> +	struct tuple_field *field;
> +
> +	field = json_tree_lookup_entry(&format->fields, &format->fields.root,
> +				       &token, struct tuple_field, token);
> +	/* Check 1st field's type, but don't store offset to it. */
> +	if (validate) {
> +		check_field_type(field, pos);
> +		bit_clear(required_fields, field->id);
> +	}

2. Since you use macro, you can move bit_clear and 'if validate' into it.
These 4 lines above are repeated 2 times without changes.

Also, why do you separate the first iteration? Doesn't the first field
have offset_slot == TUPLE_OFFSET_SLOT_NIL? So its slot won't be set anyway.

> +	mp_next(&pos);
> +
> +	for (uint32_t i = 1; i < defined_field_count; i++, mp_next(&pos)) {
> +		token.num = i;
> +		field = json_tree_lookup_entry(&format->fields,
> +					       &format->fields.root, &token,
> +					       struct  tuple_field, token);
> +		if (validate) {
> +			check_field_type(field, pos);
> +			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,
> +					       0, NULL) != 0) {
> +			return -1;
> +		}
> +	}
> +
> +#undef check_field_type
> +
> +end:
> +	return validate ?
> +	       tuple_format_required_fields_validate(format, required_fields,
> +						     required_fields_sz) :
> +	       0;
> +}