[tarantool-patches] Re: [PATCH v3 2/4] box: introduce slot_cache in key_part

[Vladimir Davydov]

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 at tarantool.org>
> In-Reply-To: <cover.1536237903.git.kshcherbatov at tarantool.org>
> References: <cover.1536237903.git.kshcherbatov at tarantool.org>
> From: Kirill Shcherbatov <kshcherbatov at tarantool.org>
> 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(-)