From: Cyrill Gorcunov <gorcunov@gmail.com> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Cc: tml <tarantool-patches@dev.tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v5 5/6] box/cbox: implement cbox Lua module Date: Mon, 12 Oct 2020 00:34:03 +0300 [thread overview] Message-ID: <20201011213403.GA14048@grain> (raw) In-Reply-To: <8870495c-3e6b-08d1-b4e3-eb2f7fea6c72@tarantool.org> 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!
next prev parent reply other threads:[~2020-10-11 21:34 UTC|newest] Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-08 21:36 [Tarantool-patches] [PATCH v5 0/6] box/cbox: implement cfunc " Cyrill Gorcunov 2020-10-08 21:36 ` [Tarantool-patches] [PATCH v5 1/6] box/func: factor out c function entry structure Cyrill Gorcunov 2020-10-08 21:36 ` [Tarantool-patches] [PATCH v5 2/6] box/func: provide module_sym_call Cyrill Gorcunov 2020-10-08 21:36 ` [Tarantool-patches] [PATCH v5 3/6] box/func: more detailed error in module reloading Cyrill Gorcunov 2020-10-08 21:36 ` [Tarantool-patches] [PATCH v5 4/6] box/func: export func_split_name helper Cyrill Gorcunov 2020-10-08 21:36 ` [Tarantool-patches] [PATCH v5 5/6] box/cbox: implement cbox Lua module Cyrill Gorcunov 2020-10-08 22:35 ` Vladislav Shpilevoy 2020-10-09 6:57 ` Cyrill Gorcunov 2020-10-09 21:31 ` Vladislav Shpilevoy 2020-10-11 21:34 ` Cyrill Gorcunov [this message] 2020-10-09 21:46 ` Vladislav Shpilevoy 2020-10-11 21:58 ` Cyrill Gorcunov 2020-10-08 21:36 ` [Tarantool-patches] [PATCH v5 6/6] test: box/cfunc -- add simple module test Cyrill Gorcunov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20201011213403.GA14048@grain \ --to=gorcunov@gmail.com \ --cc=tarantool-patches@dev.tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v5 5/6] box/cbox: implement cbox Lua module' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox