Re: [Tarantool-patches] [PATCH] lua: refactor port_lua_do_dump and encode_lua_call

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Also, add the branch for 1.10:
https://github.com/tarantool/tarantool/tree/skaplun/gh-noticket-refactor-lua-call-1.10

The difference is that the port structure itself is pushed instead ctx
structure on the stack.

The patch is the following:
===================================================================
commit 59eafb8e6d8f41de06bdb28b930c9ff1d3fbe63e
Author: Sergey Kaplun <skaplun@tarantool.org>
Date:   Fri Jun 18 18:28:30 2021 +0300

    lua: refactor port_lua_do_dump and encode_lua_call

    (cherry picked from commit 434d31bdb52d80384af55acd2c3a637e5154e257)

    The old code flow was the following:

    1) `struct port_lua` given to `port_lua_do_dump()` has Lua stack with
       arguments to encode to MessagePack.

    2) The main coroutine `tarantool_L` is used to call `encode_lua_call()`
       or `encode_lua_call_16`() via `lua_cpcall()`.

    3) Objects on port coroutine are encoded via `luamp_encode()` or
       `luamp_encode_call16()`.

    4) This encoding may raise an error on unprotected `port->L` coroutine.
       This coroutine has no protected frame on it and this call should fail
       in pure Lua.

    Calling anything on unprotected coroutine is not allowed in Lua [1]:

    | If an error happens outside any protected environment, Lua calls a
    | panic function

    Lua 5.1 sets protection only for specific lua_State [2] and calls a
    panic function if we raise an error on unprotected lua_State [3].

    Nevertheless, no panic occurs now due to two facts:
    * The first one is LuaJIT's support of C++ exception handling [4] that
      allows to raise an error in Lua and catch it in C++ or vice versa. But
      documentation still doesn't allow raising errors on unprotected
      coroutines (at least we must use try-catch block).
    * The second one is the patch made in LuaJIT to restore currently
      executed coroutine, when C function or fast function raises an
      error [5][6] (see the related issue here [7][8]).

    For these reasons, when an error occurs, the unwinder searches and finds
    the C-protected stack frame from the `lua_cpcall()` for `tarantool_L`
    coroutine and unwinds until that point (without aforementioned patches
    LuaJIT just calls a panic function and exits).

    If an error is raised, and `lua_cpcall()` returns not `LUA_OK`, then the
    error from `port->L` coroutine is converted into a Tarantool error and a
    diagnostic is set.

    Such auxiliary usage of `tarantool_L` is not idiomatic for Lua.
    Internal unwinder used on M1 is not such flexible, so such misuse leads
    to panic call. Also the `tarantool_L` usage is redundant. So this patch
    drops it and uses only port coroutine instead with `lua_pcall()`.

    Functions to encode are saved to the `LUA_REGISTRY` table to
    reduce GC pressure, like it is done for other handlers [9].

    [1]: https://www.lua.org/manual/5.2/manual.html#4.6
    [2]: https://www.lua.org/source/5.1/lstate.h.html#lua_State
    [3]: https://www.lua.org/source/5.1/ldo.c.html#luaD_throw
    [4]: https://luajit.org/extensions.html#exceptions
    [5]: https://github.com/tarantool/luajit/commit/ed412cd9f55fe87fd32a69c86e1732690fc5c1b0
    [6]: https://github.com/tarantool/luajit/commit/97699d9ee2467389b6aea21a098e38aff3469b5f
    [7]: https://github.com/tarantool/tarantool/issues/1516
    [8]: https://www.freelists.org/post/luajit/Issue-with-PCALL-in-21
    [9]: https://github.com/tarantool/tarantool/commit/e88c0d21ab765d4c53bed2437c49d77b3ffe4216

    Closes #6248
    Closes #4617

diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 593317265..f173d97b7 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -53,6 +53,8 @@
  */
 enum handlers {
 	HANDLER_CALL,
+	HANDLER_ENCODE_CALL,
+	HANDLER_ENCODE_CALL_16,
 	HANDLER_EVAL,
 	HANDLER_MAX,
 };
@@ -347,10 +349,26 @@ execute_lua_eval(lua_State *L)
 	return lua_gettop(L);
 }
 
+
+/**
+ * Encode call results to msgpack from Lua stack.
+ * Lua stack has the following structure -- the last element is
+ * lightuserdata pointer to encode_lua_ctx, all other values are
+ * arguments to process.
+ * The function encodes all given Lua objects to msgpack stream
+ * from context, sets port's size and returns no value on the Lua
+ * stack.
+ * XXX: This function *MUST* be called under lua_pcall(), because
+ * luamp_encode() may raise an error.
+ */
 static int
 encode_lua_call(lua_State *L)
 {
+	assert(lua_islightuserdata(L, -1));
 	struct port_lua *port = (struct port_lua *) lua_topointer(L, -1);
+	assert(port->L == L);
+	/* Delete port from the stack. */
+	lua_pop(L, 1);
 	/*
 	 * Add all elements from Lua stack to the buffer.
 	 *
@@ -361,18 +379,33 @@ encode_lua_call(lua_State *L)
 		      luamp_error, port->L);
 
 	struct luaL_serializer *cfg = luaL_msgpack_default;
-	int size = lua_gettop(port->L);
+	const int size = lua_gettop(L);
 	for (int i = 1; i <= size; ++i)
-		luamp_encode(port->L, cfg, &stream, i);
+		luamp_encode(L, cfg, &stream, i);
 	port->size = size;
 	mpstream_flush(&stream);
 	return 0;
 }
 
+/**
+ * Encode call_16 results to msgpack from Lua stack.
+ * Lua stack has the following structure -- the last element is
+ * lightuserdata pointer to encode_lua_ctx, all other values are
+ * arguments to process.
+ * The function encodes all given Lua objects to msgpack stream
+ * from context, sets port's size and returns no value on the Lua
+ * stack.
+ * XXX: This function *MUST* be called under lua_pcall(), because
+ * luamp_encode() may raise an error.
+ */
 static int
 encode_lua_call_16(lua_State *L)
 {
+	assert(lua_islightuserdata(L, -1));
 	struct port_lua *port = (struct port_lua *) lua_topointer(L, -1);
+	assert(port->L == L);
+	/* Delete port from the stack. */
+	lua_pop(L, 1);
 	/*
 	 * Add all elements from Lua stack to the buffer.
 	 *
@@ -383,42 +416,45 @@ encode_lua_call_16(lua_State *L)
 		      luamp_error, port->L);
 
 	struct luaL_serializer *cfg = luaL_msgpack_default;
-	port->size = luamp_encode_call_16(port->L, cfg, &stream);
+	port->size = luamp_encode_call_16(L, cfg, &stream);
 	mpstream_flush(&stream);
 	return 0;
 }
 
 static inline int
-port_lua_do_dump(struct port *base, struct obuf *out, lua_CFunction handler)
+port_lua_do_dump(struct port *base, struct obuf *out, enum handlers handler)
 {
 	struct port_lua *port = (struct port_lua *)base;
 	assert(port->vtab == &port_lua_vtab);
 	/* Use port to pass arguments to encoder quickly. */
 	port->out = out;
+	lua_State *L = port->L;
 	/*
-	 * Use the same global state, assuming the encoder doesn't
-	 * yield.
+	 * At the moment Lua stack holds only values to encode.
+	 * Insert corresponding encoder to the bottom and push
+	 * encode context as lightuserdata to the top.
 	 */
-	struct lua_State *L = tarantool_L;
-	int top = lua_gettop(L);
-	if (lua_cpcall(L, handler, port) != 0) {
-		luaT_toerror(port->L);
+	const int size = lua_gettop(L);
+	lua_rawgeti(L, LUA_REGISTRYINDEX, execute_lua_refs[handler]);
+	assert(lua_isfunction(L, -1) && lua_iscfunction(L, -1));
+	lua_insert(L, 1);
+	lua_pushlightuserdata(L, port);
+	/* nargs -- all arguments + lightuserdata. */
+	if (luaT_call(L, size + 1, 0) != 0)
 		return -1;
-	}
-	lua_settop(L, top);
 	return port->size;
 }
 
 static int
 port_lua_dump(struct port *base, struct obuf *out)
 {
-	return port_lua_do_dump(base, out, encode_lua_call);
+	return port_lua_do_dump(base, out, HANDLER_ENCODE_CALL);
 }
 
 static int
 port_lua_dump_16(struct port *base, struct obuf *out)
 {
-	return port_lua_do_dump(base, out, encode_lua_call_16);
+	return port_lua_do_dump(base, out, HANDLER_ENCODE_CALL_16);
 }
 
 static void
@@ -499,6 +535,8 @@ box_lua_call_init(struct lua_State *L)
 
 	lua_CFunction handles[] = {
 		[HANDLER_CALL] = execute_lua_call,
+		[HANDLER_ENCODE_CALL] = encode_lua_call,
+		[HANDLER_ENCODE_CALL_16] = encode_lua_call_16,
 		[HANDLER_EVAL] = execute_lua_eval,
 	};
 
===================================================================

-- 
Best regards,
Sergey Kaplun