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

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.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