From: Igor Munkin <imun@tarantool.org> To: Alexander Turenko <alexander.turenko@tarantool.org> Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 3/3] lua: expose temporary Lua state for iproto calls Date: Wed, 1 Jul 2020 23:37:06 +0300 [thread overview] Message-ID: <20200701203706.GC5559@tarantool.org> (raw) In-Reply-To: <88f5a7e52eb203bbb959f0f811766887f89371f0.1592416673.git.alexander.turenko@tarantool.org> 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. <snipped> > 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; > } > <snipped> > -- > 2.25.0 > -- Best regards, IM
next prev parent reply other threads:[~2020-07-01 20:47 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-06-17 21:06 [Tarantool-patches] [PATCH v2 0/3] Merger's NULL defererence Alexander Turenko 2020-06-17 21:06 ` [Tarantool-patches] [PATCH v2 1/3] merger: fix NULL dereference when called via iproto Alexander Turenko 2020-06-18 22:48 ` Vladislav Shpilevoy 2020-06-19 8:50 ` Alexander Turenko 2020-06-19 23:32 ` Vladislav Shpilevoy 2020-06-21 18:28 ` Alexander Turenko 2020-07-01 20:36 ` Igor Munkin 2020-07-16 20:10 ` Alexander Turenko 2020-07-16 21:42 ` Igor Munkin 2020-07-16 22:44 ` Igor Munkin 2020-07-17 3:08 ` Alexander Turenko 2020-06-17 21:06 ` [Tarantool-patches] [PATCH v2 2/3] merger: clean fiber-local Lua stack after next() Alexander Turenko 2020-06-19 8:50 ` Alexander Turenko 2020-07-01 20:36 ` Igor Munkin 2020-07-16 20:11 ` Alexander Turenko 2020-07-16 22:07 ` Igor Munkin 2020-07-17 3:08 ` Alexander Turenko 2020-06-17 21:06 ` [Tarantool-patches] [PATCH v2 3/3] lua: expose temporary Lua state for iproto calls Alexander Turenko 2020-07-01 20:37 ` Igor Munkin [this message] 2020-07-16 20:11 ` Alexander Turenko 2020-07-16 22:33 ` Igor Munkin 2020-07-17 3:09 ` Alexander Turenko 2020-06-22 20:38 ` [Tarantool-patches] [PATCH v2 0/3] Merger's NULL defererence Vladislav Shpilevoy 2020-07-17 11:28 ` Alexander Turenko
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200701203706.GC5559@tarantool.org \ --to=imun@tarantool.org \ --cc=alexander.turenko@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v2 3/3] lua: expose temporary Lua state for iproto calls' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox