[Tarantool-patches] [PATCH v2 2/3] merger: clean fiber-local Lua stack after next()

[Igor Munkin]

Sasha,

Thanks for your comments, LGTM!

On 16.07.20, Alexander Turenko wrote:
> > > Now those functions may be called only from Lua and if the fiber-local
> > > Lua state is present it is the same as one that is passed to a Lua/C
> > 
> > Typo: s/present/presented/.
> 
> Cited from [1]:
> 
>  | "As presented" (verb) connotes deliberate placement. "As present"
>  | (adjective) just means it's there.
> 
> 'It is there' meanging fits better here, IMHO.
> 
> Isn't I miss something about grammar here?

I guess no, thanks for clarification!

> 
> [1]: https://www.instructionalsolutions.com/blog/bid/102954/Tables-in-Report-Writing-Presented-or-Present
> 

<snipped>

> > > @@ -206,14 +216,10 @@ luaT_temp_luastate(int *coro_ref)
> > >   * It is the other half of `luaT_temp_luastate()`.
> > >   */
> > >  static void
> > > -luaT_release_temp_luastate(int coro_ref)
> > > +luaT_release_temp_luastate(struct lua_State *L, int coro_ref, int top)
> > >  {
> > > -	/*
> > > -	 * FIXME: The reusable fiber-local Lua state is not
> > > -	 * unreferenced here (coro_ref == LUA_REFNIL), but
> > > -	 * it must be truncated to its past top to prevent
> > > -	 * stack overflow.
> > > -	 */
> > > +	if (top >= 0)
> > > +		lua_settop(L, top);
> > >  	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref);
> > 
> > Minor: You can just either restore top value for fiber-local Lua state
> > or unreference Lua coroutine without restoring a pointer to its stack
> > top slot. As a result you need to preserve the top value only for the
> > first case (i.e. when the coro_ref is LUA_NOREF) and ignore the value
> > for all other cases.
> 
> Are you propose the following?
> 
>  | if (top >= 0)
>  |         lua_settop(L, top);
>  | else
>  |         luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref);

Kinda, but with <coro_ref> in condition:
| if (coro_ref == LUA_NOREF)
| 	lua_settop(L, top);
| else
| 	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref);

Feel free to ignore this comment if it doesn't looks logical to you.

> 

<snipped>

> 
> Vlad even proposed to drop `top >= 0`, which works in fact, but Lua
> Reference Manual does not guarantee it (it was discussed within this
> mailing thread).

I see Reference Manual allows any integer number to be passed as <idx>
but I agree that behaviour for negative values are not clear from it.
JFYI, LuaJIT works the same way here vanilla Lua 5.1 does. So I'm much
closer to Vlad's opinion here, but doesn't insist on such <lua_settop>
usage, since it might not be clear from Manual.

> 
> > > +static int
> > > +lbox_check_merge_source_call_next(struct lua_State *L)
> > > +{
> > > +	assert(lua_gettop(L) == 1);
> > > +
> > > +	/*
> > > +	 * Ensure that there is reusable temporary Lua stack.
> > > +	 *
> > > +	 * Note: It may be the same as L (and usually do).
> > 
> > Minor: It would be nice to mention (at least for inquisitive persons) a
> > case when <temporary_L> differs from the given <L> for Lua-born fibers.
> 
> No-no, it is always so for a Lua born fiber. It seems I should reword
> the comment in a more strict way and explain why I use <temporary_L>
> explicitly despite the fact that it is always the same as <L>.
> 
> The new comment:
> 
>  | /*
>  |  * Ensure that there is a reusable temporary Lua stack.
>  |  *
>  |  * Note: It is the same as <L> for a Lua born fiber (at
>  |  * least at the moment of writing), but it is the
>  |  * implementation detail and the test looks more clean
>  |  * when we don't lean on this fact.
>  |  */

OK, now it's clear, thanks!

-- 
Best regards,
IM