From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 23 Jan 2019 11:38:59 +0300 From: Vladimir Davydov Subject: Re: [PATCH 4/4] Allow to reuse tuple_formats for ephemeral spaces Message-ID: <20190123083859.jcs43edwinmd4asr@esperanza> References: <29b9663f6447dc56f6dcc5878b215cfb4863b3c2.1548165435.git.kyukhin@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <29b9663f6447dc56f6dcc5878b215cfb4863b3c2.1548165435.git.kyukhin@tarantool.org> To: Kirill Yukhin Cc: tarantool-patches@freelists.org List-ID: On Tue, Jan 22, 2019 at 05:07:20PM +0300, Kirill Yukhin wrote: > diff --git a/src/box/blackhole.c b/src/box/blackhole.c > index 64e24cd..16958a2 100644 > --- a/src/box/blackhole.c > +++ b/src/box/blackhole.c > @@ -157,7 +157,7 @@ blackhole_engine_create_space(struct engine *engine, struct space_def *def, > struct tuple_format *format; > format = tuple_format_new(&tuple_format_runtime->vtab, NULL, NULL, 0, > def->fields, def->field_count, def->dict, > - false, def->exact_field_count); > + false, false, def->exact_field_count); > if (format == NULL) { > free(space); > return NULL; > diff --git a/src/box/box.cc b/src/box/box.cc > index 9f2fd6d..3799544 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -46,6 +46,7 @@ > #include > #include "main.h" > #include "tuple.h" > +#include "tuple_format.h" > #include "session.h" > #include "schema.h" > #include "engine.h" > @@ -2050,6 +2051,9 @@ box_init(void) > */ > session_init(); > > + if (tuple_format_init() != 0) > + diag_raise(); > + Please also implement tuple_format_free(). Call it in unit tests. Add it to the commented block in box_free() so that we won't forget it when we fix box shutdown. > if (tuple_init(lua_hash) != 0) > diag_raise(); > > diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c > index 5cf70ab..254ef24 100644 > --- a/src/box/memtx_engine.c > +++ b/src/box/memtx_engine.c > @@ -994,6 +994,12 @@ memtx_engine_gc_f(va_list va) > struct memtx_engine *memtx = va_arg(va, struct memtx_engine *); > while (!fiber_is_cancelled()) { > bool stop; > + struct errinj *delay = errinj(ERRINJ_MEMTX_DELAY_GC, > + ERRINJ_BOOL); > + if (delay != NULL && delay->bparam) { > + while (delay->bparam) > + fiber_sleep(0.001); > + } > memtx_engine_run_gc(memtx, &stop); > if (stop) { > fiber_yield_timeout(TIMEOUT_INFINITY); > diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c > index 49d09af..e6909f9 100644 > --- a/src/box/memtx_space.c > +++ b/src/box/memtx_space.c > @@ -992,7 +992,8 @@ memtx_space_new(struct memtx_engine *memtx, > struct tuple_format *format = > tuple_format_new(&memtx_tuple_format_vtab, memtx, keys, key_count, > def->fields, def->field_count, def->dict, > - def->opts.is_temporary, def->exact_field_count); > + def->opts.is_temporary, def->opts.is_ephemeral, > + def->exact_field_count); > if (format == NULL) { > free(memtx_space); > return NULL; > diff --git a/src/box/space.c b/src/box/space.c > index 4d174f7..316b34b 100644 > --- a/src/box/space.c > +++ b/src/box/space.c > @@ -194,10 +194,11 @@ space_new(struct space_def *def, struct rlist *key_list) > struct space * > space_new_ephemeral(struct space_def *def, struct rlist *key_list) > { > + assert(def->opts.is_temporary); > + assert(def->opts.is_ephemeral); > struct space *space = space_new(def, key_list); > if (space == NULL) > return NULL; > - space->def->opts.is_temporary = true; Should have been done in patch 2. > space->vtab->init_ephemeral_space(space); > return space; > } > diff --git a/src/box/space_def.c b/src/box/space_def.c > index 1d2bfcb..c37df24 100644 > --- a/src/box/space_def.c > +++ b/src/box/space_def.c > @@ -53,6 +53,7 @@ checks_array_decode(const char **str, uint32_t len, char *opt, uint32_t errcode, > const struct space_opts space_opts_default = { > /* .group_id = */ 0, > /* .is_temporary = */ false, > + /* .is_ephemeral = */ false, > /* .view = */ false, > /* .sql = */ NULL, > /* .checks = */ NULL, > @@ -61,6 +62,7 @@ const struct space_opts space_opts_default = { > const struct opt_def space_opts_reg[] = { > OPT_DEF("group_id", OPT_UINT32, struct space_opts, group_id), > OPT_DEF("temporary", OPT_BOOL, struct space_opts, is_temporary), > + OPT_DEF("ephemeral", OPT_BOOL, struct space_opts, is_ephemeral), This isn't needed as "ephemeral" space option is purely ephemeral, meaning it should never be persisted. > OPT_DEF("view", OPT_BOOL, struct space_opts, is_view), > OPT_DEF("sql", OPT_STRPTR, struct space_opts, sql), > OPT_DEF_ARRAY("checks", struct space_opts, checks, > @@ -267,6 +269,7 @@ space_def_new_ephemeral(uint32_t field_count) > &space_opts_default, > &field_def_default, 0); > space_def->opts.is_temporary = true; > + space_def->opts.is_ephemeral = true; > return space_def; > } > > diff --git a/src/box/space_def.h b/src/box/space_def.h > index e8d46c5..ea20b1d 100644 > --- a/src/box/space_def.h > +++ b/src/box/space_def.h > @@ -58,6 +58,11 @@ struct space_opts { > * does not require manual release. > */ > bool is_temporary; > + /** > + * This flag is set if space is ephemeral and hence > + * its format might be re-used. > + */ > + bool is_ephemeral; > /** > * If the space is a view, then it can't feature any > * indexes, and must have SQL statement. Moreover, > diff --git a/src/box/tuple.c b/src/box/tuple.c > index 5295800..765e40b 100644 > --- a/src/box/tuple.c > +++ b/src/box/tuple.c > @@ -208,7 +208,8 @@ tuple_init(field_name_hash_f hash) > * Create a format for runtime tuples > */ > tuple_format_runtime = tuple_format_new(&tuple_format_runtime_vtab, NULL, > - NULL, 0, NULL, 0, NULL, false, 0); > + NULL, 0, NULL, 0, NULL, false, > + false, 0); > if (tuple_format_runtime == NULL) > return -1; > > @@ -380,7 +381,8 @@ box_tuple_format_new(struct key_def **keys, uint16_t key_count) > { > box_tuple_format_t *format = > tuple_format_new(&tuple_format_runtime_vtab, NULL, > - keys, key_count, NULL, 0, NULL, false, 0); > + keys, key_count, NULL, 0, NULL, false, false, > + 0); > if (format != NULL) > tuple_format_ref(format); > return format; > diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c > index 6edc0fb..5d54e94 100644 > --- a/src/box/tuple_format.c > +++ b/src/box/tuple_format.c > @@ -34,6 +34,8 @@ > #include "tuple_format.h" > #include "coll_id_cache.h" > > +#include "third_party/PMurHash.h" > + > /** Global table of tuple formats */ > struct tuple_format **tuple_formats; > static intptr_t recycled_format_ids = FORMAT_ID_NIL; > @@ -276,7 +278,11 @@ tuple_format_register(struct tuple_format *format) > formats_capacity = new_capacity; > tuple_formats = formats; > } > - if (formats_size == FORMAT_ID_MAX + 1) { > + struct errinj *inj = errinj(ERRINJ_TUPLE_FORMAT_COUNT, > + ERRINJ_INT); > + if ((inj != NULL && inj->iparam > 0 > + && formats_size >= inj->iparam + 1) > + || formats_size == FORMAT_ID_MAX + 1) { IMO the code would be easier to follow if you rewrote it as follows: uint32_t formats_size_max = FORMAT_ID_MAX + 1; struct errinj *inj = errinj(ERRINJ_TUPLE_FORMAT_COUNT, ERRINJ_INT); if (inf != NULL && inj->iparam > 0) formats_size = inj->iparam; if (formats_size >= fromats_size_max) { > diag_set(ClientError, ER_TUPLE_FORMAT_LIMIT, > (unsigned) formats_capacity); > return -1; > @@ -380,10 +386,77 @@ 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(a); ++i) { > + struct tuple_field *field_a = tuple_format_field( > + (struct tuple_format *)a, i); Please don't use const for struct tuple_format to avoid these type conversions. > + struct tuple_field *field_b = tuple_format_field( > + (struct tuple_format *)b, i); > + if (field_a->type != field_b->type) > + return (int)field_a->type - (int)field_b->type; > + if (field_a->coll_id != field_b->coll_id) > + return (int)field_a->coll_id - (int)field_b->coll_id; > + if (field_a->nullable_action != field_b->nullable_action) > + return (int)field_a->nullable_action - > + (int)field_b->nullable_action; > + if (field_a->is_key_part != field_b->is_key_part) > + return (int)field_a->is_key_part - > + (int)field_b->is_key_part; > + } > + > + return 0; > +} > + > +static uint32_t > +tuple_format_hash(struct tuple_format *format) > +{ > + uint32_t h = 0; 'h' should be initially set to a prime number, say 13. Take a look at HASH_SEED in tuple_hash.cc or json.c. > + uint32_t carry = 0; > + uint32_t size = 0; > + for (uint32_t i = 0; i < tuple_format_field_count(format); ++i) { > + struct tuple_field *f = tuple_format_field(format, i); > + PMurHash32_Process(&h, &carry, &f->type, > + sizeof(enum field_type)); > + size += sizeof(enum field_type); > + PMurHash32_Process(&h, &carry, &f->coll_id, > + sizeof(uint32_t)); If we change coll_id type from uint32_t to uint16_t, we'll almost certainly overlook this place. Please use sizeof(f->coll_id). Also, I think we should define a macro here that would compute a hash over a tuple_field struct member and increment the size: tuple_format_hash(...) { #define TUPLE_FIELD_MEMBER_HASH(field, member, h, carry, size) ... for each field TUPLE_FIELD_MEMBER_HASH(f, coll_id, h, carry, size); ... #undef TUPLE_FIELD_MEMBER_HASH } > + size += sizeof(uint32_t); > + PMurHash32_Process(&h, &carry, &f->nullable_action, > + sizeof(enum on_conflict_action)); > + size += sizeof(enum on_conflict_action); > + PMurHash32_Process(&h, &carry, &f->is_key_part, sizeof(bool)); > + size += sizeof(bool); > + } > + return PMurHash32_Result(h, carry, size); > +} > + > +#define MH_SOURCE 1 > +#define mh_name _tuple_format > +#define mh_key_t struct tuple_format * > +#define mh_node_t struct tuple_format * > +#define mh_arg_t void * > +#define mh_hash(a, arg) ((*(a))->hash) > +#define mh_hash_key(a, arg) ((a)->hash) > +#define mh_cmp(a, b, arg) (tuple_format_cmp(*(a), *(b))) > +#define mh_cmp_key(a, b, arg) (tuple_format_cmp((a), *(b))) > +#include "salad/mhash.h" > + > +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) > { > + 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); Since you add a format to the hash in tuple_format_new(), I guess you should remove it from the hash in tuple_format_delete(), not in tuple_format_destroy(). > free(format->required_fields); > tuple_format_destroy_fields(format); > tuple_dictionary_unref(format->dict); > @@ -402,7 +475,8 @@ 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, struct tuple_dictionary *dict, > - bool is_temporary, uint32_t exact_field_count) > + bool is_temporary, bool is_ephemeral, > + uint32_t exact_field_count) > { > struct tuple_format *format = > tuple_format_alloc(keys, key_count, space_field_count, dict); > @@ -411,18 +485,46 @@ tuple_format_new(struct tuple_format_vtab *vtab, void *engine, > format->vtab = *vtab; > format->engine = engine; > format->is_temporary = is_temporary; > + format->is_ephemeral = is_ephemeral; > format->exact_field_count = exact_field_count; > - if (tuple_format_register(format) < 0) { > - tuple_format_destroy(format); > - free(format); > - return NULL; > - } > if (tuple_format_create(format, keys, key_count, space_fields, > space_field_count) < 0) { > tuple_format_delete(format); > return NULL; > } > - return format; > + format->hash = tuple_format_hash(format); I don't think we should compute the hash if we aren't going to reuse the format: the hash function won't work for all formats as it doesn't check dict, engine, temporary flag. I'd also add assertions making sure those fields are unset if the space is going to be reused. > + struct errinj *inj = errinj(ERRINJ_TUPLE_FORMAT_COUNT, ERRINJ_INT); > + if (inj != NULL && inj->iparam == 200) { > + inj = NULL; > + } Looks like this piece left from some preliminary version of the patch. Please remove. > + if (format->is_ephemeral) { > + mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format, > + NULL); > + if (key != mh_end(tuple_formats_hash)) { > + struct tuple_format **entry = mh_tuple_format_node(tuple_formats_hash, key); > + tuple_format_destroy(format); > + free(format); > + return *entry; This piece of code should be factored out to a separate function, say tuple_format_reuse(), with a comment explaining why we only reuse ephemeral formats. > + } else { > + if (tuple_format_register(format) < 0) { > + tuple_format_destroy(format); > + free(format); > + return NULL; > + } > + mh_tuple_format_put(tuple_formats_hash, > + (const > + struct tuple_format **)&format, Splitting type conversion looks ugly. Please don't do that. Better not align arguments by parenthesis in such a case: mh_tuple_format_put(tuple_formats_hash, (const struct tuple_format **)&format, NULL, NULL); Also, you forgot to check ENOMEM error here (say hi to #3534 :) > + NULL, NULL); > + return format; > + } > + } else { > + if (tuple_format_register(format) < 0) { Please try to rearrange the code somehow so that there's only one place in the code where you call tuple_format_register(). > + tuple_format_destroy(format); > + free(format); > + return NULL; > + } > + return format; > + } > } > > bool > @@ -823,3 +925,15 @@ 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/src/box/tuple_format.h b/src/box/tuple_format.h > index 9778fda..d347eb7 100644 > --- a/src/box/tuple_format.h > +++ b/src/box/tuple_format.h > @@ -151,6 +151,11 @@ struct tuple_format { > * in progress. > */ > bool is_temporary; > + /** > + * This format belongs to ephemeral space and thus might > + * be shared with other ephemeral spaces. > + */ > + bool is_ephemeral; > /** > * Size of field map of tuple in bytes. > * \sa struct tuple > @@ -193,6 +198,11 @@ struct tuple_format { > * tuple_field::token. > */ > struct json_tree fields; > + Extra new line. > + /** > + * Hash key for shared formats of epeheral spaces. > + */ > + uint32_t hash; I'd move this definition higher, after 'id'. > }; > > /** > @@ -200,7 +210,7 @@ struct tuple_format { > * a given format. > */ > static inline uint32_t > -tuple_format_field_count(struct tuple_format *format) > +tuple_format_field_count(const struct tuple_format *format) Please don't do that. I deliberately avoid using const for tuple_format as it uses reference counting and json tree, which don't play nicely with const. > { > const struct json_token *root = &format->fields.root; > return root->children != NULL ? root->max_child_idx + 1 : 0; > @@ -263,6 +273,7 @@ tuple_format_unref(struct tuple_format *format) > * @param key_count The number of keys in @a keys array. > * @param space_fields Array of fields, defined in a space format. > * @param space_field_count Length of @a space_fields. > + * @param is_ephemeral Set if format belongs to ephemeral space. is_ephemeral goes after is_temporary in the argument list. > * @param is_temporary Set if format is temporary. > * @param exact_field_count Exact field count for format. > * > @@ -274,7 +285,8 @@ 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, struct tuple_dictionary *dict, > - bool is_temporary, uint32_t exact_field_count); > + bool is_temporary, bool is_ephemeral, > + uint32_t exact_field_count); > > /** > * Check, if @a format1 can store any tuples of @a format2. For > @@ -459,6 +471,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 formats. I'd rather say 'Initialize tuple format subsystem' or 'Initialize tuple format library'. > + * @retval 0 on success, -1 otherwise. > + */ > +int > +tuple_format_init(); > + > #if defined(__cplusplus) > } /* extern "C" */ > #endif /* defined(__cplusplus) */ > diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua > index fa7f9f2..0b50864 100644 > --- a/test/sql/errinj.test.lua > +++ b/test/sql/errinj.test.lua > @@ -5,6 +5,20 @@ box.sql.execute('pragma sql_default_engine=\''..engine..'\'') > errinj = box.error.injection > fiber = require('fiber') > > +-- gh-3924 Check that tuple_formats of ephemeral spaces are > +-- reused. > +format = {} This assignment isn't needed. Please remove. > +box.sql.execute("CREATE TABLE t4 (id INTEGER PRIMARY KEY, a INTEGER);") > +box.sql.execute("INSERT INTO t4 VALUES (1,1)") > +box.sql.execute("INSERT INTO t4 VALUES (2,1)") > +box.sql.execute("INSERT INTO t4 VALUES (3,2)") > +errinj.set('ERRINJ_TUPLE_FORMAT_COUNT', 200) > +errinj.set('ERRINJ_MEMTX_DELAY_GC', true) > +for i = 1, 201 do box.sql.execute("SELECT DISTINCT a FROM t4") end > +errinj.set('ERRINJ_MEMTX_DELAY_GC', false) > +errinj.set('ERRINJ_TUPLE_FORMAT_COUNT', -1) > +box.sql.execute('DROP TABLE t4') > + > box.sql.execute('create table test (id int primary key, a float, b text)') > box.schema.user.grant('guest','read,write,execute', 'universe') > cn = remote.connect(box.cfg.listen)