From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 347E2469719 for ; Tue, 29 Sep 2020 18:20:37 +0300 (MSK) Date: Tue, 29 Sep 2020 18:10:02 +0300 From: Igor Munkin Message-ID: <20200929151002.GY18920@tarantool.org> References: <6fc62e3a3935978b650fd6d1dcbdef9a5353a1fb.1600955781.git.tsafin@tarantool.org> <40b631b2-c660-32f7-c421-9770d5d06de4@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <40b631b2-c660-32f7-c421-9770d5d06de4@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 2.X 5/7] module api: luaT_temp_luastate & luaT_release_temp_luastate List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org, alexander.turenko@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 and (also fully implemented via Lua C standart API + already exported ). 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); > > > > 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