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

[Igor Munkin]

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