From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 92FD9469719 for ; Fri, 25 Sep 2020 01:32:02 +0300 (MSK) References: <5cbb2e41bb32a3184d2663f678b705aef823dc85.1600817803.git.alexander.turenko@tarantool.org> From: Vladislav Shpilevoy Message-ID: <6caeccfe-9310-8ae1-f4bb-078b552dae1a@tarantool.org> Date: Fri, 25 Sep 2020 00:32:01 +0200 MIME-Version: 1.0 In-Reply-To: <5cbb2e41bb32a3184d2663f678b705aef823dc85.1600817803.git.alexander.turenko@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 05/14] WIP: module api/lua: add luaT_tuple_encode() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.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 > + * (it also available as 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);