Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>,
	Igor Munkin <imun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: [Tarantool-patches] [PATCH] lua: refactor port_lua_do_dump and encode_lua_call
Date: Fri, 18 Jun 2021 21:14:16 +0300
Message-ID: <20210618181416.25454-1-skaplun@tarantool.org> (raw)

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 with `lua_cpcall()` to call
   `encode_lua_call_16()` or `encode_lua_call()`
3) Objects on minor coroutine are encoded via `luamp_encode_call16()` or
   `luamp_encode()`. 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. It is forbidden to call
   anything on unprotected coroutine [1] (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]). Netherless, there is no panic
   at 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
   permit errors on unprotected coroutines (at least we must set
   try-catch block). The second one is double monkey-patching of LuaJIT
   to restore currently executed coroutine, when C function or fast
   function raises an error [5][6] (see 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 the
   `tarantool_L` coroutine and unwinds until that point (without
   aforementioned patches LuaJIT just calls a panic function and exit).
4) 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.

The auxiliary usage of `tarantool_L` coroutine is redundant and doesn't
respect Lua idiomatic of usage. So this patch drops it and uses only
minor coroutine instead with `lua_pcall()`.

Functions to encode are saved as entrance in 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
> If an error happens outside any protected environment, Lua calls a
> panic function
[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
---

Branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-noticket-refactor-lua-call
See the benchmarks sources here [1].

Before patch:
| Encode map: 189851357 mcs, 15.8 K ps
| Encode seq: 187926351 mcs, 16.0 K ps
| Encode str: 185451675 mcs, 16.2 K ps
| Encode dig: 184833396 mcs, 16.2 K ps

After patch:
| Encode map: 187814261 mcs, 16.0 K ps
| Encode seq: 183755028 mcs, 16.3 K ps
| Encode str: 181571626 mcs, 16.5 K ps
| Encode dig: 181572998 mcs, 16.5 K ps

Looks like the perf doesn't degrade at least.

[1]: https://gist.github.com/Buristan/3e6d6bf2c722874bec55a8c5a44b98f3

 src/box/lua/call.c | 71 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 56 insertions(+), 15 deletions(-)

diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 0315e720c..3b2572096 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -61,6 +61,8 @@ enum handlers {
 	HANDLER_CALL,
 	HANDLER_CALL_BY_REF,
 	HANDLER_EVAL,
+	HANDLER_ENCODE_CALL,
+	HANDLER_ENCODE_CALL_16,
 	HANDLER_MAX,
 };
 
@@ -400,11 +402,26 @@ struct encode_lua_ctx {
 	struct mpstream *stream;
 };
 
+/**
+ * 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 proceed.
+ * 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 encode_lua_ctx *ctx =
-		(struct encode_lua_ctx *) lua_topointer(L, 1);
+		(struct encode_lua_ctx *) lua_topointer(L, -1);
+	assert(ctx->port->L == L);
+	/* Delete ctx from the stack. */
+	lua_pop(L, 1);
 	/*
 	 * Add all elements from Lua stack to the buffer.
 	 *
@@ -413,33 +430,48 @@ encode_lua_call(lua_State *L)
 	struct luaL_serializer *cfg = luaL_msgpack_default;
 	const struct serializer_opts *opts =
 		&current_session()->meta.serializer_opts;
-	int size = lua_gettop(ctx->port->L);
+	const int size = lua_gettop(L);
 	for (int i = 1; i <= size; ++i)
-		luamp_encode(ctx->port->L, cfg, opts, ctx->stream, i);
+		luamp_encode(L, cfg, opts, ctx->stream, i);
 	ctx->port->size = size;
 	mpstream_flush(ctx->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 proceed.
+ * 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 encode_lua_ctx *ctx =
-		(struct encode_lua_ctx *) lua_topointer(L, 1);
+		(struct encode_lua_ctx *) lua_topointer(L, -1);
+	/* Delete ctx from the stack. */
+	lua_pop(L, 1);
+	assert(ctx->port->L == L);
 	/*
 	 * Add all elements from Lua stack to the buffer.
 	 *
 	 * TODO: forbid explicit yield from __serialize or __index here
 	 */
 	struct luaL_serializer *cfg = luaL_msgpack_default;
-	ctx->port->size = luamp_encode_call_16(ctx->port->L, cfg, ctx->stream);
+	ctx->port->size = luamp_encode_call_16(L, cfg, ctx->stream);
 	mpstream_flush(ctx->stream);
 	return 0;
 }
 
 static inline int
 port_lua_do_dump(struct port *base, struct mpstream *stream,
-		 lua_CFunction handler)
+		 enum handlers handler)
 {
 	struct port_lua *port = (struct port_lua *) base;
 	assert(port->vtab == &port_lua_vtab);
@@ -450,13 +482,20 @@ port_lua_do_dump(struct port *base, struct mpstream *stream,
 	struct encode_lua_ctx ctx;
 	ctx.port = port;
 	ctx.stream = stream;
-	struct lua_State *L = tarantool_L;
-	int top = lua_gettop(L);
-	if (lua_cpcall(L, handler, &ctx) != 0) {
-		luaT_toerror(port->L);
+	lua_State *L = port->L;
+	/*
+	 * 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.
+	 */
+	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, &ctx);
+	/* nargs -- all arguments + lightuserdata. */
+	if (luaT_call(L, size + 1, 0) != 0)
 		return -1;
-	}
-	lua_settop(L, top);
 	return port->size;
 }
 
@@ -467,7 +506,7 @@ port_lua_dump(struct port *base, struct obuf *out)
 	struct mpstream stream;
 	mpstream_init(&stream, out, obuf_reserve_cb, obuf_alloc_cb,
 		      luamp_error, port->L);
-	return port_lua_do_dump(base, &stream, encode_lua_call);
+	return port_lua_do_dump(base, &stream, HANDLER_ENCODE_CALL);
 }
 
 static int
@@ -477,7 +516,7 @@ port_lua_dump_16(struct port *base, struct obuf *out)
 	struct mpstream stream;
 	mpstream_init(&stream, out, obuf_reserve_cb, obuf_alloc_cb,
 		      luamp_error, port->L);
-	return port_lua_do_dump(base, &stream, encode_lua_call_16);
+	return port_lua_do_dump(base, &stream, HANDLER_ENCODE_CALL_16);
 }
 
 static void
@@ -501,7 +540,7 @@ port_lua_get_msgpack(struct port *base, uint32_t *size)
 	mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
 		      luamp_error, port->L);
 	mpstream_encode_array(&stream, lua_gettop(port->L));
-	int rc = port_lua_do_dump(base, &stream, encode_lua_call);
+	int rc = port_lua_do_dump(base, &stream, HANDLER_ENCODE_CALL);
 	if (rc < 0) {
 		region_truncate(region, region_svp);
 		return NULL;
@@ -1049,6 +1088,8 @@ box_lua_call_init(struct lua_State *L)
 		[HANDLER_CALL] = execute_lua_call,
 		[HANDLER_CALL_BY_REF] = execute_lua_call_by_ref,
 		[HANDLER_EVAL] = execute_lua_eval,
+		[HANDLER_ENCODE_CALL] = encode_lua_call,
+		[HANDLER_ENCODE_CALL_16] = encode_lua_call_16,
 	};
 
 	for (int i = 0; i < HANDLER_MAX; i++) {
-- 
2.31.0


             reply	other threads:[~2021-06-18 18:15 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-18 18:14 Sergey Kaplun via Tarantool-patches [this message]
2021-06-21 20:36 ` Vladislav Shpilevoy via Tarantool-patches
2021-06-22 13:38   ` Sergey Kaplun via Tarantool-patches
2021-06-24 20:00     ` Vladislav Shpilevoy via Tarantool-patches
2021-06-29  7:07       ` Sergey Kaplun via Tarantool-patches
2021-08-01 12:34 ` Igor Munkin via Tarantool-patches
2021-08-02 14:25   ` Sergey Kaplun via Tarantool-patches
2021-08-12 17:35     ` Igor Munkin via Tarantool-patches
2021-08-13  7:30       ` Sergey Kaplun via Tarantool-patches
2021-08-13  7:41         ` Vitaliia Ioffe via Tarantool-patches
2021-08-13  9:27         ` Sergey Kaplun via Tarantool-patches
2021-08-04 22:29 ` Vladislav Shpilevoy via Tarantool-patches
2021-08-14 10:16 ` Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210618181416.25454-1-skaplun@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imun@tarantool.org \
    --cc=skaplun@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git