From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 24 Jan 2019 20:49:05 +0300 From: Vladimir Davydov Subject: Re: [PATCH v2 4/4] Allow to reuse tuple_formats for ephemeral spaces Message-ID: <20190124174904.whc63hewbxkjhyjn@esperanza> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Kirill Yukhin Cc: tarantool-patches@freelists.org List-ID: 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? > if (format == NULL) { > free(memtx_space); > return NULL; > 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. > + struct tuple_field *field_a = tuple_format_field( > + (struct tuple_format *)a, i); > + 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) > +{ > +#define TUPLE_FIELD_MEMBER_HASH(field, member, h, carry, size) \ > + PMurHash32_Process(&h, &carry, &field->member, \ > + sizeof(field->member)); \ > + size += sizeof(field->member); > + > + uint32_t h = 13; > + 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); > + TUPLE_FIELD_MEMBER_HASH(f, type, h, carry, size) > + TUPLE_FIELD_MEMBER_HASH(f, coll_id, h, carry, size) > + TUPLE_FIELD_MEMBER_HASH(f, nullable_action, h, carry, size) > + TUPLE_FIELD_MEMBER_HASH(f, is_key_part, h, carry, size) > + } > +#undef TUPLE_FIELD_MEMBER_HASH > + 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) > @@ -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?). > tuple_format_deregister(format); > tuple_format_destroy(format); > free(format); > } > > +/** > + * Try to reuse given format. This is only possible for formats > + * of ephemeral spaces, since we need to be sure that shared > + * dictionary will never be altered. If it can, then alter can > + * affect another space, which shares a format with one which is > + * altered. > + * @param p_format Double pointer to format. It is updated with > + * hashed value, if corresponding format was found > + * in hash table > + * @retval Returns true if format was found in hash table, false > + * otherwise. > + * > + */ > +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. > + 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); > + *p_format = *entry; > + return true; > + } > + return false; > +} > + > +/** > + * See justification, why ephemeral space's formats are > + * only feasible for hasing. > + * @retval 0 on success, even if format wasn't added to hash > + * -1 in case of error. > + */ > +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 ;-) > + 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. > #if defined(__cplusplus) > } /* extern "C" */ > #endif /* defined(__cplusplus) */ > 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. > 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 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. > fiber_init(fiber_c_invoke); > tuple_init(NULL); > vy_cache_env_create(&cache_env, cord_slab_cache()); > vy_cache_env_set_quota(&cache_env, cache_size); > vy_key_format = tuple_format_new(&vy_tuple_format_vtab, NULL, NULL, 0, > - NULL, 0, 0, NULL, false); > + NULL, 0, 0, NULL, false, false); > tuple_format_ref(vy_key_format); > > size_t mem_size = 64 * 1024 * 1024;