From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 62749290DE for ; Mon, 3 Sep 2018 06:35:07 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Ep0Hl-Uyt7Sh for ; Mon, 3 Sep 2018 06:35:07 -0400 (EDT) Received: from smtp18.mail.ru (smtp18.mail.ru [94.100.176.155]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id DEF09290D1 for ; Mon, 3 Sep 2018 06:35:06 -0400 (EDT) From: Vladislav Shpilevoy Subject: [tarantool-patches] Re: [PATCH v3 2/4] box: introduce slot_cache in key_part References: <9be616bdd2190a0ab38aaee981098811f542eeb1.1535354849.git.kshcherbatov@tarantool.org> Message-ID: <94ec4d86-f611-b1c8-9665-ababdf21481e@tarantool.org> Date: Mon, 3 Sep 2018 13:35:04 +0300 MIME-Version: 1.0 In-Reply-To: <9be616bdd2190a0ab38aaee981098811f542eeb1.1535354849.git.kshcherbatov@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, Kirill Shcherbatov 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 > +template 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 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:: hash(&field, &h, &carry, &total_size); return PMurHash32_Result(h, carry, total_size); @@ -175,11 +172,8 @@ struct TupleHash { 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; }