[Tarantool-patches] [PATCH 2/3] box: introduce port_c
Nikita Pettik
korablev at tarantool.org
Mon Apr 27 17:09:56 MSK 2020
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.
More information about the Tarantool-patches
mailing list