From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (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 D0303469719 for ; Fri, 2 Oct 2020 15:24:32 +0300 (MSK) From: "Timur Safin" References: <6fc62e3a3935978b650fd6d1dcbdef9a5353a1fb.1600955781.git.tsafin@tarantool.org> <40b631b2-c660-32f7-c421-9770d5d06de4@tarantool.org> <20200929151002.GY18920@tarantool.org> In-Reply-To: <20200929151002.GY18920@tarantool.org> Date: Fri, 2 Oct 2020 15:24:31 +0300 Message-ID: <09e001d698b6$fb3dced0$f1b96c70$@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Content-Language: ru 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: 'Igor Munkin' , 'Vladislav Shpilevoy' Cc: tarantool-patches@dev.tarantool.org, alexander.turenko@tarantool.org : From: Igor Munkin : Subject: Re: [Tarantool-patches] [PATCH 2.X 5/7] module api: : luaT_temp_luastate & luaT_release_temp_luastate :=20 : Vlad, :=20 : 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=20 Internal module implementation with fiber stack optimization=20 disabled, but then, when we have realized we could not avoid=20 module api extension, I've moved it back to utils Please see the older version with some code disabled due to=20 inaccessible some of fiber() internal data -=20 https://github.com/tsafin/tarantool-merge/commit/25c62cc01c487b2a2a6ea140= 340e50a0d2cb3e2a#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? :=20 : 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. :=20 : 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. :=20 : > : > 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? :=20 : 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=20 details... :=20 : > : > 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); : > > :=20 : :=20 : > : > 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. :=20 : Totally agree here. Current interface is too fragile and tricky and = IMHO : has to be redesigned/reworked prior to be exported for public usage. :=20 : > : > 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 */ : > > : > > /** : > > :=20 : 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. :=20 : -- : Best regards, : IM