Re: [Tarantool-patches] [PATCH 05/14] WIP: module api/lua: add luaT_tuple_encode()

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

See 2 comments below.

I will pause the review of the next patches until I get
back to work tomorrow/after tomorrow.

> diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
> index 766275ff2..759df0b89 100644
> --- a/src/box/lua/tuple.h
> +++ b/src/box/lua/tuple.h
> @@ -79,6 +79,35 @@ luaT_pushtuple(struct lua_State *L, box_tuple_t *tuple);
>  box_tuple_t *
>  luaT_istuple(struct lua_State *L, int idx);
>  
> +/**
> + * Encode a Lua table, a tuple or objects on a Lua stack as raw
> + * tuple data (MsgPack).
> + *
> + * @param L             Lua state
> + * @param idx           acceptable index on the Lua stack
> + *                      (or zero, see below)
> + * @param tuple_len_ptr where to store tuple data size in bytes
> + *                      (or NULL)
> + *
> + * Set idx to zero to create the new tuple from objects on the Lua
> + * stack.
> + *
> + * The data is encoded on the shared buffer: so called
> + * <tarantool_lua_ibuf> (it also available as <buffer.SHARED_IBUF>

1. Both are private. I wouldn't mention them. I think it is enough to
say, that the allocation is 'internal', and may be freed by any next
lua-related call.

> + * in Lua). The data is valid until next similar call. It is
> + * generally safe to pass the result to a box function (copy it if
> + * doubt). No need to release this buffer explicitly, it'll be
> + * reused by later calls.

2. Why exactly are we doing this export? I need a remainder. I remember
we discussed if we could return the encoded tuple on a region, but
decided it could be too slow, because could lead to oscillation of the
region's slab if the region would be truncated after every call.

Also I remember we discussed if we could expose the ibuf too. Because it
is a nice allocator to allocate contiguous memory blocks but without
an oscillation. Then we looked at it and found, that it actually
oscillates. Because we call ibuf_reinit, which puts the slab back to the
cache.

That makes me wonder why not use the region, if both suck at oscillation,
but at least we exposed and documented the region already.

If you worry about code duplication, I think it wouldn't be that terrible.
luaT_tuple_encode() uses mpstream. So most of it could be moved to a new
common static function inside lua/tuple.c which takes an mpstream, and
stores Lua objects into it. And 2 wrappers on top - one creates mpstream
from an ibuf, another from the region.

What I do worry about - the function may throw, and something would be left
on the region memory. Is it critical? Can't it happen with like any other
allocator too, so the problem is not really in the region itself?

> + *
> + * If encoding fails, raise an error.
> + *
> + * In case of any other error set a diag and return NULL.
> + *
> + * @sa luaT_tuple_new()
> + */
> +API_EXPORT char *
> +luaT_tuple_encode(struct lua_State *L, int idx, size_t *tuple_len_ptr);