From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 D5D08469711 for ; Wed, 3 Jun 2020 01:48:22 +0300 (MSK) References: <353b8524edba38a68642a10d5b6bd6b033477e33.1591028838.git.alexander.turenko@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Wed, 3 Jun 2020 00:48:21 +0200 MIME-Version: 1.0 In-Reply-To: <353b8524edba38a68642a10d5b6bd6b033477e33.1591028838.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 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 Thanks for the patch! See 3 comments below. On 01/06/2020 20:10, 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. > There are not so much of them: running a Lua trigger and construction of > a next tuple in a merge source. > > Before the patch, fiber->storage.lua.stack is filled only for the main > fiber and fibers created from Lua using fiber.create() or fiber.new(), > but not for background fibers (which serve binary protocol requests). > > 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 > us from exposing it via the fiber storage. > > Follows up #4954 > --- > src/box/lua/call.c | 27 +++++++++++++++++++++++++++ > src/lib/core/fiber.h | 8 ++++++-- > 2 files changed, 33 insertions(+), 2 deletions(-) > > diff --git a/src/box/lua/call.c b/src/box/lua/call.c > index 6588ec2fa..f69d1adbe 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. > + */ > + bool has_lua_stack = fiber()->storage.lua.stack != NULL; > + if (! has_lua_stack) > + fiber()->storage.lua.stack = L; 1. According to recent code style changes, we should omit single whitespace after unary operators. > + > 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. 2. It is not just counter-intuitive. It would break Lua-born fibers, since they would suddenly loose their stack. > + */ > + if (! has_lua_stack) > + fiber()->storage.lua.stack = NULL; > + > return 0; > } > > diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h > index cd9346a55..db68fb47e 100644 > --- a/src/lib/core/fiber.h > +++ b/src/lib/core/fiber.h > @@ -496,8 +496,12 @@ struct fiber { > struct credentials *credentials; > struct txn *txn; > /** > - * Lua stack and the optional > - * fiber.storage Lua reference. > + * Optional Lua state (may be NULL). Useful as a > + * temporary Lua state to save time and resources > + * on creating it. Should not be used in other > + * fibers. > + * > + * Optional fiber.storage Lua reference. 3. You change the comment anyway, so it would be better to make it right and put the member-related comments just above the members. One comment for the stack, and separate comment for the ref. > */ > struct { > struct lua_State *stack; >