From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 1F54B469710 for ; Sat, 21 Nov 2020 02:26:42 +0300 (MSK) References: <20201114172823.8217-1-sergepetrenko@tarantool.org> <20201114172823.8217-2-sergepetrenko@tarantool.org> From: Vladislav Shpilevoy Message-ID: <635cf529-93e6-09e3-73d4-bba8ad04e793@tarantool.org> Date: Sat, 21 Nov 2020 00:26:39 +0100 MIME-Version: 1.0 In-Reply-To: <20201114172823.8217-2-sergepetrenko@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 Hi! Thanks for the patch! Did you think of trying to flatten the first level of tuple_format.fields tree into an array, like it was before JSON indexes? So we would have an array of fields, and each one has a json-subtree in it. Which is just unused in 99.999% of cases. It could restore even more perf maybe. See 2 comments below. > diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c > index 9b817d3cf..ad2f251b4 100644 > --- a/src/box/tuple_format.c > +++ b/src/box/tuple_format.c > @@ -852,6 +852,89 @@ tuple_format1_can_store_format2_tuples(struct tuple_format *format1, > return true; > } > > +static int > +tuple_format_required_fields_validate(struct tuple_format *format, > + void *required_fields, > + uint32_t required_fields_sz); > + > +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) ({ \ > + 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; > + uint32_t defined_field_count = mp_decode_array(&pos); > + if (validate && format->exact_field_count > 0 && > + format->exact_field_count != defined_field_count) { > + diag_set(ClientError, ER_EXACT_FIELD_COUNT, > + (unsigned) defined_field_count, > + (unsigned) format->exact_field_count); > + return -1; > + } > + defined_field_count = MIN(defined_field_count, > + tuple_format_field_count(format)); > + > + void *required_fields = NULL; > + uint32_t required_fields_sz = bitmap_size(format->total_field_count); > + if (validate) { > + required_fields = region_alloc(region, required_fields_sz); > + memcpy(required_fields, format->required_fields, > + required_fields_sz); > + } > + > + if (unlikely(defined_field_count == 0)) > + goto end; 1. If the count is 0, you anyway allocate something on the region. Better do the check before allocating anything. > + > + struct json_token token = { > + .type = JSON_TOKEN_NUM, > + .num = 0, > + }; > + struct tuple_field *field; > + > + 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. */ > + if (validate) { > + check_field_type(field, pos); > + bit_clear(required_fields, field->id); > + } 2. Since you use macro, you can move bit_clear and 'if validate' into it. These 4 lines above are repeated 2 times without changes. Also, why do you separate the first iteration? Doesn't the first field have offset_slot == TUPLE_OFFSET_SLOT_NIL? So its slot won't be set anyway. > + mp_next(&pos); > + > + for (uint32_t i = 1; i < defined_field_count; i++, mp_next(&pos)) { > + token.num = i; > + field = json_tree_lookup_entry(&format->fields, > + &format->fields.root, &token, > + struct tuple_field, token); > + 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, > + 0, NULL) != 0) { > + return -1; > + } > + } > + > +#undef check_field_type > + > +end: > + return validate ? > + tuple_format_required_fields_validate(format, required_fields, > + required_fields_sz) : > + 0; > +}