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

[Nikita Pettik <korablev@tarantool.org>]

On 23 Apr 02:14, Vladislav Shpilevoy wrote:
> Hi! Thanks for the review!
> 
> On 26/03/2020 18:49, Nikita Pettik wrote:
> > 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?
> 
> It is not related (at least I think so), I saw it failing on other
> branches too.
> 
> Created an issue:
> https://github.com/tarantool/tarantool/issues/4926

Thanks. (No doubts it is not related to your patch, just want to
make sure that all flaky tests are registrated.)
 
> >> +}
> >> +
> >> +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?
> 
> Because most functions return only 1 result. And most of these
> return it as a tuple. So having this value inside the port
> preallocated allows to avoid any allocations/frees for these
> cases. Zero allocation. Although I didn't really bench how
> much it helps. I just stole it from port_tuple as a low hanging
> fruit.

On the other hand you get memory overhead, which you fight for
so much even using packed attribute...
Why not then keeping 2, 3 or whatever number benchmarks say 
entries inlined?
  
> >> +	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?
> 
> mempool_alloc() of this size is x4-5 times faster than malloc()
> according to my microbenchmarks on release build. I decided not
> to write concrete numbers x4-5, since they probably depend on
> machine.

Is there at least one real scenario where entry allocation
policy can turn out to be bottleneck in overall performance?
 
> >> 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?
> 
> -4 bytes for every object of this type by just adding one word to
> the struct? Sounds good to me. Very easy to do, to prove it works
> correct, and profit is obvious - the structure becomes 16% smaller.
> This means 16% more objects in every slab used for port_c_entry
> objects. Although I didn't measure how it affects things under heavy
> load on C functions.

Well, okay. At least its profit is clear (in contrast to others).

> >> +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).
> 
> For that we have term 'bsize'. But I don't have anything against
> count, personally. The problem here is that it will be inconsistent
> with other ports (port_lua, and port_tuple which still exists here),
> and with local variables named 'size', which are assigned to
> port->size member - that is the problem in src/box/lua/call.c.
> 
> When I renamed size to count, I got notable amount of unnecessary
> diff. So I would like to keep size unless you insist, and in that
> case I will do it in a separate patch.

Personally I would still rename it, perhaps for all port classes.
Up to you as non-functional change.