Re: [tarantool-patches] [PATCH v7 1/1] box: create bigrefs for tuples

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

On Thu, Jun 14, 2018 at 12:18:37PM +0300, imeevma@tarantool.org wrote:
> Due to limitation of reference counters for tuple being only
> 65535 it was possible to reach this limitation. This patch
> increases capacity of reference counters to 4 billions.
> 
> Closes #3224
> ---
> Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-3224-tuple-bigrefs
> Issue: https://github.com/tarantool/tarantool/issues/3224

Patch change log is missing. Example:

  https://www.freelists.org/post/tarantool-patches/PATCH-v2-Allow-to-increase-boxcfgvinyl-memory-and-memtx-memory-at-runtime

> diff --git a/src/box/box.cc b/src/box/box.cc
> index c728a4c..4257861 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -174,20 +174,22 @@ process_rw(struct request *request, struct space *space, struct tuple **result)
>  		txn_rollback_stmt();
>  		return -1;
>  	}
> +	if (result == NULL)
> +		return txn_commit_stmt(txn, request);
> +	*result = tuple;
> +	if (tuple == NULL)
> +		return txn_commit_stmt(txn, request);
>  	/*
>  	 * Pin the tuple locally before the commit,
>  	 * otherwise it may go away during yield in
>  	 * when WAL is written in autocommit mode.
>  	 */
> -	TupleRefNil ref(tuple);
> -	if (txn_commit_stmt(txn, request) != 0)
> -		return -1;
> -	if (result != NULL) {
> -		if (tuple != NULL && tuple_bless(tuple) == NULL)
> -			return -1;
> -		*result = tuple;
> -	}
> -	return 0;
> +	tuple_ref(tuple);
> +	int rc = txn_commit_stmt(txn, request);
> +	if (rc == 0)
> +		tuple_bless(tuple);
> +	tuple_unref(tuple);
> +	return rc;

As I mentioned before, I don't like the way you reworked this function
workflow and squeezed this change into this particular patch, which has
nothing to do with it. I don't really care though. Up to kostja@.

> diff --git a/src/box/tuple.c b/src/box/tuple.c
> index 014f374..4eed95f 100644
> --- a/src/box/tuple.c
> +++ b/src/box/tuple.c
> @@ -48,6 +48,28 @@ enum {
>  	OBJSIZE_MIN = 16,
>  };
>  
> +/**
> + * Container for big reference counters. Contains array of big
> + * reference counters, size of this array and number of non-zero
> + * big reference counters. When reference counter of tuple becomes
> + * more than 32767, field refs of this tuple becomes index of big
> + * reference counter in big reference counter array and field
> + * is_bigref is set true. The moment big reference becomes equal
> + * 32767 it is set to 0, refs of the tuple becomes 32767 and
> + * is_bigref becomes false. Big reference counter can be equal to
> + * 0 or be more than 32767.
> + */
> +static struct bigref_list {
> +	/** Free-list of big reference counters. */
> +	uint32_t *refs;
> +	/** Number of non-zero elements in the array. */
> +	uint16_t size;
> +	/** Capacity of the array. */
> +	uint16_t capacity;

AFAICS the only reason why you maintain both 'capacity' and 'size' is
that you want to free the array when the last bigref is released. As I
said before, I find it pointless, and now it turns out it isn't just two
lines of code, after all. So I vote for removing this logic along with
the 'size' member.

> +	/** Index of first free element. */
> +	uint16_t vacant_index;
> +} bigref_list;
> +
>  static const double ALLOC_FACTOR = 1.05;
>  
>  /**
> @@ -151,6 +173,13 @@ tuple_validate_raw(struct tuple_format *format, const char *tuple)
>  	return 0;
>  }
>  
> +/** Initialize big references container. */
> +static inline void
> +bigref_list_create()

Nit: this function should take void as an argument: in C a function
whose argument list is empty can actually take any number of arguments.

> +{
> +	memset(&bigref_list, 0, sizeof(bigref_list));
> +}
> +
>  /**
>   * Incremented on every snapshot and is used to distinguish tuples
>   * which were created after start of a snapshot (these tuples can
> @@ -211,6 +240,8 @@ tuple_init(field_name_hash_f hash)
>  
>  	box_tuple_last = NULL;
>  
> +	bigref_list_create();
> +
>  	if (coll_id_cache_init() != 0)
>  		return -1;
>  
> @@ -244,6 +275,106 @@ tuple_arena_create(struct slab_arena *arena, struct quota *quota,
>  	}
>  }
>  
> +enum {
> +	BIGREF_FACTOR = 2,
> +	BIGREF_MAX = UINT32_MAX,
> +	BIGREF_MIN_CAPACITY = 16,
> +	/**
> +	 * Only 15 bits are available for bigref list index in
> +	 * struct tuple.
> +	 */
> +	BIGREF_MAX_CAPACITY = UINT16_MAX >> 1
> +};
> +
> +/** Destroy big references and free memory that was allocated. */
> +static inline void
> +bigref_list_reset()

We usually pair 'create' with 'destroy', not 'reset'.

> +{
> +	free(bigref_list.refs);
> +	bigref_list_create();

If you don't destroy the list when the last bigref is freed, you won't
need to recreate the list here and so this function will turn into a
plain destructor. Another reason to remove that logic.

> +}
> +
> +/**
> + * Increase capacity of bigref_list.
> + */
> +static inline void
> +bigref_list_increase_capacity()

Nit: again, this function should take (void).

> +{
> +	assert(bigref_list.capacity == bigref_list.vacant_index);
> +	uint32_t *refs = bigref_list.refs;
> +	uint16_t capacity = bigref_list.capacity;
> +	if (capacity == 0)
> +		capacity = BIGREF_MIN_CAPACITY;
> +	else if (capacity < BIGREF_MAX_CAPACITY)
> +		capacity = MIN(capacity * BIGREF_FACTOR, BIGREF_MAX_CAPACITY);
> +	else
e +		panic("Too many big references");
> +	refs = (uint32_t *) realloc(refs, capacity * sizeof(*refs));
> +	if (refs == NULL) {
> +		panic("failed to reallocate %zu bytes: Cannot allocate "\
> +		      "memory.", capacity * sizeof(*refs));
> +	}
> +	for(uint16_t i = bigref_list.capacity; i < capacity; ++i)
> +		refs[i] = i + 1;
> +	bigref_list.refs = refs;
> +	bigref_list.capacity = capacity;
> +}
> +
> +/**
> + * Return index for new big reference counter and allocate memory
> + * if needed.
> + * @retval index for new big reference counter.
> + */
> +static inline uint16_t
> +bigref_list_new_index()

Nit: again, this function should take (void).

> +{
> +	if (bigref_list.size == bigref_list.capacity)

'size' is not necessary for this, instead you can use

	if (vacant_index == bigref_list.capacity)

> +		bigref_list_increase_capacity();
> +	++bigref_list.size;
> +	uint16_t vacant_index = bigref_list.vacant_index;
> +	bigref_list.vacant_index = bigref_list.refs[vacant_index];
> +	return vacant_index;
> +}