From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (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 2903845C304 for ; Wed, 2 Dec 2020 12:53:23 +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> <76a500bf-baa4-fc72-825d-ce324a190abf@tarantool.org> From: Serge Petrenko Message-ID: Date: Wed, 2 Dec 2020 12:53:21 +0300 MIME-Version: 1.0 In-Reply-To: <76a500bf-baa4-fc72-825d-ce324a190abf@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB 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: Vladislav Shpilevoy , korablev@tarantool.org Cc: tarantool-patches@dev.tarantool.org 02.12.2020 01:04, Vladislav Shpilevoy пишет: > 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. I see. I thought the compiler would do the same if it saw an inline function with explicit boolean argument. > >> 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. Thanks for all the suggestions! Tested this diff. My numbers must be off somewhere in the previous letter, because tuple_field_map_create() took ~2.15 seconds back then. This must be because I took a 19 second profile, instead of a 15 second one, I guess. Anyway, all the comparisons from the previous letter were valid. During this test tuple_field_map_create() took ~  1.6 seconds. With and without the diff applied. I double-checked it this time. It is a 15 second profile. So looks like splitting by validate doesn't help much. > > ==================== > 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; -- Serge Petrenko