From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp15.mail.ru (smtp15.mail.ru [94.100.176.133]) (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 0E3EB445321 for ; Thu, 16 Jul 2020 23:12:24 +0300 (MSK) Date: Thu, 16 Jul 2020 23:11:37 +0300 From: Alexander Turenko Message-ID: <20200716201137.3qvizw3mri64kayv@tkn_work_nb> References: <88f5a7e52eb203bbb959f0f811766887f89371f0.1592416673.git.alexander.turenko@tarantool.org> <20200701203706.GC5559@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200701203706.GC5559@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: Igor Munkin Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy > > 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/. According to [1] the word may be considered as an adjective and so using it as 'present' here is technically correct. This article gives the following difference in meanging: | "As presented" (verb) connotes deliberate placement. "As present" | (adjective) just means it's there. 'It is there' is what I want to express here, so 'present' looks as the better choice here. Does not I miss something about grammar here? [1]: https://www.instructionalsolutions.com/blog/bid/102954/Tables-in-Report-Writing-Presented-or-Present > > 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/. Thanks! Fixed. > > 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? Sure. | tarantool> box.cfg{} | tarantool> echo = function(...) return ... end | tarantool> box.schema.func.create('echo') | tarantool> box.schema.func.call('echo', {1, 2, 3}) I added the assert and verified it just in case: | diff --git a/src/box/lua/call.c b/src/box/lua/call.c | index 0315e720c..0221ffd2d 100644 | --- a/src/box/lua/call.c | +++ b/src/box/lua/call.c | @@ -561,6 +561,7 @@ box_process_lua(enum handlers handler, struct execute_lua_ctx *ctx, | * and use this one. | */ | bool has_lua_stack = fiber()->storage.lua.stack != NULL; | + assert(fiber()->storage.lua.stack == NULL); | if (!has_lua_stack) | fiber()->storage.lua.stack = L; The assert fails after the steps above. (But even if it would not be possible, I would write the code this way to don't lean on not-so-obvious details that may be changed in a future.) > Are those conditions below are strictly required by the current > implementation? When the fiber-local Lua state is present it should not be changed or zapped by the function. I don't know whether it would lead to some negative behaviour changes, but it would be at least counter-intuitive. There is the comment on the topic (I left it cited below). > > + * 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; BTW, 'present' is here again. Don't know what form is better here, but left it as is now.