Re: [Tarantool-patches] [PATCH] lua/utils: improve luaT_newthread performance

[Igor Munkin <imun@tarantool.org>]

Sergos,

On 24.07.20, Sergey Ostanevich wrote:
> Hi!
> 
> On 22 Jul 18:12, Igor Munkin wrote:

<snipped>

> > > 
> > > 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 <box_process_lua> 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 <tarantool_lua_utils_init>
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 <tarantool_lua_init>).

> 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