Re: [Tarantool-patches] [PATCH v2 3/3] lua: expose temporary Lua state for iproto calls

[Alexander Turenko <alexander.turenko@tarantool.org>]

> > 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.