Re: [Tarantool-patches] [PATCH v2 3/3] box: replace port_tuple with port_c everywhere

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the review!

On 25/04/2020 02:21, Igor Munkin wrote:
> 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/.

Fixed.

>> 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 <size> in src/box/port.h.

Forgot to revert this place after tried rename to
'count' everywhere.

====================
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index fd1628930..7c80f9c8a 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -93,7 +93,7 @@ ffi.cdef[[
         struct port_c_entry *first;
         struct port_c_entry *last;
         struct port_c_entry first_entry;
-        int count;
+        int size;
     };
 
     void
@@ -1545,7 +1545,7 @@ base_index_mt.select_ffi = function(index, key, opts)
 
     local ret = {}
     local entry = port_c.first
-    for i=1,tonumber(port_c.count),1 do
+    for i=1,tonumber(port_c.size),1 do
         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 @@
>> + * 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.

If I change it, I need to change its usage place too, and increase
the diff. If we want to make the diff even smaller and remove this
constant, it would be better to merge this commit and the first one.
I can do that, if you want.

>>  };
>> 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.

Yes. I accidentally added it to core/port.h instead of box/port.h

====================
diff --git a/src/box/port.h b/src/box/port.h
index 0100bac9a..76bb5ed1b 100644
--- a/src/box/port.h
+++ b/src/box/port.h
@@ -40,6 +40,8 @@ extern "C" {
 
 struct tuple;
 
+extern const struct port_vtab port_c_vtab;
+
 /** Port implementation used for storing raw data. */
 struct port_msgpack {
 	const struct port_vtab *vtab;
diff --git a/src/lib/core/port.h b/src/lib/core/port.h
index ade12eadf..bb7787679 100644
--- a/src/lib/core/port.h
+++ b/src/lib/core/port.h
@@ -41,8 +41,6 @@ struct obuf;
 struct lua_State;
 struct port;
 
-extern const struct port_vtab port_c_vtab;
-
 /**
  * A single port represents a destination of any output. One such
  * destination can be a Lua stack, or the binary protocol. An

====================