From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 2 Apr 2018 13:50:12 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] Re: [PATCH 1/2] vinyl: allocate upsert counter on lsregion Message-ID: <20180402105012.nr37nor2d2dnqdlv@esperanza> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: "v.shpilevoy@tarantool.org" Cc: tarantool-patches@freelists.org, kostja@tarantool.org List-ID: On Mon, Apr 02, 2018 at 01:27:14PM +0300, v.shpilevoy@tarantool.org wrote: > > @@ -134,13 +134,26 @@ struct tuple * > > vy_stmt_dup_lsregion(const struct tuple *stmt, struct lsregion *lsregion, > > int64_t alloc_id) > > { > > + enum iproto_type type = vy_stmt_type(stmt); > > size_t size = tuple_size(stmt); > > + size_t alloc_size = size; > > struct tuple *mem_stmt; > > - mem_stmt = lsregion_alloc(lsregion, size, alloc_id); > > + > > + /* Reserve one byte for UPSERT counter. */ > > + if (type == IPROTO_UPSERT) > > + alloc_size++; > > + > > + mem_stmt = lsregion_alloc(lsregion, alloc_size, alloc_id); > > if (mem_stmt == NULL) { > > diag_set(OutOfMemory, size, "lsregion_alloc", "mem_stmt"); > > return NULL; > > } > > + > > + if (type == IPROTO_UPSERT) { > > + *(uint8_t *)mem_stmt = 0; > > 1. How about to at first, set mem_stmt and at second, call vy_stmt_set_n_upserts? I think, > that this pointers magic must be hidden where possible. I thought about that, but I'd have to play with pointers anyway - see right below how I adjust mem_stmt address. > > + mem_stmt = (struct tuple *)((uint8_t *)mem_stmt + 1); This has to be done irrespective of whether I use vy_stmt_set_n_upserts or not. And since I already play with pointers, I reckoned that it would be OK to assign n_upserts here as well rather than scatter the logic between two functions. > > + } > > + > > memcpy(mem_stmt, stmt, size); > > /* > > * Region allocated statements can't be referenced or unreferenced > > > > struct tuple * > > diff --git a/src/box/vy_stmt.h b/src/box/vy_stmt.h > > index a33739d6..8958adb5 100644 > > --- a/src/box/vy_stmt.h > > +++ b/src/box/vy_stmt.h > > @@ -146,23 +146,28 @@ vy_stmt_set_type(struct tuple *stmt, enum iproto_type type) > > ((struct vy_stmt *) stmt)->type = type; > > } > > > > -/** Get upserts count of the vinyl statement. */ > > +/** > > + * Get upserts count of the vinyl statement. > > + * Only for UPSERT statements allocated on lsregion. > > 2. Can you, please, add an assertion for this? For example: > assert(! vy_stmt_is_refable(stmt)). I did: see 'assert(stmt->refs == 0)' right below. I didn't use vy_stmt_is_refable(), because it's defined below this function, and I didn't want to move it. > > > + */ > > static inline uint8_t > > vy_stmt_n_upserts(const struct tuple *stmt) > > { > > - assert(tuple_format(stmt)->extra_size == sizeof(uint8_t)); > > - return *((const uint8_t *) tuple_extra(stmt)); > > + assert(stmt->refs == 0); > > + assert(vy_stmt_type(stmt) == IPROTO_UPSERT); > > + return *((uint8_t *)stmt - 1); > > }