From: Alexander Turenko <alexander.turenko@tarantool.org> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH 2.X 6/7] module api: luaL_checkibuf & luaL_checkconstchar Date: Thu, 1 Oct 2020 06:00:04 +0300 [thread overview] Message-ID: <20201001030004.755sdofapdsuu77u@tkn_work_nb> (raw) In-Reply-To: <daccf0e8-84d3-5ed5-1974-77480900f684@tarantool.org> On Wed, Sep 30, 2020 at 01:25:15AM +0200, Vladislav Shpilevoy wrote: > On 29.09.2020 07:53, Alexander Turenko wrote: > >>> +/** > >>> + * Check if a value on @a L stack by index @a idx is an ibuf > >>> + * object. Both 'struct ibuf' and 'struct ibuf *' are accepted. > >>> + * Returns NULL, if can't convert - not an ibuf object. > >>> + */ > >>> +struct ibuf * > >>> +luaL_checkibuf(struct lua_State *L, int idx); > >> > >> 1. IMO 'check' is almost always worse than 'is'. Because you leave > >> a user no choice but to use lua_cpcall if he does not want an > >> exception. With 'is' he would be able to decide whether he wants to > >> throw. The same for the method below. > > > > I personally prefer *is* Lua functions. It is good to have *is* variants > > in the public API aside of (or even instead of) *check* ones. I don't > > insist, however. > > This is what I said. *is* is better than *check*. More freedom to decide > what to do, when it returns false. I just agreed explicitly. > > >> Also what exactly is the reason you need the ibuf checking for? > >> Ibuf is not exposed as a part of tarantool binary. It is a part of > >> small library. When we want to export parts of small from inside > >> of the executable, we need to obfuscate the types and wrap the > >> functions, because user's small library may be different from the > >> executable's small. > > > > It seems, we really should expose non-opacue <box_ibuf_t>: rpos and wpos > > are used in merger (it would be too expensive to wrap them into calls). > > The module accepts ibuf created from Lua (tarantool's <struct ibuf>), so > > linking with external small is not an option (unless we'll really care > > about ABI). > > It seems you care about ABI a lot. So why do you stop now? If we don't care > about ABI that much, then why do we worry about box_key_part_def_t? 'unless we'll really care about ABI' was regarding the small ABI. Sorry, now I see that it is not obvious from my wording. I'll repeat options as a list just in case: 1. Expose <box_ibuf_t> and <box_ibuf_*>() from the module API. 2. Ensure that we can hold small's ibuf API / ABI (from several sides: ability to extend it in future, static asserts, testing, no implicit dependency on a particular toolchain behaviour) and use it directly. Both ways are okay for me. The second one is maybe better, but it is more dangerous considering our current deadlines. I would highlight that just 'let's use small directly, unlikely it will be changed' does not work in the long terms. So the second way is not cheap, IMHO. Many things should be considered before we'll able to state that, say, ibuf API / ABI is perfectly stable. > > I am ok with box_ibuf_t, of any kind. Then we could eventually legally expose > tarantool_lua_ibuf, as at least for single alloc+free pairs it is better than > region (a tiny tiny better). > > > Maybe we can alias <box_ibuf_t> with <struct ibuf> if we'll add static > > asserts around the structure size and offset of fields? And expose > > box_ibuf_*(), of course. I meant non-opaque structure and it seems I was not right. We cannot alias <box_ibuf_t> to <struct ibuf> now and, when we'll change <struct ibuf>, provide another <box_ibuf_t>, which will have the same layout as the old <struct ibuf>. The reason is that 'real' <struct ibuf> is already available via Lua ('buffer' module and via net.box). So if we'll going this way (expose non-opaque <box_ibuf_t>), we should typedef it to another structure right now and return cdata<struct this_new_structure> from Lua. It seems, it would be good to avoid this extra complexity of code and APIs. So let's consider opaque <box_ibuf_t> approach. It'll add indirect calls on hot paths (for each tuple reading from a buffer or writing to a buffer), but I don't see any good way to overcome this. Ever increasing of tuple's reference counter will be indirect call for an external module. So lot calls to do memcpy() and advance a pointer... > > Also looks fine. However returning to the topic, it seems not related to the > Lua type ids. You don't need ibuf definition to get its type ID in C. All > is done in luaL_ctypeid function taking a string anyway. > > luaL_ctypeid(L, "struct ibuf *"); > luaL_ctypeid(L, "struct ibuf"); > > So why does Timur need that method exported? luaL_checkibuf is a trival > function, which can be implemented by luaL_iscdata() + getting ibuf type > id using luaL_ctypeid() and comparing result with the cdata type id. Timur does not need this method, he can implement it on the module side. But since our built-in module 'buffer' provides cdata<struct ibuf> and cdata<struct ibuf *>, it would be good to have Lua/C accessors in the module API. It is simple, so there are no much questions for design, and it looks worthful to expose it while we're here. I would say the same for cdata<struct key_def &>, but since there is another similar cdata type offered by the external key_def module, it has no practical reason: we should handle both anyway. Maybe uuid and decimal too, but I'm not sure. It is hard to estimate how many times those functions will be used in modules (that are written by us or by users). I would think about this like so: since we provide ability to write modules using Lua/C and have cdata<struct foo> objects in Lua API, it would be good to have luaT_isfoo() and luaT_pushfoo() functions in the module API.
next prev parent reply other threads:[~2020-10-01 2:59 UTC|newest] Thread overview: 64+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-09-24 17:00 [Tarantool-patches] [PATCH 2.X 0/7] RFC: module api: extend for external merger Lua module Timur Safin 2020-09-24 17:00 ` [Tarantool-patches] [PATCH 2.X 1/7] module api: export box_tuple_validate Timur Safin 2020-09-28 22:20 ` Vladislav Shpilevoy 2020-10-02 12:24 ` Timur Safin 2020-10-09 1:11 ` Alexander Turenko 2020-10-09 20:11 ` Vladislav Shpilevoy 2020-10-10 1:19 ` Alexander Turenko 2020-09-29 5:25 ` Alexander Turenko 2020-10-05 7:35 ` Alexander Turenko 2020-09-24 17:00 ` [Tarantool-patches] [PATCH 2.X 2/7] module api: export box_key_def_dup Timur Safin 2020-09-28 22:21 ` Vladislav Shpilevoy 2020-09-29 5:03 ` Alexander Turenko 2020-09-29 23:19 ` Vladislav Shpilevoy 2020-10-01 3:05 ` Alexander Turenko 2020-10-02 12:25 ` Timur Safin 2020-10-02 12:26 ` Timur Safin 2020-09-24 17:00 ` [Tarantool-patches] [PATCH 2.X 3/7] module api: luaT_newthread Timur Safin 2020-09-28 22:21 ` Vladislav Shpilevoy 2020-10-02 12:27 ` Timur Safin 2020-10-02 21:48 ` Vladislav Shpilevoy 2020-09-29 6:25 ` Alexander Turenko 2020-10-02 12:26 ` Timur Safin 2020-10-02 12:53 ` Alexander Turenko 2020-09-29 15:19 ` Igor Munkin 2020-10-02 16:12 ` Timur Safin 2020-10-03 16:57 ` Igor Munkin 2020-09-24 17:00 ` [Tarantool-patches] [PATCH 2.X 4/7] module api: luaL_register_module & luaL_register_type Timur Safin 2020-09-28 22:21 ` Vladislav Shpilevoy 2020-09-29 5:09 ` Alexander Turenko 2020-09-29 23:20 ` Vladislav Shpilevoy 2020-09-30 6:31 ` Alexander Turenko 2020-09-30 6:33 ` Alexander Turenko 2020-10-02 16:14 ` Timur Safin 2020-09-29 8:03 ` Igor Munkin 2020-09-29 23:21 ` Vladislav Shpilevoy 2020-10-02 16:14 ` Timur Safin 2020-10-03 3:24 ` Alexander Turenko 2020-09-24 17:00 ` [Tarantool-patches] [PATCH 2.X 5/7] module api: luaT_temp_luastate & luaT_release_temp_luastate Timur Safin 2020-09-28 22:21 ` Vladislav Shpilevoy 2020-09-29 5:17 ` Alexander Turenko 2020-09-29 23:21 ` Vladislav Shpilevoy 2020-10-01 3:35 ` Alexander Turenko 2020-09-29 15:10 ` Igor Munkin 2020-09-29 21:03 ` Alexander Turenko 2020-09-29 23:23 ` Vladislav Shpilevoy 2020-09-30 10:09 ` Alexander Turenko 2020-10-01 15:06 ` Igor Munkin 2020-10-03 2:16 ` Alexander Turenko 2020-10-02 12:24 ` Timur Safin 2020-09-24 17:00 ` [Tarantool-patches] [PATCH 2.X 6/7] module api: luaL_checkibuf & luaL_checkconstchar Timur Safin 2020-09-28 22:21 ` Vladislav Shpilevoy 2020-09-29 5:53 ` Alexander Turenko 2020-09-29 23:25 ` Vladislav Shpilevoy 2020-10-01 3:00 ` Alexander Turenko [this message] 2020-10-02 16:14 ` Timur Safin 2020-10-02 21:48 ` Vladislav Shpilevoy 2020-10-08 13:50 ` Timur Safin 2020-09-24 17:00 ` [Tarantool-patches] [PATCH 2.X 7/7] module api: luaL_cdata_iscallable Timur Safin 2020-09-28 22:21 ` Vladislav Shpilevoy 2020-09-29 5:19 ` Alexander Turenko 2020-10-02 16:14 ` Timur Safin 2020-10-02 12:23 ` [Tarantool-patches] [PATCH 2.X 0/7] RFC: module api: extend for external merger Lua module Timur Safin 2020-10-02 21:49 ` Vladislav Shpilevoy 2020-10-03 2:54 ` Alexander Turenko
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=20201001030004.755sdofapdsuu77u@tkn_work_nb \ --to=alexander.turenko@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH 2.X 6/7] module api: luaL_checkibuf & luaL_checkconstchar' \ /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