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

[Serge Petrenko <sergepetrenko@tarantool.org>]

14.11.2020 21:00, Cyrill Gorcunov пишет:
> 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;						       \
>> +	}								       \
>> +}

Hi! Thanks for the review!

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


Yes, you're correct. Thanks for pointing this out!

I've amended the patch. Incremental diff is below.


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


diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
index f2db521ea..ad2f251b4 100644
--- a/src/box/tuple_format.c
+++ b/src/box/tuple_format.c
@@ -861,15 +861,14 @@ 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))) {    \
+#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;
@@ -904,9 +903,10 @@ tuple_field_map_create_plain(struct tuple_format 
*format, const char *tuple,
         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);
-       if (validate)
+       if (validate) {
+               check_field_type(field, pos);
                 bit_clear(required_fields, field->id);
+       }
         mp_next(&pos);

         for (uint32_t i = 1; i < defined_field_count; i++, mp_next(&pos)) {
@@ -914,9 +914,10 @@ tuple_field_map_create_plain(struct tuple_format 
*format, const char *tuple,
                 field = json_tree_lookup_entry(&format->fields,
&format->fields.root, &token,
                                                struct tuple_field, token);
-               check_field_type(field, pos);
-               if (validate)
+               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,

-- 
Serge Petrenko