From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 17 Sep 2018 20:08:58 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] Re: [PATCH v3 2/4] box: introduce slot_cache in key_part Message-ID: <20180917170858.u6zw7asvnibss56s@esperanza> References: <9be616bdd2190a0ab38aaee981098811f542eeb1.1535354849.git.kshcherbatov@tarantool.org> <94ec4d86-f611-b1c8-9665-ababdf21481e@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Kirill Shcherbatov Cc: Vladislav Shpilevoy , Tarantool MailList List-ID: It looks like you plucked some random parts from the main patch just to make it look smaller. Please don't do that, because it doesn't make it any easier to review. Split a patch set so that each individual patch makes sense and can be applied independently. I see at least two separate things you're doing here that could be committed after the first review iteration: 1. Deconstantifying key_def throughout the code. 2. Introducing tuple_field_by_part helper. Please factor out those parts in separate patches and write a brief descriptive comment to each of them (what you're doing in a nutshell and why). Before you do that, I can't review this patch without risking to miss something critical. A few things that I did manage to notice: - You add has_json_path template parameter to comparators. This looks completely out-of-place. I think this change should go along with the patch that implements json path indexes. - Although offset_slot and offset_slot_epoch are added to key_part, they are never used. Looks like this also should go to another patch. May be, it's worth factoring out the 'epoch' optimization to a separate patch that would be applied on top of json indexes - after all, it's just an optimization, right? - The epoch is really easy to miss when creating a tuple format: > @@ -158,6 +158,7 @@ blackhole_engine_create_space(struct engine *engine, struct space_def *def, > return NULL; > } > format->exact_field_count = def->exact_field_count; > + format->epoch = ++epoch; I think it should be passed as an argument to tuple_format_new. Also, IMO it'd be more straightforward to pass the actual epoch that should be used for the new format, not the previous one. - In tuple_common_key_parts the following type cast isn't needed: > + struct key_part *part = (struct key_part *)&key_def->parts[i]; On Thu, Sep 06, 2018 at 03:47:09PM +0300, Kirill Shcherbatov wrote: > From 996ee351112bb070d511636bc702496bc445f047 Mon Sep 17 00:00:00 2001 > Message-Id: <996ee351112bb070d511636bc702496bc445f047.1536237903.git.kshcherbatov@tarantool.org> > In-Reply-To: > References: > From: Kirill Shcherbatov > Date: Thu, 9 Aug 2018 15:02:44 +0300 > Subject: [PATCH 2/4] box: introduce slot_cache in key_part > > Same key_part could be used in different formats multiple > times, so different field->offset_slot would be allocated. > In most scenarios we work with series of tuples of same > format, and (in general) format lookup for field would be > expensive operation for JSON-paths defined in key_part. > New slot_cache field in key_part structure and epoch-based > mechanism to validate it actuality should be effective > approach to improve performance. > New routine tuple_field_by_part use tuple and key_part > to access field that allows to rework and speedup all > scenarios of access tuple data by index. > This also allows to work with JSON-path key_parts later. > > Part of #1012. > --- > src/box/alter.cc | 6 +- > src/box/blackhole.c | 3 +- > src/box/engine.h | 11 ++-- > src/box/key_def.c | 16 +++-- > src/box/key_def.h | 38 +++++++----- > src/box/memtx_bitset.c | 5 +- > src/box/memtx_engine.c | 4 +- > src/box/memtx_hash.h | 4 +- > src/box/memtx_rtree.c | 3 +- > src/box/memtx_space.c | 3 +- > src/box/memtx_space.h | 2 +- > src/box/schema.cc | 4 +- > src/box/space.c | 4 +- > src/box/space.h | 8 ++- > src/box/sysview.c | 3 +- > src/box/tuple.h | 14 +++++ > src/box/tuple_bloom.c | 8 +-- > src/box/tuple_bloom.h | 8 +-- > src/box/tuple_compare.cc | 141 ++++++++++++++++++++++++++----------------- > src/box/tuple_compare.h | 5 +- > src/box/tuple_extract_key.cc | 53 +++++++++------- > src/box/tuple_format.c | 12 ++++ > src/box/tuple_format.h | 18 ++++++ > src/box/tuple_hash.cc | 63 ++++++++++++------- > src/box/tuple_hash.h | 9 ++- > src/box/vinyl.c | 3 +- > src/box/vy_history.c | 2 +- > src/box/vy_history.h | 2 +- > src/box/vy_lsm.c | 2 + > src/box/vy_mem.c | 8 +-- > src/box/vy_mem.h | 10 +-- > src/box/vy_range.c | 2 +- > src/box/vy_range.h | 4 +- > src/box/vy_run.c | 39 ++++++------ > src/box/vy_run.h | 34 +++++------ > src/box/vy_stmt.c | 15 ++--- > src/box/vy_stmt.h | 39 ++++++------ > src/box/vy_upsert.c | 2 +- > src/box/vy_upsert.h | 2 +- > src/box/vy_write_iterator.c | 8 +-- > src/box/vy_write_iterator.h | 6 +- > 41 files changed, 374 insertions(+), 249 deletions(-)