Re: [PATCH v5 2/3] box: introduce tuple_chunk infrastructure

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Thu, Jul 25, 2019 at 09:39:44PM +0300, Kirill Shcherbatov wrote:
> Introduced a new object tuple_chunk: a memory allocation is
> associated with given tuple. tuple_format's vtab is extended
> with few new methods to manage tuple_chunks lifecycle.
> Implemented corresponding methid for memtx engine: a memory
> chunks are allocated with memtx's smalloc allocator.
> 
> Needed for #1260

I wouldn't use a separate patch for this, because this patch introduces
some code which is only used by the following patches and useless
otherwise. It's difficult to review without peeking at the following
patches. It's not important though so never mind.

> ---
>  src/box/tuple.h        | 28 ++++++++++++++++++++++++++++
>  src/box/tuple_format.h |  9 +++++++++
>  src/box/memtx_engine.c | 27 +++++++++++++++++++++++++++
>  src/box/tuple.c        |  8 ++++++++
>  src/box/vy_stmt.c      |  2 ++
>  5 files changed, 74 insertions(+)
> 
> diff --git a/src/box/tuple.h b/src/box/tuple.h
> index 99dfeb82d..60b6fb474 100644
> --- a/src/box/tuple.h
> +++ b/src/box/tuple.h
> @@ -447,6 +447,34 @@ tuple_delete(struct tuple *tuple)
>  	format->vtab.tuple_delete(format, tuple);
>  }
>  
> +/** Tuple chunk memory object. */
> +struct tuple_chunk {
> +	/** The payload size. Needed to perform memory release.*/
> +	uint32_t data_sz;
> +	/** Metadata object payload. */
> +	char data[0];
> +};

Do we really need this level of abstraction? Can't we simply add tuple
format methods to allocate some opaque extra data and free it? Can't
tell without looking at the next patch...

> +
> +/** Calculate the size of tuple_chunk object by given data_sz. */
> +uint32_t
> +tuple_chunk_sz(uint32_t data_sz);
> +
> +/** Allocate a new tuple_chunk for given tuple. */
> +static inline struct tuple_chunk *
> +tuple_chunk_new(struct tuple *tuple, uint32_t data_sz)
> +{
> +	struct tuple_format *format = tuple_format(tuple);
> +	return format->vtab.tuple_chunk_new(format, tuple, data_sz);
> +}
> +
> +/** Free a tuple_chunk is allocated for given tuple. */
> +static inline void
> +tuple_chunk_delete(struct tuple *tuple, struct tuple_chunk *tuple_chunk)
> +{
> +	struct tuple_format *format = tuple_format(tuple);
> +	format->vtab.tuple_chunk_delete(format, tuple_chunk);
> +}

Why pass tuples to these methods? Solely to extract a format?

> diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
> index 428491c2d..fbb3151c9 100644
> --- a/src/box/memtx_engine.c
> +++ b/src/box/memtx_engine.c
> @@ -1126,9 +1126,36 @@ memtx_tuple_delete(struct tuple_format *format, struct tuple *tuple)
>  		smfree_delayed(&memtx->alloc, memtx_tuple, total);
>  }
>  
> +void
> +metmx_tuple_chunk_delete(struct tuple_format *format,
> +			 struct tuple_chunk *tuple_chunk)
> +{
> +	struct memtx_engine *memtx = (struct memtx_engine *)format->engine;
> +	uint32_t sz = tuple_chunk_sz(tuple_chunk->data_sz);
> +	smfree(&memtx->alloc, tuple_chunk, sz);
> +}
> +
> +struct tuple_chunk *
> +memtx_tuple_chunk_new(struct tuple_format *format, struct tuple *tuple,
> +		      uint32_t data_sz)
> +{
> +	struct memtx_engine *memtx = (struct memtx_engine *)format->engine;
> +	uint32_t sz = tuple_chunk_sz(data_sz);
> +	struct tuple_chunk *tuple_chunk =
> +		(struct tuple_chunk *) smalloc(&memtx->alloc, sz);
> +	if (tuple == NULL) {

s/tuple/tuple_chunk

Looks like you don't need 'tuple' here.

> +		diag_set(OutOfMemory, sz, "smalloc", "tuple");
> +		return NULL;
> +	}
> +	tuple_chunk->data_sz = data_sz;
> +	return tuple_chunk;
> +}
> +
>  struct tuple_format_vtab memtx_tuple_format_vtab = {
>  	memtx_tuple_delete,
>  	memtx_tuple_new,
> +	metmx_tuple_chunk_delete,
> +	memtx_tuple_chunk_new,
>  };
>  
>  /**
> diff --git a/src/box/tuple.c b/src/box/tuple.c
> index c0e94d55b..25f85f732 100644
> --- a/src/box/tuple.c
> +++ b/src/box/tuple.c
> @@ -67,6 +67,8 @@ runtime_tuple_new(struct tuple_format *format, const char *data, const char *end
>  static struct tuple_format_vtab tuple_format_runtime_vtab = {
>  	runtime_tuple_delete,
>  	runtime_tuple_new,
> +	NULL,
> +	NULL,
>  };
>  
>  static struct tuple *
> @@ -785,3 +787,9 @@ mp_str(const char *data)
>  		return "<failed to format message pack>";
>  	return buf;
>  }
> +
> +uint32_t
> +tuple_chunk_sz(uint32_t data_sz)
> +{
> +	return sizeof(struct tuple_chunk) + data_sz;
> +}

This function could be inlined.