[Tarantool-patches] [PATCH 2/3] box: introduce port_c

[Igor Munkin]

Vlad,

Thanks for the patch! I see no flaws, but I left several nits below in
addition to Nikita's comments.

On 08.03.20, Vladislav Shpilevoy wrote:
> Port_c is a new descendant of struct port. It is used now for
> public C functions to store their result. Currently they can
> return only a tuple, but it will change soon, they will be able to
> return arbitrary MessagePack.
> 
> Port_tuple is not removed, because still is used for box_select(),
> for functional indexes, and in SQL as a base for port_sql.
> Although that may be changed later. Functional indexes really need
> only a single MessagePack object from their function. While
> box_select() working via port_tuple or port_c didn't show any
> significant difference during micro benchmarks.
> 
> Part of #4641
> ---
> 
> As you can see, port_tuple is used in a very few places now. And
> by performance it is not really different from the new port_c,
> according to micro benchmarks I did. Moreover, it may be actually
> faster for functional indexes - users won't need to create a tuple
> just to return a functional index key, which is usually small, I
> believe.
> 
> If you (reviewers) want me to drop port_tuple in scope of this
> patchset, I can do that. At the moment of sending the patchset I
> decided to postpone it.
> 
>  src/box/box.cc      |   2 +-
>  src/box/func.c      |   2 +-
>  src/box/lua/misc.cc |  20 +++++
>  src/box/port.c      | 191 +++++++++++++++++++++++++++++++++++++++++---
>  src/box/port.h      |  50 ++++++++++++
>  src/box/sql/func.c  |  22 ++---
>  6 files changed, 266 insertions(+), 21 deletions(-)
> 

<snipped>

> diff --git a/src/box/port.c b/src/box/port.c
> index 6e2fe3a6e..925a4b2d5 100644
> --- a/src/box/port.c
> +++ b/src/box/port.c

<snipped>

> @@ -127,28 +132,194 @@ port_tuple_dump_msgpack(struct port *base, struct obuf *out)
>  extern void
>  port_tuple_dump_lua(struct port *base, struct lua_State *L, bool is_flat);
>  

<snipped>

> +
> +static void
> +port_c_destroy(struct port *base)
> +{
> +	struct port_c *port = (struct port_c *)base;
> +	struct port_c_entry *pe = port->first;
> +	if (pe == NULL)
> +		return;
> +	port_c_destroy_entry(pe);

Minor: It's worth to drop a few words here that the port->first points
to port->first_entry and it is released with the port_c structure.

> +	pe = pe->next;
> +	while (pe != NULL) {
> +		struct port_c_entry *cur = pe;
> +		pe = pe->next;
> +		port_c_destroy_entry(cur);
> +		mempool_free(&port_entry_pool, cur);
> +	}
> +}
> +

<snipped>

> +
> +int
> +port_c_add_mp(struct port *base, const char *mp, const char *mp_end)
> +{
> +	struct port_c *port = (struct port_c *)base;
> +	struct port_c_entry *pe;
> +	uint32_t size = mp_end - mp;

Minor: since there are no guarantees except the existing contract and
common sense making this value be non-negative, I propose to add a
corresponding assert. Feel free to ignore.

> +	char *dst;
> +	if (size <= PORT_ENTRY_SIZE) {
> +		/*
> +		 * Alloc on a mempool is several times faster than
> +		 * on the heap. And it perfectly fits any
> +		 * MessagePack number, a short string, a boolean.
> +		 */
> +		dst = mempool_alloc(&port_entry_pool);
> +		if (dst == NULL) {
> +			diag_set(OutOfMemory, size, "mempool_alloc", "dst");
> +			return -1;
> +		}
> +	} else {
> +		dst = malloc(size);
> +		if (dst == NULL) {
> +			diag_set(OutOfMemory, size, "malloc", "dst");
> +			return -1;
> +		}
> +	}
> +	pe = port_c_new_entry(port);
> +	if (pe != NULL) {
> +		memcpy(dst, mp, size);
> +		pe->mp = dst;
> +		pe->mp_size = size;
> +		return 0;
> +	}
> +	if (size <= PORT_ENTRY_SIZE)
> +		mempool_free(&port_entry_pool, dst);
> +	else
> +		free(dst);
> +	return -1;
> +}
> +

<snipped>

> -- 
> 2.21.1 (Apple Git-122.3)
> 

-- 
Best regards,
IM