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

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

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.

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

====================
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;