From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 14 Jun 2018 13:22:03 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] [PATCH v7 1/1] box: create bigrefs for tuples Message-ID: <20180614102203.lutaaazuxqqkzlbs@esperanza> References: <6d1598bb7fedf1041fb78f389db6530879da43ec.1528966547.git.imeevma@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6d1598bb7fedf1041fb78f389db6530879da43ec.1528966547.git.imeevma@gmail.com> To: imeevma@tarantool.org Cc: tarantool-patches@freelists.org List-ID: 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; > +}