From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (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 1293D469719 for ; Mon, 16 Nov 2020 10:34:43 +0300 (MSK) References: <20201114172823.8217-1-sergepetrenko@tarantool.org> <20201114172823.8217-2-sergepetrenko@tarantool.org> <20201114180034.GM2021@grain> From: Serge Petrenko Message-ID: Date: Mon, 16 Nov 2020 10:34:42 +0300 MIME-Version: 1.0 In-Reply-To: <20201114180034.GM2021@grain> 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: Cyrill Gorcunov Cc: v.shpilevoy@tarantool.org, tarantool-patches@dev.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