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

[Igor Munkin <imun@tarantool.org>]

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.

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?

> 
> 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.

-- 
Best regards,
IM