From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 68B574696C3 for ; Mon, 27 Apr 2020 17:09:57 +0300 (MSK) Date: Mon, 27 Apr 2020 14:09:56 +0000 From: Nikita Pettik Message-ID: <20200427140955.GB30870@tarantool.org> References: <145c14e137171e6ee017da22aa579b369f7ccee8.1583689251.git.v.shpilevoy@tarantool.org> <20200326174911.GA5718@tarantool.org> <7301da02-16f8-2629-b673-8d6125032111@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <7301da02-16f8-2629-b673-8d6125032111@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 2/3] box: introduce port_c List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.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.