From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 0CD074696C3 for ; Sat, 25 Apr 2020 03:28:17 +0300 (MSK) Date: Sat, 25 Apr 2020 03:21:09 +0300 From: Igor Munkin Message-ID: <20200425002109.GK11314@tarantool.org> References: <4177aec25c7ff283575a0ccb3a3a62d2ee51fde8.1587600640.git.v.shpilevoy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <4177aec25c7ff283575a0ccb3a3a62d2ee51fde8.1587600640.git.v.shpilevoy@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 3/3] box: replace port_tuple with port_c everywhere List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Vlad, Thanks for the patch! Please consider the comments I left below. On 23.04.20, Vladislav Shpilevoy wrote: > Port_tuple is exactly the same as port_c, but is not able to store > raw MessagePack. I theory it sounds like port_tuple should be a Typo: s/I theory/In theory/. > bit simpler and therefore faster, but in fact it is not. > Microbenchmarks didn't reveal any difference. So port_tuple is no > longer needed, all its functionality is covered by port_c. > > Follow up #4641 > --- > src/box/box.cc | 4 +- > src/box/execute.c | 10 ++-- > src/box/execute.h | 13 +++-- > src/box/key_list.c | 4 +- > src/box/lua/execute.c | 6 +-- > src/box/lua/misc.cc | 18 ------- > src/box/lua/schema.lua | 29 ++++++----- > src/box/port.c | 109 ++--------------------------------------- > src/box/port.h | 41 ---------------- > src/lib/core/port.h | 4 +- > 10 files changed, 41 insertions(+), 197 deletions(-) > > diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua > index 10584494b..fd1628930 100644 > --- a/src/box/lua/schema.lua > +++ b/src/box/lua/schema.lua > @@ -79,17 +79,21 @@ ffi.cdef[[ > box_txn_savepoint_t * > box_txn_savepoint(); > > - struct port_tuple_entry { > - struct port_tuple_entry *next; > - struct tuple *tuple; > + struct __attribute__((packed)) port_c_entry { > + struct port_c_entry *next; > + union { > + struct tuple *tuple; > + char *mp; > + }; > + uint32_t mp_size; > }; > > - struct port_tuple { > + struct port_c { > const struct port_vtab *vtab; > - int size; > - struct port_tuple_entry *first; > - struct port_tuple_entry *last; > - struct port_tuple_entry first_entry; > + struct port_c_entry *first; > + struct port_c_entry *last; > + struct port_c_entry first_entry; > + int count; It's still in src/box/port.h. > }; > > void > @@ -1541,8 +1544,8 @@ base_index_mt.select_ffi = function(index, key, opts) > end > > local ret = {} > - local entry = port_tuple.first > - for i=1,tonumber(port_tuple.size),1 do > + local entry = port_c.first > + for i=1,tonumber(port_c.count),1 do It's still in src/box/port.h. > ret[i] = tuple_bless(entry.tuple) > entry = entry.next > end > diff --git a/src/box/port.c b/src/box/port.c > index 2c1fadb5c..9d9fc1dbc 100644 > --- a/src/box/port.c > +++ b/src/box/port.c > @@ -38,106 +38,15 @@ > #include "errinj.h" > > /** > - * The pools is used both by port_c and port_tuple, since their > - * entires are almost of the same size. Also port_c can use > - * objects from the pool to store result data in their memory, > - * when it fits. > + * The pools is used by port_c to allocate entries and to store Typo: Just a reminder. I see you've already fixed it in the previous patch. > + * result data when it fits into an object from the pool. > */ > static struct mempool port_entry_pool; > > enum { > - PORT_ENTRY_SIZE = MAX(sizeof(struct port_c_entry), > - sizeof(struct port_tuple_entry)), > + PORT_ENTRY_SIZE = sizeof(struct port_c_entry), Minor: PORT_ENTRY_SIZE is introduced in the first patch of the series. After applying these changes it looks excess. Feel free to ignore if you see any rationale to leave this constant. > }; > > diff --git a/src/lib/core/port.h b/src/lib/core/port.h > index bfdfa4656..ade12eadf 100644 > --- a/src/lib/core/port.h > +++ b/src/lib/core/port.h > @@ -41,6 +41,8 @@ struct obuf; > struct lua_State; > struct port; > > +extern const struct port_vtab port_c_vtab; As we discussed this line should be moved to src/box/port.h. > + > /** > * A single port represents a destination of any output. One such > * destination can be a Lua stack, or the binary protocol. An > -- > 2.21.1 (Apple Git-122.3) > -- Best regards, IM