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

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Sat, Nov 14, 2020 at 08:28:22PM +0300, Serge Petrenko wrote:
...
> +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) {					       \
> +	if (validate &&							       \
> +	    !field_mp_type_is_compatible(field->type, pos,		       \
> +					 tuple_field_is_nullable(field))) {    \
> +		diag_set(ClientError, ER_FIELD_TYPE, tuple_field_path(field),  \
> +			 field_type_strs[field->type]);			       \
> +		return -1;						       \
> +	}								       \
> +}

Serge, I'm completely not familiar with the code thus may be simply wrong but
@check_field_type test for @validate first, right?
...
> +
> +	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. */
> +	check_field_type(field, pos);

	check_field_type -> if (validate ...)

> +	if (validate)
> +		bit_clear(required_fields, field->id);

and here we test for if (validate) again. Should not we simply
drop if (validate) from check_field_type and call this macro under
the caller's if? IOW

	if (validate) {
		check_field_type();
		bit_clear();
	}

While check_field_type will be something like

#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;						\
	}								\
})

 - if I'm right we may fix it on top (actually since these two ifs are close
   to each other they won't hurt hw branch predictor even in current form or
   may be compiler merge these two basic blocks under one "if" flow)

> +		check_field_type(field, pos);
> +		if (validate)
> +			bit_clear(required_fields, field->id);

and here too.

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