Tarantool development patches archive
 help / color / mirror / Atom feed
From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Kaplun <skaplun@tarantool.org>
Cc: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>,
	tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] lua: refactor port_lua_do_dump and encode_lua_call
Date: Sat, 14 Aug 2021 13:16:41 +0300
Message-ID: <20210814101641.GP27855@tarantool.org> (raw)
In-Reply-To: <20210618181416.25454-1-skaplun@tarantool.org>

Sergey,

I've checked the patch into 1.10, 2.7, 2.8 and master.

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
>    `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(-)
> 

<snipped>

> -- 
> 2.31.0
> 

-- 
Best regards,
IM

      parent reply	other threads:[~2021-08-14 10:40 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-18 18:14 Sergey Kaplun via Tarantool-patches
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 [this message]

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=20210814101641.GP27855@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