From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Fri, 25 Jan 2019 09:05:18 +0300 From: Kirill Yukhin Subject: Re: [PATCH v2 4/4] Allow to reuse tuple_formats for ephemeral spaces Message-ID: <20190125060518.kfnj4ju3azgmeg5s@tarantool.org> References: <20190124174904.whc63hewbxkjhyjn@esperanza> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20190124174904.whc63hewbxkjhyjn@esperanza> To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: Hello Vladimir, Thanks for review! My answers are inlined, iterative patch is in the bottom. -- Regards, Kirill Yukhin On 24 Jan 20:49, Vladimir Davydov wrote: > On Thu, Jan 24, 2019 at 03:48:48PM +0300, Kirill Yukhin wrote: > > diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c > > index 4aad8da..cbc5091 100644 > > --- a/src/box/memtx_space.c > > +++ b/src/box/memtx_space.c > > @@ -993,7 +993,7 @@ memtx_space_new(struct memtx_engine *memtx, > > tuple_format_new(&memtx_tuple_format_vtab, memtx, keys, key_count, > > def->fields, def->field_count, > > def->exact_field_count, def->dict, > > - def->opts.is_temporary); > > + def->opts.is_temporary, def->opts.is_temporary); > > is_temporary, is_temporary WTF? Fixed. > > diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c > > index 559c601..1e8675c 100644 > > --- a/src/box/tuple_format.c > > +++ b/src/box/tuple_format.c > > @@ -380,6 +387,69 @@ error: > > return NULL; > > } > > > > +static int > > +tuple_format_cmp(const struct tuple_format *a, const struct tuple_format *b) > > +{ > > + if (a->exact_field_count != b->exact_field_count) > > + return a->exact_field_count - b->exact_field_count; > > + if (tuple_format_field_count(a) != tuple_format_field_count(b)) > > + return tuple_format_field_count(a) - tuple_format_field_count(b); > > + > > + for (uint32_t i = 0; i < tuple_format_field_count((struct tuple_format *)a); ++i) { > ^^^^^^^^^^^^^^^^^^^^^^^^ > You forgot to drop this type conversion. Removed. > > +struct mh_tuple_format_t *tuple_formats_hash = NULL; > > + > > /** Free tuple format resources, doesn't unregister. */ > > static inline void > > tuple_format_destroy(struct tuple_format *format) > > @@ -392,17 +462,82 @@ tuple_format_destroy(struct tuple_format *format) > > void > > tuple_format_delete(struct tuple_format *format) > > { > > + mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format, NULL); > > + if (key != mh_end(tuple_formats_hash)) > > + mh_tuple_format_del(tuple_formats_hash, key, NULL); > > Please factor this out to a separate function complimentary to > tuple_format_add_to_hash (tuple_format_remove_from_hash?). Done. > > +static bool > > +tuple_format_reuse(struct tuple_format **p_format) > > +{ > > + struct tuple_format *format = *p_format; > > + if (!format->is_ephemeral) > > + return false; > > + /* > > + * These fields do not participate in hashing. > > + * Make sure they're unset. > > + */ > > + assert(format->dict->name_count == 0); > > + assert(format->is_temporary); > > + format->hash = tuple_format_hash(format); > > The fact that in tuple_format_add_to_hash you implicitly rely on > format->hash having been initialized here looks confusing. Let's > initialize hash for all formats. I moved assignement to tuple_format_new(). > > +static int > > +tuple_format_add_to_hash(struct tuple_format *format) > > +{ > > + if(!format->is_ephemeral) > > + return 0; > > + assert(format->dict->name_count == 0); > > + assert(format->is_temporary); > > + mh_int_t key = mh_tuple_format_put(tuple_formats_hash, > > + (const struct tuple_format **)&format, > > + NULL, NULL); > > + if (key == mh_end(tuple_formats_hash)) > > You forgot to set diag (say hi to #3534 again ;-) Fixed. > > + return -1; > > + else > > + return 0; > > +} > > + > > struct tuple_format * > > tuple_format_new(struct tuple_format_vtab *vtab, void *engine, > > struct key_def * const *keys, uint16_t key_count, > > const struct field_def *space_fields, > > uint32_t space_field_count, uint32_t exact_field_count, > > - struct tuple_dictionary *dict, bool is_temporary) > > + struct tuple_dictionary *dict, bool is_temporary, > > + bool is_ephemeral) > > { > > struct tuple_format *format = > > tuple_format_alloc(keys, key_count, space_field_count, dict); > > diff --git a/src/box/tuple_format.h b/src/box/tuple_format.h > > index d15fef2..3fc3a23 100644 > > --- a/src/box/tuple_format.h > > +++ b/src/box/tuple_format.h > > @@ -459,6 +470,13 @@ tuple_field_by_part_raw(struct tuple_format *format, const char *data, > > return tuple_field_raw(format, data, field_map, part->fieldno); > > } > > > > +/** > > + * Initialize tuple format subsystem. > > + * @retval 0 on success, -1 otherwise. > > + */ > > +int > > +tuple_format_init(); > > + > > tuple_format_init is declared in the very end of the file while > tuple_format_free in the very beginning. Urghhh. Please fix. I moved tuple format init before free. > > diff --git a/test/unit/tuple_bigref.c b/test/unit/tuple_bigref.c > > index 20eab61..76a2e98 100644 > > --- a/test/unit/tuple_bigref.c > > +++ b/test/unit/tuple_bigref.c > > @@ -143,6 +143,7 @@ main() > > > > memory_init(); > > fiber_init(fiber_c_invoke); > > + tuple_format_init(); > > This is redundant. Remove please. Removed. > > diff --git a/test/unit/vy_iterators_helper.c b/test/unit/vy_iterators_helper.c > > index 6808ff4..395ad15 100644 > > --- a/test/unit/vy_iterators_helper.c > > +++ b/test/unit/vy_iterators_helper.c > > @@ -17,12 +17,13 @@ vy_iterator_C_test_init(size_t cache_size) > > say_set_log_level(S_WARN); > > > > memory_init(); > > + tuple_format_init(); > > Ditto. Ditto. -- Regards, Kirill Yukhin diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c index cbc5091..5f8cc98 100644 --- a/src/box/memtx_space.c +++ b/src/box/memtx_space.c @@ -993,7 +993,7 @@ memtx_space_new(struct memtx_engine *memtx, tuple_format_new(&memtx_tuple_format_vtab, memtx, keys, key_count, def->fields, def->field_count, def->exact_field_count, def->dict, - def->opts.is_temporary, def->opts.is_temporary); + def->opts.is_temporary, def->opts.is_ephemeral); if (format == NULL) { free(memtx_space); return NULL; diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c index 1e8675c..305834e 100644 --- a/src/box/tuple_format.c +++ b/src/box/tuple_format.c @@ -395,7 +395,7 @@ tuple_format_cmp(const struct tuple_format *a, const struct tuple_format *b) if (tuple_format_field_count(a) != tuple_format_field_count(b)) return tuple_format_field_count(a) - tuple_format_field_count(b); - for (uint32_t i = 0; i < tuple_format_field_count((struct tuple_format *)a); ++i) { + for (uint32_t i = 0; i < tuple_format_field_count(a); ++i) { struct tuple_field *field_a = tuple_format_field( (struct tuple_format *)a, i); struct tuple_field *field_b = tuple_format_field( @@ -459,12 +459,18 @@ tuple_format_destroy(struct tuple_format *format) tuple_dictionary_unref(format->dict); } -void -tuple_format_delete(struct tuple_format *format) +static void +tuple_format_remove_from_hash(struct tuple_format *format) { mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format, NULL); if (key != mh_end(tuple_formats_hash)) mh_tuple_format_del(tuple_formats_hash, key, NULL); +} + +void +tuple_format_delete(struct tuple_format *format) +{ + tuple_format_remove_from_hash(format); tuple_format_deregister(format); tuple_format_destroy(format); free(format); @@ -495,7 +501,6 @@ tuple_format_reuse(struct tuple_format **p_format) */ assert(format->dict->name_count == 0); assert(format->is_temporary); - format->hash = tuple_format_hash(format); mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format, NULL); if (key != mh_end(tuple_formats_hash)) { @@ -525,10 +530,13 @@ tuple_format_add_to_hash(struct tuple_format *format) mh_int_t key = mh_tuple_format_put(tuple_formats_hash, (const struct tuple_format **)&format, NULL, NULL); - if (key == mh_end(tuple_formats_hash)) + if (key == mh_end(tuple_formats_hash)) { + diag_set(OutOfMemory, 0, "tuple_format_add_to_hash", + "tuple formats hash entry"); return -1; - else + } else { return 0; + } } struct tuple_format * @@ -551,6 +559,7 @@ tuple_format_new(struct tuple_format_vtab *vtab, void *engine, if (tuple_format_create(format, keys, key_count, space_fields, space_field_count) < 0) goto err; + format->hash = tuple_format_hash(format); if (tuple_format_reuse(&format)) return format; if (tuple_format_register(format) < 0) @@ -747,6 +756,18 @@ tuple_format_min_field_count(struct key_def * const *keys, uint16_t key_count, return min_field_count; } +int +tuple_format_init() +{ + tuple_formats_hash = mh_tuple_format_new(); + if (tuple_formats_hash == NULL) { + diag_set(OutOfMemory, sizeof(struct mh_tuple_format_t), "malloc", + "tuple format hash"); + return -1; + } + return 0; +} + /** Destroy tuple format subsystem and free resourses */ void tuple_format_free() @@ -964,15 +985,3 @@ error: tt_sprintf("error in path on position %d", rc)); return -1; } - -int -tuple_format_init() -{ - tuple_formats_hash = mh_tuple_format_new(); - if (tuple_formats_hash == NULL) { - diag_set(OutOfMemory, sizeof(struct mh_tuple_format_t), "malloc", - "tuple format hash"); - return -1; - } - return 0; -} diff --git a/test/unit/tuple_bigref.c b/test/unit/tuple_bigref.c index 76a2e98..20eab61 100644 --- a/test/unit/tuple_bigref.c +++ b/test/unit/tuple_bigref.c @@ -143,7 +143,6 @@ main() memory_init(); fiber_init(fiber_c_invoke); - tuple_format_init(); tuple_init(NULL); tuple_end = mp_encode_array(tuple_end, 1); diff --git a/test/unit/vy_iterators_helper.c b/test/unit/vy_iterators_helper.c index 395ad15..55d8504 100644 --- a/test/unit/vy_iterators_helper.c +++ b/test/unit/vy_iterators_helper.c @@ -17,7 +17,6 @@ vy_iterator_C_test_init(size_t cache_size) say_set_log_level(S_WARN); memory_init(); - tuple_format_init(); fiber_init(fiber_c_invoke); tuple_init(NULL); vy_cache_env_create(&cache_env, cord_slab_cache());