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>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] lua: refactor port_lua_do_dump and encode_lua_call
Date: Tue, 22 Jun 2021 16:38:18 +0300
Message-ID: <YNHnytU/zbRd1LV7@root> (raw)
In-Reply-To: <67687e87-fb0e-1477-ac76-5fa9b04b9e3a@tarantool.org>

Hi! Thanks for the review!

On 21.06.21, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> > 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
> > @@ -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);
> 
> If I remember correctly, this leads to moving all the existing
> stack elements forward. Which might be expensive. I know from
> Vlad Grubov's words that they have code with hundreds of values in
> multireturn from stored functions. Did you bench what happens when
> the Lua coroutine contains a lot of values? In the bench by the
> link above I see only 1-element array and map. Size of the array
> and map does not matter though. Only multireturn is interesting
> here. Like 'return 1, 2, 3, ...'.

I've added this benchmark (200 numbers to return) to the gist.
Local results for the bench:

Master:
| Encode mul: 237900953 mcs, 12.6 K ps

My branch:
| Encode mul: 235735350 mcs, 12.7 K ps

`luamp_encode()` has the biggest impact in `port_do_lua_dump()`
(`lua_insert()` costs ~0.1% of the whole runtime).

-- 
Best regards,
Sergey Kaplun

  reply	other threads:[~2021-06-22 13:39 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 [this message]
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=YNHnytU/zbRd1LV7@root \
    --to=tarantool-patches@dev.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