From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <imun@tarantool.org>
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 dev.tarantool.org (Postfix) with ESMTPS id 5650741C5DB
 for <tarantool-patches@dev.tarantool.org>;
 Sun, 21 Jun 2020 00:34:00 +0300 (MSK)
Date: Sun, 21 Jun 2020 00:24:49 +0300
From: Igor Munkin <imun@tarantool.org>
Message-ID: <20200620212449.GB3503@tarantool.org>
References: <cover.1592597647.git.imun@tarantool.org>
 <7b8e95f3d6da39f0c3c6ca742abccddda817ac5e.1592597647.git.imun@tarantool.org>
 <4150e5b8-b4c0-7151-d06e-a61d81eff97b@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <4150e5b8-b4c0-7151-d06e-a61d81eff97b@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH 2/2] box: reduce box_process_lua Lua
 GC memory usage
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org

Vlad,

Thanks for the review! I fixed your comments and updated the remote
branch.

On 20.06.20, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> On 19/06/2020 22:40, Igor Munkin wrote:
> > <box_process_lua> function created a new GCfunc object for a handler
> > having no upvalues depending on the request context on each call.
> > 
> > The changes introduces the folliwing mapping:
> 
> Either 'changes' -> 'change', or 'introduces' -> 'introduce'.
> 
> 'folliwing' -> 'following'.

My bad, fixed.

> 
> > | <handler id> -> <handler GCfunc object>
> > Initializing this mapping on Tarantool startup is aimed to reduce Lua GC
> > memory usage.
> > 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> >  src/box/lua/call.c | 32 +++++++++++++++++++++++++++-----
> >  1 file changed, 27 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> > index 6588ec2fa..e1b1a5e81 100644
> > --- a/src/box/lua/call.c
> > +++ b/src/box/lua/call.c
> > @@ -48,6 +48,15 @@
> >  #include "mpstream/mpstream.h"
> >  #include "box/session.h"
> >  
> > +enum handlers {
> > +	HANDLER_CALL,
> > +	HANDLER_CALL_BY_REF,
> > +	HANDLER_EVAL,
> > +	HANDLER_MAX,
> 
> Would be nice to have a comment here explaining why so complex.
> Why lua_pushcfunction() can't be used on each call.

OK, here is the diff:

================================================================================

diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index e1b1a5e81..e52f16ca4 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -48,6 +48,15 @@
 #include "mpstream/mpstream.h"
 #include "box/session.h"
 
+/*
+ * Handlers identifiers to obtain lua_Cfunction reference from
+ * Lua registry table. These handlers are initialized on Tarantool
+ * startup and are used until the Lua universe is destroyed.
+ * Such approach reduces Lua GC usage since there is no need to
+ * create short-lived GCfunc objects for the corresponding C
+ * function on each iproto CALL/EVAL request or stored Lua
+ * procedure call.
+ */
 enum handlers {
 	HANDLER_CALL,
 	HANDLER_CALL_BY_REF,

================================================================================

> 
> > +};
> > +
> > +static int execute_lua_refs[HANDLER_MAX];
> > +
> >  /**
> >   * A helper to find a Lua function by name and put it
> >   * on top of the stack.

-- 
Best regards,
IM