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 fixes! LGTM, with a several typos in the commit message
mentioned below. Moreover, please rebase your branch on the current
master to check nothing is broken.

On 02.08.21, Sergey Kaplun wrote:
> Hi, Igor!
> 
> Thanks for the review!
> 

<snipped>

> 
> The new commit message is the following:
> 
> ===================================================================
> lua: refactor port_lua_do_dump and encode_lua_call
> 
> 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 exit).

Typo: s/exit/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.
> 
> The auxiliary usage of `tarantool_L` coroutine doesn't respect Lua
> idiomatic of usage. Internal unwinder used on M1 is not such flexible,

Typo: Too much "usage", so I propose the following wording for the
previous sentence:
| Such auxiliary usage of `tarantool_L` is not idiomatic for Lua.

> so such misuse leads to panic call. Also the `tarantool_L` usage is
> redundant. So this patch drops it and uses only minor coroutine instead

Typo: Again, not minor coroutine, but port coroutine (as we agreed in
the previous review).

> with `lua_pcall()`.
> 
> Functions to encode are saved as entrance in the `LUA_REGISTRY` table to

Typo: s/saved as entrance in/saved 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
> ===================================================================
> 
> > > ---
> > > 
> > > 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
> 
> Here the results with disabled GC:
> Before patch:
> 
> Encode map: 4679394 mcs, 21.4 K ps
> Encode seq: 4559824 mcs, 21.9 K ps
> Encode str: 4574213 mcs, 21.9 K ps
> Encode dig: 4595043 mcs, 21.8 K ps
> Encode mul: 5978444 mcs, 16.7 K ps
> 
> After:
> 
> Encode map: 4739110 mcs, 21.1 K ps
> Encode seq: 4528261 mcs, 22.1 K ps
> Encode str: 4576910 mcs, 21.8 K ps
> Encode dig: 4506142 mcs, 22.2 K ps
> Encode mul: 6016659 mcs, 16.6 K ps
> 
> I suppose, that values are almost the same, at least within the margin
> of error.
> Note: I reduced amount of iterations 30 times. So inaccuracy increased.
> 
> > * one with considerable amount of elements on Lua stack, but not
> >   triggering stack resize (AFAIU, 200 is too much)
> 
> Tried with 40 items on the stack:
> 
> Without GC:
> 
> Master:
> Encode mul: 4895280 mcs, 20.4 K ps
> 
> Branch:
> Encode mul: 4896076 mcs, 20.4 K ps
> 
> With GC:
> 
> Master:
> Encode mul: 5123580 mcs, 19.5 K ps
> 
> Branch:
> Encode mul: 5050863 mcs, 19.8 K ps
> 
> Seems pretty equal too.

Mystery. Anyway, the current performance is not lost and this is great!

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

<snipped>

> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM