[tarantool-patches] [PATCH v7 1/1] box: create bigrefs for tuples
Vladimir Davydov
vdavydov.dev at gmail.com
Thu Jun 14 13:22:03 MSK 2018
On Thu, Jun 14, 2018 at 12:18:37PM +0300, imeevma at 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 at .
> 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;
> +}
More information about the Tarantool-patches
mailing list