Re: [PATCH v2 4/4] Allow to reuse tuple_formats for ephemeral spaces

[Vladimir Davydov <vdavydov.dev@gmail.com>]

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;