From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 711BF45C304 for ; Wed, 2 Dec 2020 01:04:11 +0300 (MSK) References: <20201114172823.8217-1-sergepetrenko@tarantool.org> <20201114172823.8217-2-sergepetrenko@tarantool.org> <635cf529-93e6-09e3-73d4-bba8ad04e793@tarantool.org> <2b49855f-3337-0828-465b-38fbd7ef0912@tarantool.org> <9eba270a-c0fb-3238-bc2e-2e39fe73eb91@tarantool.org> <301e2d4d-3534-d03e-4dec-84c72d4c351d@tarantool.org> From: Vladislav Shpilevoy Message-ID: <76a500bf-baa4-fc72-825d-ce324a190abf@tarantool.org> Date: Tue, 1 Dec 2020 23:04:09 +0100 MIME-Version: 1.0 In-Reply-To: <301e2d4d-3534-d03e-4dec-84c72d4c351d@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 1/2] box: speed up tuple_field_map_create List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Serge Petrenko , korablev@tarantool.org Cc: tarantool-patches@dev.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;