From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (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 04D59441841 for ; Thu, 26 Mar 2020 20:49:12 +0300 (MSK) Date: Thu, 26 Mar 2020 17:49:11 +0000 From: Nikita Pettik Message-ID: <20200326174911.GA5718@tarantool.org> References: <145c14e137171e6ee017da22aa579b369f7ccee8.1583689251.git.v.shpilevoy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <145c14e137171e6ee017da22aa579b369f7ccee8.1583689251.git.v.shpilevoy@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 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 > #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); >