From: Kirill Yukhin <kyukhin@tarantool.org>
To: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: tarantool-patches@freelists.org
Subject: Re: [PATCH v2 4/4] Allow to reuse tuple_formats for ephemeral spaces
Date: Fri, 25 Jan 2019 09:05:18 +0300 [thread overview]
Message-ID: <20190125060518.kfnj4ju3azgmeg5s@tarantool.org> (raw)
In-Reply-To: <20190124174904.whc63hewbxkjhyjn@esperanza>
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());
next prev parent reply other threads:[~2019-01-25 6:05 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-24 12:48 [PATCH v2 0/4] Reuse tuple formats " Kirill Yukhin
2019-01-24 12:48 ` [PATCH v2 1/4] Pass necessary fields to tuple_format contructor Kirill Yukhin
2019-01-24 17:26 ` Vladimir Davydov
2019-01-24 12:48 ` [PATCH v2 2/4] Set is_temporary flag for formats of ephemeral spaces Kirill Yukhin
2019-01-24 17:26 ` Vladimir Davydov
2019-01-24 12:48 ` [PATCH v2 3/4] sql: set error type in case of ephemral space creation failure Kirill Yukhin
2019-01-24 17:26 ` Vladimir Davydov
2019-01-24 12:48 ` [PATCH v2 4/4] Allow to reuse tuple_formats for ephemeral spaces Kirill Yukhin
2019-01-24 17:49 ` Vladimir Davydov
2019-01-25 6:05 ` Kirill Yukhin [this message]
2019-01-25 9:01 ` Vladimir Davydov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190125060518.kfnj4ju3azgmeg5s@tarantool.org \
--to=kyukhin@tarantool.org \
--cc=tarantool-patches@freelists.org \
--cc=vdavydov.dev@gmail.com \
--subject='Re: [PATCH v2 4/4] Allow to reuse tuple_formats for ephemeral spaces' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox