[tarantool-patches] Re: [PATCH 1/2] vinyl: allocate upsert counter on lsregion

[Vladimir Davydov]

On Mon, Apr 02, 2018 at 01:27:14PM +0300, v.shpilevoy at 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);
> > }