From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (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 A8CD7445320 for ; Fri, 17 Jul 2020 06:09:37 +0300 (MSK) Date: Fri, 17 Jul 2020 06:08:51 +0300 From: Alexander Turenko Message-ID: <20200717030851.r2zlxwgbd4nnzcjq@tkn_work_nb> References: <20200701203650.GB5559@tarantool.org> <20200716201104.xkexzk2tpgtrgth3@tkn_work_nb> <20200716220720.GB18920@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200716220720.GB18920@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 2/3] merger: clean fiber-local Lua stack after next() 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 > > > > @@ -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 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. To be honest, I would prefer to keep the current way: luaT_temp_luastate() fills and and they separately control whether lua_settop() and luaL_unref() will be called / will have an effect in luaT_release_temp_luastate(). It is quite straightforward. But I understand that when lua_settop() has the check, but luaL_unref() has no, it looks a bit lopsided and it is quite natural to want to make it visually balanced. > > 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 > 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 > usage, since it might not be clear from Manual. I'm quite sure we just have no such guarantee. I disagree with the point that it is guaranteed, but in some unclear way. No. I performed an attempt to find what exactly guarantees this behaviour and found nothing. I described it in [1]. Since it unlikely will be changed, we can remove the check and leave a comment to describe the behaviour we lean on. But the check just shorter and cleaner, I don't see a reason to replace it with the comment. [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-June/017750.html