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

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sergey,

Thanks for the patch! The changes are fine, but please consider the
comments below.

On 18.06.21, Sergey Kaplun wrote:
> 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

What is this minor coroutine? To make terms more consistent in scope of
this commit message, let's define this in the first bullet and use it
everywhere. For me the best option is "port coroutine".

>    `luamp_encode()`. This encoding may raise an error on unprotected

The next bullet is recommended to be started from this sentence.

>    `port->L` coroutine. This coroutine has no protected frame on it
>    and this call should fail in pure Lua. It is forbidden to call

Ditto.

>    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

Ditto.

>    at now due to two facts. The first one is LuaJIT's support of C++

And sublist is strongly recommended for "the first" and "the second"
facts. It'll make this War and Peace part more readable.

>    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

I doubt this is monkey-patching, but rather a monkey's patch.

>    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

Well, as for me, the main problem is violating Lua coroutine practice
(it's worth to mention, that internal unwinder used on M1 is not such
flexible, so this misuse still leads to panic call), and the minor
reason is tarantool_L usage redundancy.

> 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

Some copy-pasting artefacts.

> [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

This patch resolves #6248, BTW.

> ---
> 
> 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.

At first, I would like to emphasize that we have no option for merging
or not the fix for this issue.

Re benchmarks: It's worth to mention you're measuring two performance
critical changes: <lua_insert> effect and lower GC pressure. So, it's
interesting to see the following benchmarks:
* one with disabled GC and GC stats
* one with considerable amount of elements on Lua stack, but not
  triggering stack resize (AFAIU, 200 is too much)

Here are my points:
* There is no such huge increase as a result of reducing GC pressure
* Moving 1-5 8-byte elements is neglible for performance
* Moving 200(*) elements as a result of the guest stack resize affects
  both patched and vanilla versions
* <lua_insert> measurements are affected by resizing (considering your
  perf stats)

Anyway, though these are kinda independent changes, we see no
performance degradation using both of them in a single patch, so I guess
we have no reason to worry about.

(*) I'm not sure about the exact amount of the elements to be moved.

> 
> [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,

Minor: Entries are not sorted.

>  	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.

Typo: s/proceed/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.
> + */

<snipped>

> +/**
> + * 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.

Ditto.

> + * 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.
> + */

<snipped>

> @@ -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);

Side note: I don't like this line, like all we do. The only better
approach in my head is to implement something similar to VARG frame, but
I doubt this approach have no pure Lua violations. Let's return to this
later a bit.

> +	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;
>  }
>  

<snipped>

> @@ -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,

Minor: Entries are also not sorted.

>  	};
>  
>  	for (int i = 0; i < HANDLER_MAX; i++) {
> -- 
> 2.31.0
> 

-- 
Best regards,
IM