From: Igor Munkin <imun@tarantool.org> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v2 3/3] box: replace port_tuple with port_c everywhere Date: Sat, 25 Apr 2020 03:21:09 +0300 [thread overview] Message-ID: <20200425002109.GK11314@tarantool.org> (raw) In-Reply-To: <4177aec25c7ff283575a0ccb3a3a62d2ee51fde8.1587600640.git.v.shpilevoy@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(-) > <snipped> > 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 <size> in src/box/port.h. > }; > > void <snipped> > @@ -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 <size> 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. > }; > <snipped> > 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 <snipped> > -- > 2.21.1 (Apple Git-122.3) > -- Best regards, IM
next prev parent reply other threads:[~2020-04-25 0:28 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-04-23 0:12 [Tarantool-patches] [PATCH v2 0/3] box_return_mp Vladislav Shpilevoy 2020-04-23 0:12 ` [Tarantool-patches] [PATCH v2 1/3] box: introduce port_c Vladislav Shpilevoy 2020-04-24 12:22 ` Igor Munkin 2020-04-24 22:06 ` Vladislav Shpilevoy 2020-04-23 0:12 ` [Tarantool-patches] [PATCH v2 2/3] box: introduce box_return_mp() public C function Vladislav Shpilevoy 2020-04-24 12:22 ` Igor Munkin 2020-04-27 15:14 ` Nikita Pettik 2020-04-27 21:29 ` Vladislav Shpilevoy 2020-04-27 22:55 ` Nikita Pettik 2020-04-23 0:12 ` [Tarantool-patches] [PATCH v2 3/3] box: replace port_tuple with port_c everywhere Vladislav Shpilevoy 2020-04-25 0:21 ` Igor Munkin [this message] 2020-04-26 19:22 ` Vladislav Shpilevoy 2020-04-27 9:12 ` Igor Munkin 2020-04-27 9:18 ` Igor Munkin 2020-04-27 14:10 ` Nikita Pettik 2020-04-28 11:08 ` [Tarantool-patches] [PATCH v2 0/3] box_return_mp Kirill Yukhin
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=20200425002109.GK11314@tarantool.org \ --to=imun@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v2 3/3] box: replace port_tuple with port_c everywhere' \ /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