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

[v.shpilevoy at tarantool.org]

Hello. Please, see 2 comments below.

> 30 марта 2018 г., в 18:47, Vladimir Davydov <vdavydov.dev at gmail.com> написал(а):
> 
> Currently, we store upsert counter in tuple metadata (that's what
> upsert_format is for), but since it's only relevant for tuples of
> the memory level, we can store it on lsregion, right before tuple
> data. Let's do it now so that we can get rid of upsert_format.
> ---
> src/box/vy_stmt.c | 24 ++++++++++++++++--------
> src/box/vy_stmt.h | 23 ++++++++++++++---------
> 2 files changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c
> index 84182e76..ac60c89e 100644
> --- a/src/box/vy_stmt.c
> +++ b/src/box/vy_stmt.c
> @@ -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.

> +		mem_stmt = (struct tuple *)((uint8_t *)mem_stmt + 1);
> +	}
> +
> 	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)).

> + */
> 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);
> }
> 
> -/** Set upserts count of the vinyl statement. */
> +/**
> + * Set upserts count of the vinyl statement.
> + * Only for UPSERT statements allocated on lsregion.
> + */
> static inline void
> vy_stmt_set_n_upserts(struct tuple *stmt, uint8_t n)
> {
> -	struct tuple_format *format = tuple_format(stmt);
> -	assert(format->extra_size == sizeof(uint8_t));
> -	char *extra = (char *) stmt + stmt->data_offset -
> -		      tuple_format_meta_size(format);
> -	*((uint8_t *) extra) = n;
> +	assert(stmt->refs == 0);
> +	assert(vy_stmt_type(stmt) == IPROTO_UPSERT);
> +	*((uint8_t *)stmt - 1) = n;
> }
> 
> /** Get the column mask of the specified tuple. */
> -- 
> 2.11.0
> 
>