[Tarantool-patches] [PATCH] lua: refactor port_lua_do_dump and encode_lua_call
imun at tarantool.org
Thu Aug 12 20:35:16 MSK 2021
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!
> 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
> 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 :
> | If an error happens outside any protected environment, Lua calls a
> | panic function
> Lua 5.1 sets protection only for specific lua_State  and calls a
> panic function if we raise an error on unprotected lua_State .
> Nevertheless, no panic occurs now due to two facts:
> * The first one is LuaJIT's support of C++ exception handling  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  (see the related issue here ).
> 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).
> 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
| 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 .
> : https://www.lua.org/manual/5.2/manual.html#4.6
> : https://www.lua.org/source/5.1/lstate.h.html#lua_State
> : https://www.lua.org/source/5.1/ldo.c.html#luaD_throw
> : https://luajit.org/extensions.html#exceptions
> : https://github.com/tarantool/luajit/commit/ed412cd9f55fe87fd32a69c86e1732690fc5c1b0
> : https://github.com/tarantool/luajit/commit/97699d9ee2467389b6aea21a098e38aff3469b5f
> : https://github.com/tarantool/tarantool/issues/1516
> : https://www.freelists.org/post/luajit/Issue-with-PCALL-in-21
> : 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 .
> > >
> > > 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
> 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:
> Encode mul: 4895280 mcs, 20.4 K ps
> Encode mul: 4896076 mcs, 20.4 K ps
> With GC:
> Encode mul: 5123580 mcs, 19.5 K ps
> 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.
> Best regards,
> Sergey Kaplun
More information about the Tarantool-patches