Re: [PATCH v5 8/9] box: introduce offset slot cache in key_part

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Mon, Nov 26, 2018 at 01:49:42PM +0300, Kirill Shcherbatov wrote:
> Tuned tuple_field_by_part_raw routine with key_part's
> offset_slot_cache. Introduced tuple_format epoch to test validity
> of this cache. The key_part caches last offset_slot source
> format to make epoch comparison because same space may have
> multiple format of same epoch that have different key_parts and
> related offset_slots distribution.
> 
> Part of #1012

If I didn't know what this patch is about, I'd be puzzled after reading
this comment. I'd put it like that:

  tuple_field_by_part looks up the tuple_field corresponding to the
  given key part in tuple_format in order to quickly retrieve the offset
  of indexed data from the tuple field map. For regular indexes this
  operation is blazing fast, however of JSON indexes it is not as we
  have to parse the path to data and then do multiple lookups in a JSON
  tree. Since tuple_field_by_part is used by comparators, we should
  strive to make this routine as fast as possible for all kinds of
  indexes.

  This patch introduces an optimization that is supposed to make
  tuple_field_by_part for JSON indexes as fast as it is for regular
  indexes in most cases. We do that by caching the offset slot right in
  key_part. There's a catch here however - we create a new format
  whenever an index is dropped or created and we don't reindex old
  tuples. As a result, there may be several generations of tuples in the
  same space, all using different formats while there's the only key_def
  used for comparison.

  To overcome this problem, we introduce the notion of tuple_format
  epoch. This is a counter incremented each time a new format is
  created. We store it in tuple_format and key_def, and we only use
  the offset slot cached in a key_def if it's epoch coincides with the
  epoch of the tuple format. If they don't, we look up a tuple_field as
  before, and then update the cached value provided the epoch of the
  tuple format is newer than the key_def's.

> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 029da02..6291159 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -856,7 +856,10 @@ alter_space_do(struct txn *txn, struct alter_space *alter)
>  	 * Create a new (empty) space for the new definition.
>  	 * Sic: the triggers are not moved over yet.
>  	 */
> -	alter->new_space = space_new_xc(alter->space_def, &alter->key_list);
> +	alter->new_space =
> +		space_new_xc(alter->space_def, &alter->key_list,
> +			     alter->old_space->format != NULL ?
> +			     alter->old_space->format->epoch + 1 : 1);

Passing the epoch around looks ugly. Let's introduce a global counter
and bump it right in tuple_format_new(). If you worry about disk_format
and mem_format having different epochs in vinyl, it's not really a
problem: make epoch an optional argument to tuple_format_new(); if the
caller passes an epoch, use it, otherwise generate a new one; in vinyl
reuse mem_format->epoch for disk_format.

> diff --git a/src/box/key_def.h b/src/box/key_def.h
> index 7731e48..3e08eb4 100644
> --- a/src/box/key_def.h
> +++ b/src/box/key_def.h
> @@ -95,6 +95,14 @@ struct key_part {
>  	char *path;
>  	/** The length of JSON path. */
>  	uint32_t path_len;
> +	/**
> +	 * Source format for offset_slot_cache hit validations.
> +	 * Cache is expected to use "the format with the newest
> +	 * epoch is most relevant" strategy.
> +	 */
> +	struct tuple_format *format_cache;

Why did you decide to store tuple_format in key_part instead of epoch,
as you did originally? I liked that more.

> +	/** Cache with format's field offset slot. */

	/**
	 * Cached value of the offset slot corresponding to
	 * the indexed field (tuple_field::offset_slot).
	 * Valid only if key_def::epoch equals the epoch of
	 * the tuple format. Updated in tuple_field_by_part
	 * to always store the offset corresponding to the
	 * most recent tuple format (the one with the greatest
	 * epoch value).
	 */

> +	int32_t offset_slot_cache;

> diff --git a/src/box/tuple_format.h b/src/box/tuple_format.h
> index 860f052..8a7ebfa 100644
> --- a/src/box/tuple_format.h
> +++ b/src/box/tuple_format.h
> @@ -137,6 +137,8 @@ tuple_field_is_nullable(const struct tuple_field *tuple_field)
>   * Tuple format describes how tuple is stored and information about its fields
>   */
>  struct tuple_format {
> +	/** Counter that grows incrementally on space rebuild. */

... used for caching offset slot in key_part, for more details
see key_part::offset_slot_cache

> +	uint64_t epoch;