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

[Alexander Turenko <alexander.turenko@tarantool.org>]

> > +/**
> > + * 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.

It is not more actual: I changed luaT_tuple_encode() to use the box
region.

> 
> > + * 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.

Not exactly. I doubt about changing the existing user visible function
from ibuf to region: box.tuple.new(), but I'm okay with exposing the new
one that is initially based on the region allocator. The reason is that
I unable to provide a good reason for such change for an existing
function. Maybe it'll degrade by performance in some cases. Maybe not.
Anyway, there is no reason to change the allocator under box.tuple.new()
(except our internal coding problems).

Maybe I worry more than necessary. I don't know, so I would go by the
more safe pathway.

> 
> 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.

Not exactly. It is seems, it is just mistake on the caller side:
ibuf_reinit() is called instead of ibuf_reset(). It is in my backlog,
I'll file an issue and fix it when time will permit. So ibuf itself is
out of suspicions.

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

This arguments works for me. It seems, we usually lean on the region in
the public APIs, so it would be better to continue to do so and keep
some consistency. At least it is easier to understand and use.

> 
> 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.

Not exactly so: mpstream initialization also may raise an error. But,
yep, it is still possible to do some code unification.

It does not look nice, but at least not so disgusting as code
duplication.

> 
> 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?

I filed #5382 regarding this and fixed it within the patchset.