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

[Nikita Pettik <korablev@tarantool.org>]

On 08 Mar 18:47, Vladislav Shpilevoy wrote:

In general, patch is okay. Below some comments which can be
considered as nits.

I see that your branch doesn't pass CI testing:
https://travis-ci.org/github/tarantool/tarantool/jobs/659861432?utm_medium=notification&utm_source=github_status

I haven't managed to find box/alter_limits among existing flaky
tests. If this test fail is not really related to your patches,
could you please file an issue?

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

I'd do it as a follow-up in case you have time to do it.
Otherwise please just file an issue.

> 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
> @@ -37,7 +37,12 @@
>  #include <fiber.h>
>  #include "errinj.h"
>  
> -static struct mempool port_tuple_entry_pool;

I'd add comment explaining that port_entry_pool holds both
port_c and port_tuple entries now. Moreover, as an optimization
some msgpacks stored in port_c also can be allocated in this
pool (as far as I understand).

> +static struct mempool port_entry_pool;
> +
> +enum {
> +	PORT_ENTRY_SIZE = MAX(sizeof(struct port_c_entry),
> +			      sizeof(struct port_tuple_entry)),
> +};
>  
> @@ -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);
>  
> +static inline void
> +port_c_destroy_entry(struct port_c_entry *pe)
> +{

I'd add a brief reference like:

/* For allocation policy see port_c_add_tuple() and port_c_add_mp(). */

> +	if (pe->mp_size == 0)
> +		tuple_unref(pe->tuple);
> +	else if (pe->mp_size <= PORT_ENTRY_SIZE)
> +		mempool_free(&port_entry_pool, pe->mp);
> +	else
> +		free(pe->mp);
> +}
> +
> +static inline struct port_c_entry *
> +port_c_new_entry(struct port_c *port)
> +{
> +	struct port_c_entry *e;
> +	if (port->size == 0) {
> +		e = &port->first_entry;

I don't really get this 'first_entry' optimization. Why do you
need it at all?

> +		port->first = e;
> +		port->last = e;
> +	} else {
> +		e = mempool_alloc(&port_entry_pool);
> +		if (e == NULL) {
> +			diag_set(OutOfMemory, sizeof(*e), "mempool_alloc", "e");
> +			return NULL;
> +		}
> +		port->last->next = e;
> +		port->last = e;
> +	}
> +	e->next = NULL;
> +	++port->size;
> +	return e;
> +}
> +
> +int
> +port_c_add_tuple(struct port *base, struct tuple *tuple)
> +{
> +	struct port_c *port = (struct port_c *)base;
> +	struct port_c_entry *pe = port_c_new_entry(port);

I guess reverting condition would make code flow more straightforward:
if (pe == NULL)
	return -1;
...

> +	if (pe != NULL) {
> +		/* 0 mp_size means the entry stores a tuple. */
> +		pe->mp_size = 0;
> +		pe->tuple = tuple;
> +		tuple_ref(tuple);
> +		return 0;
> +	}
> +	return -1;
> +}
> +
> +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;
> +	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);

Dubious optimization...I mean is this code complication really
worth it?

> +		if (dst == NULL) {
> +			diag_set(OutOfMemory, size, "mempool_alloc", "dst");
> +			return -1;
> +		}
> +
> +static int
> +port_c_dump_msgpack_16(struct port *base, struct obuf *out)
> +{
> +	struct port_c *port = (struct port_c *)base;
> +	struct port_c_entry *pe;
> +	for (pe = port->first; pe != NULL; pe = pe->next) {
> +		uint32_t size = pe->mp_size;
> +		if (size == 0) {
> +			if (tuple_to_obuf(pe->tuple, out) != 0)
> +				return -1;
> +		} else if (obuf_dup(out, pe->mp, size) != size) {
> +			diag_set(OutOfMemory, size, "obuf_dup", "data");
> +			return -1;
> +		}
> +		ERROR_INJECT(ERRINJ_PORT_DUMP, {
> +			diag_set(OutOfMemory,
> +				 size == 0 ? tuple_size(pe->tuple) : size,
> +				 "obuf_dup", "data");
> +			return -1;
> +		});

You added error injection, but did not test it. Could you provide test
(or point me at it please)?

> +	}
> +	return port->size;
> +}
> +
> diff --git a/src/box/port.h b/src/box/port.h
> index 9d3d02b3c..cd40d8a15 100644
> --- a/src/box/port.h
> +++ b/src/box/port.h
> @@ -130,6 +130,56 @@ void
>  port_vdbemem_create(struct port *base, struct sql_value *mem,
>  		    uint32_t mem_count);
>  
> +/**
> + * The struct is PACKED to avoid unnecessary 4 byte padding

Is this really profitable?

> + * after mp_size. These objects are never stored on stack, neither
> + * allocated as an array, so the padding is not needed.
> + */
> +struct PACKED port_c_entry {
> +	struct port_c_entry *next;
> +	union {
> +		/** Valid if mp_size is 0. */
> +		struct tuple *tuple;
> +		/**
> +		 * Valid if mp_size is > 0. MessagePack is
> +		 * allocated either on heap or on the port entry
> +		 * mempool, if it fits into a pool object.
> +		 */
> +		char *mp;
> +	};
> +	uint32_t mp_size;
> +};
> +
> +/**
> + * C port is used by C functions from the public API. They can
> + * return tuples and arbitrary MessagePack.
> + */
> +struct port_c {
> +	const struct port_vtab *vtab;
> +	struct port_c_entry *first;
> +	struct port_c_entry *last;
> +	struct port_c_entry first_entry;
> +	int size;

Why do you call it size? I'd name it entry_cnt or count
(imho port size = sum of all entries' sizes).

> +};
> +
> +static_assert(sizeof(struct port_c) <= sizeof(struct port),
> +	      "sizeof(struct port_c) must be <= sizeof(struct port)");
> +
>  port_init(void);
>