From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-f195.google.com (mail-lj1-f195.google.com [209.85.208.195]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 102A5469719 for ; Mon, 12 Oct 2020 00:34:06 +0300 (MSK) Received: by mail-lj1-f195.google.com with SMTP id f21so14964752ljh.7 for ; Sun, 11 Oct 2020 14:34:06 -0700 (PDT) Date: Mon, 12 Oct 2020 00:34:03 +0300 From: Cyrill Gorcunov Message-ID: <20201011213403.GA14048@grain> References: <20201008213608.1022476-1-gorcunov@gmail.com> <20201008213608.1022476-6-gorcunov@gmail.com> <3cc78ca9-5bf1-0128-1a09-313256be9253@tarantool.org> <20201009065755.GJ2069@grain> <8870495c-3e6b-08d1-b4e3-eb2f7fea6c72@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8870495c-3e6b-08d1-b4e3-eb2f7fea6c72@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v5 5/6] box/cbox: implement cbox Lua module List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tml On Fri, Oct 09, 2020 at 11:31:12PM +0200, Vladislav Shpilevoy wrote: > >>> + > >>> + int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX); > >>> + lua_xmove(L, args_L, lua_gettop(L) - 1); > >> > >> 2. Why didn't you use lua_remove() to get rid of the unnecessary > >> parts of the stack? > > > > Which one you think is redundant? The object itself? If so then > > it sits as a bottom element of the stack and removing it would > > simply make code slower. > > Yes, I see the problem now. First issue is that you can remove > the root argument, because it may triggers its GC. Second issue > is that you still need luaT_newthread(), because port_lua uses > tarantool_L. Both issues can be easily fixed in port_lua. > > https://github.com/tarantool/tarantool/issues/5406 Thanks for making an issue but i've a serious doubt about this "easily fixed" statement. > > Vlad this "prepare environment for calling" part of code is very > > sensitive to the arguments, I tried a various combinations in > > attempts to get rid of lua thread and none of them work. So I > > left it as is and put a FIXME, because it requires more serious > > rework. > > FIXME don't work, they are never fixed after creation unless you > have a ticket. Although these times even a ticket tends to be sent > to the wishlist for years. We've a ticket now. Moreover the use of newthread is definitely not a blocker for the functionality. We've been using it in _func space for years. As to me -- unitifaction of "call" procedure looks like a separate task which is of course related to the cbox but we started implementing cbox code not to eliminate "thread" call in first place or optimize a code flow. But I agree we've to do something. > >>> + > >>> + lua_pushstring(L, "__call"); > >>> + lua_pushcfunction(L, lcbox_func_call); > >>> + lua_settable(L, -3); > >>> + > >>> + lua_setmetatable(L, -2); > >> > >> 3. What happens when the function object is deleted by Lua GC? > >> It seems the user can't get the function again without dropping > >> it. > > > > And that is a good question. I must confess I missed Lua GC aspect > > here. Thanks a huge! I'll rework! > > Another thing to look at - on each call you perform a lookup in the > function hash. But you don't really need it. You can store the > function pointer in a hidden field. For example, in __index metatable > of the function object. Or inside the function object with _ prefix, > so at least it won't be visible for autocompletion. Or turn the > function object into cdata. Anyway you will need it to properly > implement a GC handler. And this cdata would be a struct > lbox_sym/lbox_func/whatever with the function pointer in it. I though about it as well but must confess I don't like this approach: it means the memory allocated by our cbox code then passed to luajit as a variable and then we always "trust" this object. I'll think about more. Still the mark about GC handler is correct, thanks!