From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp61.i.mail.ru (smtp61.i.mail.ru [217.69.128.41]) (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 02974469719 for ; Fri, 2 Oct 2020 19:14:56 +0300 (MSK) From: "Timur Safin" References: <51700ea1fd13c5cbea49e3426551d1c16c1bda02.1600955781.git.tsafin@tarantool.org> <2958f26a-ef0a-4e13-70ec-b99b05cdb2b9@tarantool.org> <20200929055301.udvoq7j4n6fupit6@tkn_work_nb> <20201001030004.755sdofapdsuu77u@tkn_work_nb> In-Reply-To: <20201001030004.755sdofapdsuu77u@tkn_work_nb> Date: Fri, 2 Oct 2020 19:14:57 +0300 Message-ID: <0ade01d698d7$2bf55fa0$83e01ee0$@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 6/7] module api: luaL_checkibuf & luaL_checkconstchar List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: 'Alexander Turenko' , 'Vladislav Shpilevoy' Cc: tarantool-patches@dev.tarantool.org : From: Alexander Turenko : Subject: Re: [Tarantool-patches] [PATCH 2.X 6/7] module api: : luaL_checkibuf & luaL_checkconstchar :=20 : 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. :=20 : I just agreed explicitly. Ok, I see the common consensus about naming here, but teher are bigger = questions about small reuse... :=20 : > : > >> 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 : 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 ), : 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? :=20 : 'unless we'll really care about ABI' was regarding the small ABI. = Sorry, : now I see that it is not obvious from my wording. :=20 : I'll repeat options as a list just in case: :=20 : 1. Expose and () 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. :=20 : Both ways are okay for me. The second one is maybe better, but it is : more dangerous considering our current deadlines. :=20 : 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. This is big question - how to deal with 3rd party libraries here in = modules and across all repositories. Especially if we already have our library = extracted to the separate repository, like in case of small. I've put my thoughts = about this topic elsewhere (see my reply to the cover email), and this worth = to=20 discuss with Alexander Lyapunov also... :=20 : > : > 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 with if we'll add = static : > > asserts around the structure size and offset of fields? And expose : > > box_ibuf_*(), of course. :=20 : I meant non-opaque structure and it seems I was not right. We cannot : alias to now and, when we'll change : , provide another , which will have the same : layout as the old . The reason is that 'real' : is already available via Lua ('buffer' module and via net.box). So if : we'll going this way (expose non-opaque ), we should = typedef : it to another structure right now and return : cdata from Lua. It seems, it would be good = to : avoid this extra complexity of code and APIs. :=20 : So let's consider opaque 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. :=20 : So lot calls to do memcpy() and advance a pointer... :=20 : > : > 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. :=20 : Timur does not need this method, he can implement it on the module = side. :=20 : But since our built-in module 'buffer' provides cdata and : cdata, 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. :=20 : I would say the same for cdata, 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. :=20 : 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 = objects : in Lua API, it would be good to have luaT_isfoo() and luaT_pushfoo() : functions in the module API. Exactly so! Timur