Re: [Tarantool-patches] [PATCH 2.X 5/7] module api: luaT_temp_luastate & luaT_release_temp_luastate

["Timur Safin" <tsafin@tarantool.org>]

: From: Igor Munkin <imun@tarantool.org>
: Subject: Re: [Tarantool-patches] [PATCH 2.X 5/7] module api:
: luaT_temp_luastate & luaT_release_temp_luastate
: 
: Vlad,
: 
: I guess there should be a question to be asked in scope of the original
: series: what performance enhancement can we obtain with such hacky
: optimization? Do we have any benchmarks? If the performance impact is
: negligible I believe these interfaces should be left internal.

That's quite good question - till some moment in the past we have used 
Internal module implementation with fiber stack optimization 
disabled, but then, when we have realized we could not avoid 
module api extension, I've moved it back to utils

Please see the older version with some code disabled due to 
inaccessible some of fiber() internal data - 
https://github.com/tsafin/tarantool-merge/commit/25c62cc01c487b2a2a6ea140340e50a0d2cb3e2a#diff-2f3bd595507f3ac1f19396c3d20c6cc0L192-L233

We could move it back to module, but I need some advice how to not lost
some speed due to not reusing existing stack?

: 
: On 29.09.20, Vladislav Shpilevoy wrote:
: > Thanks for the patch!
: >
: > I strongly don't like this export. It seems to be too internal.
: >
: > But I have no better ideas than propose to implement your own
: > lua_State cache in the merger module. It does not seem to be too
: > hard, and I don't think it would occupy much memory.
: 
: Well, such exercise looks like fun but it's better to make it once and
: provide user-friendly interfaces by so called "modulekit" to be used in
: third party modules. Anyway, using this routine seems a bit complicated
: than <lua_newstate> and <luaT_newstate> (also fully implemented via Lua
: C standart API + already exported <luaT_call>). Ergo one ought to push
: extern developers using the new one besides exporting it.
: 
: >
: > Just a simple list of lua_State objects, created by luaT_newthread
: > on demand, and put back into the list when unused. What is wrong
: > with that?
: 
: Again, how much will performance be nerfed if we simply create a new Lua
: coroutine by demand?

Looks like Alexander Turenko has already answered this question -
But I'll return back to you soon with discussion on fiber stack reuse 
details...


: 
: >
: > Adding Igor as Lua master to help with this.
: >
: > See 2 comments below.
: >
: > > diff --git a/src/lua/utils.h b/src/lua/utils.h
: > > index 9b1fe7e57..da0140076 100644
: > > --- a/src/lua/utils.h
: > > +++ b/src/lua/utils.h
: > > @@ -554,6 +554,41 @@ luaL_iscallable(lua_State *L, int idx);
: > >  struct lua_State *
: > >  luaT_newthread(struct lua_State *L);
: > >
: 
: <snipped>
: 
: >
: > 2. I would encapsulate the out parameters into some kind of
: > luaT_temp_state_ctx or something like that. Probably not even
: > opaque, but extendible.
: 
: Totally agree here. Current interface is too fragile and tricky and IMHO
: has to be redesigned/reworked prior to be exported for public usage.
: 
: >
: > Also maybe worth grouping these methods by using a common prefix:
: >
: > 	luaT_temp_luastate_get
: > 	luaT_temp_luastate_release
: >
: > > + *
: > > + * Return NULL and set a diag at failure.
: > > + */
: > > +struct lua_State *
: > > +luaT_temp_luastate(int *coro_ref, int *top);
: > > +
: > > +/**
: > > + * Release a temporary Lua state.
: > > + *
: > > + * It complements `luaT_temp_luastate()`.
: > > + */
: > > +void
: > > +luaT_release_temp_luastate(struct lua_State *L, int coro_ref, int
: top);
: > > +
: > >  /** \endcond public */
: > >
: > >  /**
: > >
: 
: Finally, I see no strict arguments *for* exporting these routines, and
: have many doubts regarding its current design and *obligatoriness*. Yes,
: they may be useful inside the platform but do they look like ones user
: must have? I tend to "no" for now.

Yup, time to return back and polish prior local to module design.

: 
: --
: Best regards,
: IM