From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (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 1AF9B45C304 for ; Wed, 2 Dec 2020 13:22:46 +0300 (MSK) References: <0b29e29d9ff3cf4f06a9bbe4357c3177b5f0b4e0.1606734392.git.sergepetrenko@tarantool.org> <9186497b-e9f6-a925-6c8c-c8ff1cbff2d0@tarantool.org> From: Serge Petrenko Message-ID: <4d7b9f9b-68d8-65ff-8f7b-bd627c7e6253@tarantool.org> Date: Wed, 2 Dec 2020 13:22:44 +0300 MIME-Version: 1.0 In-Reply-To: <9186497b-e9f6-a925-6c8c-c8ff1cbff2d0@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH v2 2/2] box:refactor tuple_field_raw to omit path checks 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:03, Vladislav Shpilevoy пишет: > Thanks for the patch! > > Please, add a whitespace after 'box:' in the commit title. Thanks for the review! Fixed. > > The patch looks good. See one another possible opt below. > But the patch already is fine, and the opt is dubious - you > can ignore it if you think all is good already. > > On 30.11.2020 12:14, Serge Petrenko wrote: >> tuple_field_raw is an alias to tuple_field_raw_by_path with zero path. >> This involves multiple path != NULL checks which aren't needed for tuple >> field access by field number. The checks make this function rather slow >> compared to its 1.10 counterpart (see results below). >> >> In order to fix perf problems when JSON path indices aren't involved, >> factor out the part of tuple_field_raw_by_path which is responsible for >> direct field access by number and place it in tuple_field_raw. >> >> This patch was tested by snapshot recovery part involving secondary >> index building for a 1.5G snapshot with >> one space and one secondary index over 4 integer and one string field. >> Comparison table is below: >> >> Version | time(seconds) | Change relative to 1.10 >> ---------------|----------------|------------------------ >> 1.10 | 2:24 | -/- >> 2.x(unpatched) | 3:03 | + 27% >> 2.x (patched) | 2:10 | - 10% >> >> Numbers below show cumulative time spent in tuple_compare_slowpath, >> for 1.10 / 2.x(unpatched) / 2.x(patched) for 15, 19 and 14 second >> profiles respectively: 13.9 / 17.8 / 12.5. >> >> tuple_field_raw() isn't measured directly, since it's inlined, but all >> its uses come from tuple_compare_slowpath. >> >> As the results show, we manage to be even faster, than 1.10 used to be >> in this test. This must be due to tuple comparison hints, which are >> present only in 2.x. >> >> Closes #4774 >> --- >> src/box/tuple.h | 29 +++++++++++++++++++++++++++-- >> 1 file changed, 27 insertions(+), 2 deletions(-) >> >> diff --git a/src/box/tuple.h b/src/box/tuple.h >> index 755aee506..fd373fdbf 100644 >> --- a/src/box/tuple.h >> +++ b/src/box/tuple.h >> @@ -697,8 +697,33 @@ static inline const char * >> tuple_field_raw(struct tuple_format *format, const char *tuple, >> const uint32_t *field_map, uint32_t field_no) >> { >> - return tuple_field_raw_by_path(format, tuple, field_map, field_no, >> - NULL, 0, NULL, MULTIKEY_NONE); >> + if (likely(field_no < format->index_field_count)) { >> + int32_t offset_slot; >> + uint32_t offset = 0; >> + struct tuple_field *field; >> + if (field_no == 0) { >> + mp_decode_array(&tuple); >> + return tuple; >> + } >> + struct json_token *token = format->fields.root.children[field_no]; >> + field = json_tree_entry(token, struct tuple_field, token); >> + offset_slot = field->offset_slot; >> + if (offset_slot == TUPLE_OFFSET_SLOT_NIL) >> + goto parse; >> + offset = field_map_get_offset(field_map, offset_slot, MULTIKEY_NONE); > What if we would remove multikey argument from field_map_get_offset, > and introduce a new function field_map_get_offset_mk? > > field_map_get_offset would do a plain load_u32 without any ifs. > field_map_get_offset_mk would check multikey like now, but we won't > use it here. Ok. Tried the diff below. Tested 2 times. 1st time 2:13, 2nd time 2:06. Average 2:09.5. Same as without the diff. ================================== diff --git a/src/box/field_map.h b/src/box/field_map.h index d8ef726a1..6e33e888d 100644 --- a/src/box/field_map.h +++ b/src/box/field_map.h @@ -172,6 +172,17 @@ field_map_get_offset(const uint32_t *field_map, int32_t offset_slot,         return offset;  } +static inline uint32_t +field_map_get_offset_plain(const uint32_t *field_map, int32_t offset_slot) +{ +       /* +        * Can not access field_map as a normal uint32 array +        * because its alignment may be < 4 bytes. Need to use +        * unaligned store-load operations explicitly. +        */ +       uint32_t offset = load_u32(&field_map[offset_slot]); +       return offset; +}  /**   * Initialize field_map_builder.   * diff --git a/src/box/tuple.h b/src/box/tuple.h index fd373fdbf..c79b0dd87 100644 --- a/src/box/tuple.h +++ b/src/box/tuple.h @@ -710,7 +710,7 @@ tuple_field_raw(struct tuple_format *format, const char *tuple,                 offset_slot = field->offset_slot;                 if (offset_slot == TUPLE_OFFSET_SLOT_NIL)                         goto parse; -               offset = field_map_get_offset(field_map, offset_slot, MULTIKEY_NONE); +               offset = field_map_get_offset_plain(field_map, offset_slot);                 if (offset == 0)                         return NULL;                 tuple += offset; > >> + if (offset == 0) >> + return NULL;> + tuple += offset; >> + } else { >> +parse: >> + ERROR_INJECT(ERRINJ_TUPLE_FIELD, return NULL); >> + uint32_t field_count = mp_decode_array(&tuple); >> + if (unlikely(field_no >= field_count)) >> + return NULL; >> + for ( ; field_no > 0; field_no--) >> + mp_next(&tuple); >> + } >> + return tuple; >> } >> >> /** -- Serge Petrenko