From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 93A36445320 for ; Fri, 24 Jul 2020 22:28:33 +0300 (MSK) Date: Fri, 24 Jul 2020 22:18:14 +0300 From: Igor Munkin Message-ID: <20200724191814.GS18920@tarantool.org> References: <20200722113055.GB894@tarantool.org> <20200722151209.GP18920@tarantool.org> <20200724161534.GE894@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200724161534.GE894@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] lua/utils: improve luaT_newthread performance List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Ostanevich Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy Sergos, On 24.07.20, Sergey Ostanevich wrote: > Hi! > > On 22 Jul 18:12, Igor Munkin wrote: > > > > > > I wonder if it can be done in a more safe way in luaT_newthread() so > > > that if the refernece is not initialized then initialize it, rather > > > assert in debug and - I suppose - fail with creation in release? I see > > > an overhead for condition here and making even bigger indirect > > > machinery for the wrappers itself won't bring a lot either. > > > > Frankly speaking, it looks an overkill to me. What are your exact > > concerns? I added those asserts mostly for self-check. This is a hot > > path in call/eval request handling, fiber creation and stored procedures > > calls, so Debug mode testing fails if the problem occurs. This approach > > is the similar to one[1] but there are no such checks. > > This static reference can't be unintentionally changed outside this > > translation unit, so its usage is well-localized. My arguments are not > > about performance but sanity. Do you see any flaws in this design? > > > > The change is not the issue, the missing initialization is. Since The reference is initialized in scope of function. If it was not called other vital Tarantool internals also would be missing (e.g. box.NULL, ibuf). > luaT_newthread() is external - there _could_ be some circumstances that > it will be called w/o init done. I believe the lua_rawgeti() should luaT_newthread can't be called prior to utils initialization, since there is no lua_State to be passed to it (AFAICS all Lua-dependent subsystems are initialized in scope of ). > fail, so my question if it will report a comprehensible error? If it Everything is fine with Debug mode (considering several asserts I added), so here are several examples for Release mode: 1. Reference is not initialized on Tarantool startup: ================================================================================ diff --git a/src/lua/utils.c b/src/lua/utils.c index af114b0a2..6d422eef3 100644 --- a/src/lua/utils.c +++ b/src/lua/utils.c @@ -1305,6 +1305,5 @@ tarantool_lua_utils_init(struct lua_State *L) assert(CTID_UUID != 0); lua_pushcfunction(L, luaT_newthread_wrapper); - luaT_newthread_ref = luaL_ref(L, LUA_REGISTRYINDEX); return 0; } ================================================================================ | $ ./src/tarantool | PANIC: unprotected error in call to Lua API (?) 2. Reference is corrupted while Tarantool is running: ================================================================================ diff --git a/src/lua/utils.c b/src/lua/utils.c index af114b0a2..be56ba12f 100644 --- a/src/lua/utils.c +++ b/src/lua/utils.c @@ -998,6 +998,7 @@ luaL_toint64(struct lua_State *L, int idx) int luaT_toerror(lua_State *L) { + luaT_newthread_ref = LUA_NOREF; struct error *e = luaL_iserror(L, -1); if (e != NULL) { /* Re-throw original error */ ================================================================================ | $ cat ~/ref.lua | local fiber = require('fiber') | | fiber.create(function() a() end) | fiber.create(function() a() end) | $ ./src/tarantool ~/ref.lua | LuajitError: /home/imun/ref.lua:3: attempt to call global 'a' (a nil value) | LuajitError: attempt to call a nil value | fatal error, exiting the event loop > will - it should be enough, so I enforce my LGTM once more. We're testing lots of various builds and I believe our testing system covers most of the cases occurring in real life. > > Regards, > Sergos > > -- Best regards, IM