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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the fixes!

See 11 comments below, a commit on the branch
and a diff at the bottom this email.

> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index a6299a1..a46b886 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -1032,6 +1032,42 @@ ModifySpace::~ModifySpace()
>   		space_def_delete(new_def);
>   }
>   
> +class ModifySpaceFormat: public AlterSpaceOp
> +{
> +public:
> +    ModifySpaceFormat(struct alter_space *alter) : AlterSpaceOp(alter) {}
> +    virtual void alter(struct alter_space *alter);
> +};

1. Comments.

> +
> +void
> +ModifySpaceFormat:: alter(struct alter_space * alter)

2. Redundant white spaces.

(fixed by me)

> +{
> +	struct tuple_format *format = alter->new_space != NULL ?
> +				      alter->new_space->format : NULL;

3. How new_space can be NULL? alter() called only after new_space
is created.

(fixed by me)

> +	if (format == NULL)
> +		return;
> +	struct rlist *key_list = &alter->key_list;
> +	bool is_format_epoch_changed = false;
> +	struct index_def *index_def;
> +	rlist_foreach_entry(index_def, key_list, link) {
> +		struct key_part *part = index_def->key_def->parts;
> +		struct key_part *parts_end =
> +			part + index_def->key_def->part_count;
> +		for (; part < parts_end; part++) {
> +			struct tuple_field *field =
> +				&format->fields[part->fieldno];
> +			if (field->offset_slot != part->offset_slot)
> +				is_format_epoch_changed = true;

4. Makes no sense to continue the cycle from this
moment.

(fixed by me)

> +		}
> +	}
> +	format->epoch = alter->old_space != NULL &&
> +			alter->old_space->format != NULL ?
> +			alter->old_space->format->epoch : 0;

5. How old_space can be NULL here? It is stored in struct alter
right in its constructor. What is more, how old_space->format
can be NULL, if a new space format is not NULL? And you do not
need is_format_epoch_changed in such a case.

(fixed by me)

> +	if (is_format_epoch_changed)
> +		format->epoch++;
> +}
> +
> +
>   /** DropIndex - remove an index from space. */
>   
>   class DropIndex: public AlterSpaceOp
> @@ -1316,6 +1352,7 @@ RebuildIndex::prepare(struct alter_space *alter)
>   	/* Get the new index and build it.  */
>   	new_index = space_index(alter->new_space, new_index_def->iid);
>   	assert(new_index != NULL);
> +	assert(alter->new_space != NULL && alter->old_space != NULL);
>   	space_build_index_xc(alter->old_space, new_index,

6. Garbage diff.

(fixed by me)

>   			     alter->new_space->format);
>   }
> diff --git a/src/box/key_def.h b/src/box/key_def.h
> index aecbe03..a32c34c 100644
> --- a/src/box/key_def.h
> +++ b/src/box/key_def.h
> @@ -74,6 +74,17 @@ struct key_part {
>   	struct coll *coll;
>   	/** True if a part can store NULLs. */
>   	bool is_nullable;
> +	/**
> +	 * Epoch of offset slot cache. Initialized with
> +	 * incremental epoch of format on caching it's field's
> +	 * offset_slot via tuple_field_by_part_raw to speed up
> +	 * access on subsequent calls with same format.
> +	 * Cache is expected to use "the eldest format is most

7. Why 'the eldest'? As I remember, we decided to prefer newer
format offset slots.

> +	 * relevant" strategy.
> +	 */
> +	uint64_t offset_slot_epoch;
> +	/** Cache with format's field offset slot. */
> +	int32_t offset_slot;
>   };
>   
>   struct key_def;
> diff --git a/src/box/memtx_bitset.c b/src/box/memtx_bitset.c
> index a665f1a..9529618 100644
> --- a/src/box/memtx_bitset.c
> +++ b/src/box/memtx_bitset.c
> @@ -283,8 +283,12 @@ memtx_bitset_index_replace(struct index *base, struct tuple *old_tuple,
>   	}
>   
>   	if (new_tuple != NULL) {
> -		const char *field;
> -		field = tuple_field(new_tuple, base->def->key_def->parts[0].fieldno);
> +		const char *field =
> +			tuple_field_by_part_raw(tuple_format(new_tuple),
> +						tuple_data(new_tuple),
> +						tuple_field_map(new_tuple),
> +						(struct key_part *)
> +						base->def->key_def->parts);

8. Looks like you ignored my comment about why do you
case struct key_part * to self. Let me fix it for you
where possible. Where it is not possible, please,
remove 'const struct key_def' qualifier from the function
declaration and do not cast too.

Also, as you could see, when a tuple_field function is
introduced, it has two versions: raw and not raw. Not raw
functions are used exactly for such hunks, when you do
not need to get multiple fields.

(partially fixed by me)

>   		uint32_t key_len;
>   		const void *key = make_key(field, &key_len);
>   #ifndef OLD_GOOD_BITSET
> diff --git a/src/box/tuple_compare.cc b/src/box/tuple_compare.cc
> index e53afba..5d7df4d 100644
> --- a/src/box/tuple_compare.cc
> +++ b/src/box/tuple_compare.cc
> @@ -449,7 +459,7 @@ tuple_common_key_parts(const struct tuple *tuple_a,
>   	return i;
>   }
>   
> -template<bool is_nullable, bool has_optional_parts>
> +template<bool is_nullable, bool has_optional_parts, bool has_json_path>

9. You said, that has_json_path is substituted with is_flat, but looks
like you did not. Please, fix all the remarks of the previous review
more accurate.

>   static inline int
>   tuple_compare_slowpath(const struct tuple *tuple_a, const struct tuple *tuple_b,
>   		       const struct key_def *key_def)
> diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
> index b385c0d..2d4a85f 100644
> --- a/src/box/tuple_format.c
> +++ b/src/box/tuple_format.c
> @@ -232,6 +232,11 @@ tuple_format_alloc(struct key_def * const *keys, uint16_t key_count,
>   		format->dict = dict;
>   		tuple_dictionary_ref(dict);
>   	}
> +	/*
> +	 * Set invalid epoch that should be changed later on
> +	 * attaching to space.
> +	 */
> +	format->epoch = 1;

10. Why is this epoch invalid? As I understand, a format can live
with it normally but comparators will be slow.

>   	format->refs = 0;
>   	format->id = FORMAT_ID_NIL;
>   	format->field_count = field_count;
> diff --git a/src/box/vy_stmt.h b/src/box/vy_stmt.h
> index 273d5e8..c4885c0 100644
> --- a/src/box/vy_stmt.h
> +++ b/src/box/vy_stmt.h
> @@ -719,7 +719,12 @@ static inline bool
>   vy_tuple_key_contains_null(const struct tuple *tuple, const struct key_def *def)
>   {
>   	for (uint32_t i = 0; i < def->part_count; ++i) {
> -		const char *field = tuple_field(tuple, def->parts[i].fieldno);
> +		const char *field =
> +			tuple_field_by_part_raw(tuple_format(tuple),
> +						tuple_data(tuple),
> +						tuple_field_map(tuple),
> +						(struct key_part *)
> +						&def->parts[i]);

11. The same as in the previous review: do not call tuple_format()
and other tuple functions multiple times when possible.

(fixed by me)

>   		if (field == NULL || mp_typeof(*field) == MP_NIL)
>   			return true;
>   	}
> 

Diff of my fixes here and on the branch:

commit 2bb228dafc7b8b1f69e1ed077c01861ea50e6ec9
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Thu Aug 30 16:32:28 2018 -0300

     Review fixes

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 0833635f6..3267b978a 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1036,18 +1036,19 @@ class ModifySpaceFormat: public AlterSpaceOp
  {
  public:
      ModifySpaceFormat(struct alter_space *alter) : AlterSpaceOp(alter) {}
-    virtual void alter(struct alter_space *alter);
+
+    virtual void
+    alter(struct alter_space *alter);
  };
  
  void
-ModifySpaceFormat:: alter(struct alter_space * alter)
+ModifySpaceFormat::alter(struct alter_space *alter)
  {
-	struct tuple_format *format = alter->new_space != NULL ?
-				      alter->new_space->format : NULL;
+	struct tuple_format *format = alter->new_space->format;
  	if (format == NULL)
  		return;
+	format->epoch = alter->old_space->format->epoch;
  	struct rlist *key_list = &alter->key_list;
-	bool is_format_epoch_changed = false;
  	struct index_def *index_def;
  	rlist_foreach_entry(index_def, key_list, link) {
  		struct key_part *part = index_def->key_def->parts;
@@ -1056,15 +1057,12 @@ ModifySpaceFormat:: alter(struct alter_space * alter)
  		for (; part < parts_end; part++) {
  			struct tuple_field *field =
  				&format->fields[part->fieldno];
-			if (field->offset_slot != part->offset_slot)
-				is_format_epoch_changed = true;
+			if (field->offset_slot != part->offset_slot) {
+				++format->epoch;
+				return;
+			}
  		}
  	}
-	format->epoch = alter->old_space != NULL &&
-			alter->old_space->format != NULL ?
-			alter->old_space->format->epoch : 1;
-	if (is_format_epoch_changed)
-		format->epoch++;
  }
  
  
@@ -1352,7 +1350,6 @@ RebuildIndex::prepare(struct alter_space *alter)
  	/* Get the new index and build it.  */
  	new_index = space_index(alter->new_space, new_index_def->iid);
  	assert(new_index != NULL);
-	assert(alter->new_space != NULL && alter->old_space != NULL);
  	space_build_index_xc(alter->old_space, new_index,
  			     alter->new_space->format);
  }
diff --git a/src/box/memtx_bitset.c b/src/box/memtx_bitset.c
index 9529618d2..cd7362ee1 100644
--- a/src/box/memtx_bitset.c
+++ b/src/box/memtx_bitset.c
@@ -284,11 +284,8 @@ memtx_bitset_index_replace(struct index *base, struct tuple *old_tuple,
  
  	if (new_tuple != NULL) {
  		const char *field =
-			tuple_field_by_part_raw(tuple_format(new_tuple),
-						tuple_data(new_tuple),
-						tuple_field_map(new_tuple),
-						(struct key_part *)
-						base->def->key_def->parts);
+			tuple_field_by_part(new_tuple,
+					    base->def->key_def->parts);
  		uint32_t key_len;
  		const void *key = make_key(field, &key_len);
  #ifndef OLD_GOOD_BITSET
diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c
index 00aaf79d4..f2aa6c3e5 100644
--- a/src/box/memtx_rtree.c
+++ b/src/box/memtx_rtree.c
@@ -112,11 +112,8 @@ extract_rectangle(struct rtree_rect *rect, const struct tuple *tuple,
  		  struct index_def *index_def)
  {
  	assert(index_def->key_def->part_count == 1);
-	const char *elems =
-		tuple_field_by_part_raw(tuple_format(tuple), tuple_data(tuple),
-					tuple_field_map(tuple),
-					(struct key_part *)
-					index_def->key_def->parts);
+	const char *elems = tuple_field_by_part(tuple,
+						index_def->key_def->parts);
  	unsigned dimension = index_def->opts.dimension;
  	uint32_t count = mp_decode_array(&elems);
  	return mp_decode_rect(rect, dimension, elems, count, "Field");
diff --git a/src/box/tuple.h b/src/box/tuple.h
index 2e84516de..b638f5086 100644
--- a/src/box/tuple.h
+++ b/src/box/tuple.h
@@ -43,6 +43,7 @@ extern "C" {
  
  struct slab_arena;
  struct quota;
+struct key_part;
  
  /**
   * A format for standalone tuples allocated on runtime arena.
@@ -521,6 +522,19 @@ tuple_field(const struct tuple *tuple, uint32_t fieldno)
  			       tuple_field_map(tuple), fieldno);
  }
  
+/**
+ * Get a field refereed by index @part in tuple.
+ * @param tuple Tuple to get the field from.
+ * @param part Index part to use.
+ * @retval Field data if the field exists or NULL.
+ */
+static inline const char *
+tuple_field_by_part(const struct tuple *tuple, struct key_part *part)
+{
+	return tuple_field_by_part_raw(tuple_format(tuple), tuple_data(tuple),
+				       tuple_field_map(tuple), part);
+}
+
  /**
   * Get tuple field by its JSON path.
   * @param tuple Tuple to get field from.
diff --git a/src/box/tuple_format.h b/src/box/tuple_format.h
index ecbf64c24..9406d5bbc 100644
--- a/src/box/tuple_format.h
+++ b/src/box/tuple_format.h
@@ -330,12 +330,12 @@ tuple_init_field_map(const struct tuple_format *format, uint32_t *field_map,
  		     const char *tuple);
  
  /**
- * Get a field refereed by multipart index @part in tuple.
+ * Get a field refereed by index @part in tuple.
   * @param format Tuple format.
   * @param tuple A pointer to MessagePack array.
   * @param field_map A pointer to the LAST element of field map.
- * @param part Multipart index part to use.
- * @retval Field data if field exists or NULL.
+ * @param part Index part to use.
+ * @retval Field data if the field exists or NULL.
   */
  const char *
  tuple_field_by_part_raw(const struct tuple_format *format, const char *data,
diff --git a/src/box/tuple_hash.cc b/src/box/tuple_hash.cc
index 561ca550a..c9a16ee31 100644
--- a/src/box/tuple_hash.cc
+++ b/src/box/tuple_hash.cc
@@ -158,11 +158,8 @@ struct TupleHash
  		uint32_t carry = 0;
  		uint32_t total_size = 0;
  		const char *field =
-			tuple_field_by_part_raw(tuple_format(tuple),
-						tuple_data(tuple),
-						tuple_field_map(tuple),
-						(struct key_part *)
-						key_def->parts);
+			tuple_field_by_part(tuple,
+					    (struct key_part *) key_def->parts);
  		TupleFieldHash<TYPE, MORE_TYPES...>::
  			hash(&field, &h, &carry, &total_size);
  		return PMurHash32_Result(h, carry, total_size);
@@ -175,11 +172,8 @@ struct TupleHash<FIELD_TYPE_UNSIGNED> {
  			     const struct key_def *key_def)
  	{
  		const char *field =
-			tuple_field_by_part_raw(tuple_format(tuple),
-						tuple_data(tuple),
-						tuple_field_map(tuple),
-						(struct key_part *)
-						key_def->parts);
+			tuple_field_by_part(tuple,
+					    (struct key_part *) key_def->parts);
  		uint64_t val = mp_decode_uint(&field);
  		if (likely(val <= UINT32_MAX))
  			return val;
@@ -322,10 +316,8 @@ tuple_hash_key_part(uint32_t *ph1, uint32_t *pcarry,
  		    const struct tuple *tuple,
  		    const struct key_part *part)
  {
-	const char *field =
-		tuple_field_by_part_raw(tuple_format(tuple), tuple_data(tuple),
-					tuple_field_map(tuple),
-					(struct key_part *)part);
+	const char *field = tuple_field_by_part(tuple,
+						(struct key_part *) part);
  	if (field == NULL)
  		return tuple_hash_null(ph1, pcarry);
  	return tuple_hash_field(ph1, pcarry, &field, part->coll);
diff --git a/src/box/vy_stmt.h b/src/box/vy_stmt.h
index c4885c09b..4f8cbbdd3 100644
--- a/src/box/vy_stmt.h
+++ b/src/box/vy_stmt.h
@@ -716,15 +716,15 @@ vy_tuple_format_new_with_colmask(struct tuple_format *mem_format);
   * @retval Does the key contain NULL or not?
   */
  static inline bool
-vy_tuple_key_contains_null(const struct tuple *tuple, const struct key_def *def)
+vy_tuple_key_contains_null(const struct tuple *tuple, struct key_def *def)
  {
-	for (uint32_t i = 0; i < def->part_count; ++i) {
+	struct tuple_format *format = tuple_format(tuple);
+	const char *data = tuple_data(tuple);
+	const uint32_t *field_map = tuple_field_map(tuple);
+	for (struct key_part *part = def->parts, *end = part + def->part_count;
+	     part < end; ++part) {
  		const char *field =
-			tuple_field_by_part_raw(tuple_format(tuple),
-						tuple_data(tuple),
-						tuple_field_map(tuple),
-						(struct key_part *)
-						&def->parts[i]);
+			tuple_field_by_part_raw(format, data, field_map, part);
  		if (field == NULL || mp_typeof(*field) == MP_NIL)
  			return true;
  	}