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 CD20B45C304 for ; Wed, 2 Dec 2020 01:03:13 +0300 (MSK) References: <0b29e29d9ff3cf4f06a9bbe4357c3177b5f0b4e0.1606734392.git.sergepetrenko@tarantool.org> From: Vladislav Shpilevoy Message-ID: <9186497b-e9f6-a925-6c8c-c8ff1cbff2d0@tarantool.org> Date: Tue, 1 Dec 2020 23:03:11 +0100 MIME-Version: 1.0 In-Reply-To: <0b29e29d9ff3cf4f06a9bbe4357c3177b5f0b4e0.1606734392.git.sergepetrenko@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Serge Petrenko , korablev@tarantool.org Cc: tarantool-patches@dev.tarantool.org Thanks for the patch! Please, add a whitespace after 'box:' in the commit title. 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. > + 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; > } > > /**