From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (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 2BBCE469719 for ; Mon, 12 Oct 2020 22:06:03 +0300 (MSK) Date: Mon, 12 Oct 2020 22:06:21 +0300 From: Alexander Turenko Message-ID: <20201012190621.ocgqlzrgyqrgu4ta@tkn_work_nb> References: <5cbb2e41bb32a3184d2663f678b705aef823dc85.1600817803.git.alexander.turenko@tarantool.org> <6caeccfe-9310-8ae1-f4bb-078b552dae1a@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <6caeccfe-9310-8ae1-f4bb-078b552dae1a@tarantool.org> 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: Vladislav Shpilevoy Cc: tarantool-patches@dev.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 > > + * (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. 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.