From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 3C66825405 for ; Fri, 13 Sep 2019 18:28:50 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ETXTtpAltQK3 for ; Fri, 13 Sep 2019 18:28:50 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id DF7B325364 for ; Fri, 13 Sep 2019 18:28:49 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 3/4] tuple: use global msgpack serializer in Lua tuple References: <98110cbc514cb13f3ba3b298ed5b6a493fcc36e5.1568055477.git.v.shpilevoy@tarantool.org> <20190912232758.qvb3kloft3ngv6fj@tkn_work_nb> From: Vladislav Shpilevoy Message-ID: Date: Sat, 14 Sep 2019 00:32:39 +0200 MIME-Version: 1.0 In-Reply-To: <20190912232758.qvb3kloft3ngv6fj@tkn_work_nb> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Alexander Turenko Cc: tarantool-patches@freelists.org Thanks for the review! On 13/09/2019 01:27, Alexander Turenko wrote: > LGTM. > > Several questions are below. > > I agree that the variant with triggers looks more natural. > > Should not we add box_lua_tuple_free() and call trigger_destroy() > inside? I know, we now don't call tarantool_lua_free(), but I hope it > will be fixed someday. > > If you'll going to add it, please, add also the call to luaT_tuple_new.c > for the symmetry with box_lua_tuple_init(). It also will help to keep > this test clean from ASAN / Valgrind point of view: I did verify it > aganst one of those tools at the time of writing the test (don't > remember against which of them). I tried to add box_lua_tuple_free, but as you mentioned, we don't have box_lua_free() so that function is unused. I don't think it should be added now. Also I stumbled into a problem, that luaT_tuple_new.c uses luaopen_msgpack, but we don't have luaclose_msgpack. So there are 2 holes. Moreover, as I understand, no one of our Lua C modules really drops its bindings from Lua namespace. It is not related to this issue, but I remember that someday we wanted to be able to 'uncfg' box, like it was not called. > > Hm. We don't have box_lua_free() at all. I'm doubtful now. > > WBR, Alexander Turenko. > >> +static inline void >> +tuple_serializer_fill(void) >> +{ >> + luaL_serializer_copy_options(&tuple_serializer, luaL_msgpack_default); >> + tuple_serializer.encode_sparse_ratio = 0; >> +} > > Is not this name quite common? Maybe tuple_serializer_update_options()? > Honestly, I don't really care that much. The function with all its invocations fits in one screen. Both names are ok for me: diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c index fc22f5572..8b59466b9 100644 --- a/src/box/lua/tuple.c +++ b/src/box/lua/tuple.c @@ -554,7 +554,7 @@ static const struct luaL_Reg lbox_tuple_iterator_meta[] = { /* }}} */ static inline void -tuple_serializer_fill(void) +tuple_serializer_update_options(void) { luaL_serializer_copy_options(&tuple_serializer, luaL_msgpack_default); tuple_serializer.encode_sparse_ratio = 0; @@ -565,7 +565,7 @@ on_msgpack_serializer_update(struct trigger *trigger, void *event) { (void) trigger; (void) event; - tuple_serializer_fill(); + tuple_serializer_update_options(); } void @@ -584,7 +584,7 @@ box_lua_tuple_init(struct lua_State *L) luamp_set_encode_extension(luamp_encode_extension_box); - tuple_serializer_fill(); + tuple_serializer_update_options(); trigger_create(&tuple_serializer.update_trigger, on_msgpack_serializer_update, NULL, NULL); trigger_add(&luaL_msgpack_default->on_update,