[tarantool-patches] Re: [PATCH v3 2/4] box: introduce slot_cache in key_part
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Sep 3 13:35:04 MSK 2018
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 at 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;
}
More information about the Tarantool-patches
mailing list