From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 CDE9A42EF5C for ; Wed, 1 Jul 2020 23:47:20 +0300 (MSK) Date: Wed, 1 Jul 2020 23:37:06 +0300 From: Igor Munkin Message-ID: <20200701203706.GC5559@tarantool.org> References: <88f5a7e52eb203bbb959f0f811766887f89371f0.1592416673.git.alexander.turenko@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <88f5a7e52eb203bbb959f0f811766887f89371f0.1592416673.git.alexander.turenko@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 3/3] lua: expose temporary Lua state for iproto calls List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy Sasha, Thanks for the patch! It LGTM except the nits I mentioned below. On 18.06.20, Alexander Turenko wrote: > There is code that may save some time and resources for creating a new > Lua state when it is present in the fiber storage of a current fiber. Typo: s/present/presented/. > There are not so much of them: running a Lua trigger and construction of > a next tuple in a merge source. > This patch fills fiber->storage.lua.stack for background fibers that > serve a Lua call or eval: we already have this state and nothing prevent Typo: s/prevent/prevents/. > us from exposing it via the fiber storage. > > Follows up #4954 > --- > src/box/lua/call.c | 27 +++++++++++++++++++++++++++ > src/lib/core/fiber.h | 14 ++++++++++---- > 2 files changed, 37 insertions(+), 4 deletions(-) > > diff --git a/src/box/lua/call.c b/src/box/lua/call.c > index 6588ec2fa..ccdef6662 100644 > --- a/src/box/lua/call.c > +++ b/src/box/lua/call.c > @@ -537,12 +537,39 @@ box_process_lua(lua_CFunction handler, struct execute_lua_ctx *ctx, > port_lua_create(ret, L); > ((struct port_lua *) ret)->ref = coro_ref; > > + /* > + * A code that need a temporary fiber-local Lua state may > + * save some time and resources for creating a new state > + * and use this one. > + */ Could you please provide an example for the fiber calling this function with non-NULL fiber-local Lua state? Are those conditions below are strictly required by the current implementation? > + bool has_lua_stack = fiber()->storage.lua.stack != NULL; > + if (!has_lua_stack) > + fiber()->storage.lua.stack = L; > + > lua_pushcfunction(L, handler); > lua_pushlightuserdata(L, ctx); > if (luaT_call(L, 1, LUA_MULTRET) != 0) { > + if (!has_lua_stack) > + fiber()->storage.lua.stack = NULL; > port_lua_destroy(ret); > return -1; > } > + > + /* > + * Since this field is optional we're not obligated to > + * keep it until the Lua state will be unreferenced in > + * port_lua_destroy(). > + * > + * There is no much sense to keep it beyond the Lua call, > + * so let's zap now. > + * > + * But: keep the stack if it was present before the call, > + * because it would be counter-intuitive if the existing > + * state pointer would be zapped after this function call. > + */ > + if (!has_lua_stack) > + fiber()->storage.lua.stack = NULL; > + > return 0; > } > > -- > 2.25.0 > -- Best regards, IM