From: Vladimir Davydov <vdavydov.dev@gmail.com>
To: imeevma@tarantool.org
Cc: v.shpilevoy@tarantool.org, tarantool-patches@freelists.org,
kostja@tarantool.org
Subject: Re: [PATCH v4 2/5] iproto: replace obuf by mpstream in execute.c
Date: Mon, 3 Dec 2018 18:21:38 +0300 [thread overview]
Message-ID: <20181203152138.p2uclz5p2pfkigup@esperanza> (raw)
In-Reply-To: <a857e1129f9e38bde8c5c6037149887f695ec6dc.1543747066.git.imeevma@gmail.com>
On Sun, Dec 02, 2018 at 02:03:21PM +0300, imeevma@tarantool.org wrote:
> This patch is the most dubious patch due to the implicit use of
> mpstream as a stream for obuf. Discussion and patch below.
>
> It is worth noting that in this version of the patch nothing
> changes. At this point there is no approved solution for this
> patch.
>
>
> On 11/30/18 1:55 PM, Vladimir Davydov wrote:
> > On Fri, Nov 30, 2018 at 01:45:48PM +0300, Vladislav Shpilevoy wrote:
> >>
> >>
> >> On 30/11/2018 13:19, Vladimir Davydov wrote:
> >>> On Thu, Nov 29, 2018 at 05:04:06PM +0300, Vladislav Shpilevoy wrote:
> >>>> On 29/11/2018 13:53, Vladimir Davydov wrote:
> >>>>> On Tue, Nov 27, 2018 at 10:25:43PM +0300, imeevma@tarantool.org wrote:
> >>>>>> @@ -625,81 +608,53 @@ sql_prepare_and_execute(const struct sql_request *request,
> >>>>>> }
> >>>>>>
> >>>>>> int
> >>>>>> -sql_response_dump(struct sql_response *response, int *keys, struct obuf *out)
> >>>>>> +sql_response_dump(struct sql_response *response, int *keys,
> >>>>>> + struct mpstream *stream)
> >>>>>> {
> >>>>>> sqlite3 *db = sql_get();
> >>>>>> struct sqlite3_stmt *stmt = (struct sqlite3_stmt *) response->prep_stmt;
> >>>>>> - struct port_tuple *port_tuple = (struct port_tuple *) &response->port;
> >>>>>> int rc = 0, column_count = sqlite3_column_count(stmt);
> >>>>>> if (column_count > 0) {
> >>>>>> - if (sql_get_description(stmt, out, column_count) != 0) {
> >>>>>> + if (sql_get_description(stmt, stream, column_count) != 0) {
> >>>>>> err:
> >>>>>> rc = -1;
> >>>>>> goto finish;
> >>>>>> }
> >>>>>> *keys = 2;
> >>>>>> - int size = mp_sizeof_uint(IPROTO_DATA) +
> >>>>>> - mp_sizeof_array(port_tuple->size);
> >>>>>> - char *pos = (char *) obuf_alloc(out, size);
> >>>>>> - if (pos == NULL) {
> >>>>>> - diag_set(OutOfMemory, size, "obuf_alloc", "pos");
> >>>>>> - goto err;
> >>>>>> - }
> >>>>>> - pos = mp_encode_uint(pos, IPROTO_DATA);
> >>>>>> - pos = mp_encode_array(pos, port_tuple->size);
> >>>>>> - /*
> >>>>>> - * Just like SELECT, SQL uses output format compatible
> >>>>>> - * with Tarantool 1.6
> >>>>>> - */
> >>>>>> - if (port_dump_msgpack_16(&response->port, out) < 0) {
> >>>>>> + mpstream_encode_uint(stream, IPROTO_DATA);
> >>>>>> + mpstream_flush(stream);
> >>>>>> + if (port_dump_msgpack(&response->port, stream->ctx) < 0) {
> >>>>>
> >>>>> stream->ctx isn't guaranteed to be an obuf
> >>>>>
> >>>>> And when you introduce vstream later, you simply move this code to
> >>>>> another file. This is confusing. May be we should pass alloc/reserve
> >>>>> used in mpstream to port_dump instead of obuf?
> >>>>
> >>>> Good idea, though not sure, if it is worth slowing down port_dump_msgpack
> >>>> adding a new level of indirection. Since port_dump_msgpack is a hot path
> >>>> and is used for box.select.
> >>>>
> >>>> Maybe it is better to just rename port_dump_msgpack to port_dump_obuf
> >>>> and rename vstream_port_dump to vstream_port_dump_obuf? If we ever will
> >>>> dump port to not obuf, then we will just add a new method to port_vtab.
> >>>>
> >>>> Also, it would make port_dump_obuf name consistent with port_dump_lua -
> >>>> in both cases we not just dump in a specific format, but to a concrete
> >>>> destination: obuf and lua stack. Now port_dump_msgpack anyway is restricted
> >>>> by obuf destination.
> >>>
> >>> There's port_dump_plain, which dumps port contents in a specific format.
> >>> So port_dump_obuf would look ambiguous.
> >>>
> >>>>
> >>>> If you worry about how to call sql_response_dump() to not obuf, then there
> >>>> is another option. Anyway rename port_dump_msgpack to port_dump_obuf and
> >>>> introduce a new method: port_dump_mpstream. It will take mpstream and use
> >>>> its reserve/alloc/error functions. It allows us to do not slow down box.select,
> >>>> but use the full power of virtual functions in execute.c, which definitely is
> >>>> not hot.
> >>>
> >>> That would interconnect port and mpstream, make them dependent on each
> >>> other. I don't think that would be good.
> >>>
> >>>>
> >>>> mpstream implementation of vstream will call port_dump_mpstream, and
> >>>> luastream implementation of vstream will call port_dump_lua as it does now.
> >>>> box.select and iproto_call will use port_dump_obuf.
> >>>>
> >>>> I prefer the second option: introduce port_dump_mpstream. It is ok for you?
> >>>
> >>> I may be wrong, but IMO there isn't much point in optimizing box.select,
> >>> because it's very limited in its applicability. People already prefer to
> >>> use box.call over box.insert/select/etc over iproto, and with the
> >>> appearance of box.execute they are likely to stop using plain box.select
> >>> at all.
> >>>
> >>> That said, personally I would try to pass reserve/alloc methods to port,
> >>> see how it goes.
> >>>
> >>
> >> I do not see a reason to slow down box.select if we can don't do it.
> >> Yeas, people use IPROTO_CALL, but in stored functions they use box
> >> functions including select.
> >
> > box.select called from Lua code doesn't use port_dump_msgpack.
> >
> >>
> >> Ok, instead of port_dump_mpstream we can rename port_dump_msgpack to
> >> port_dump_obuf and add port_dump_msgpack which does not depend on
> >> mpstream and takes alloc/reserve/ctx directly.
> >
> > Better call the optimized version (the one without callbacks)
> > port_dump_msgpack_obuf to avoid confusion IMO.
> >
> > Anyway, I'd try to run cbench to see if it really perfomrs better
> > than the one using callbacks.
>
> @@ -625,81 +608,53 @@ sql_prepare_and_execute(const struct sql_request *request,
> }
>
> int
> -sql_response_dump(struct sql_response *response, int *keys, struct obuf *out)
> +sql_response_dump(struct sql_response *response, int *keys,
> + struct mpstream *stream)
> {
> sqlite3 *db = sql_get();
> struct sqlite3_stmt *stmt = (struct sqlite3_stmt *) response->prep_stmt;
> - struct port_tuple *port_tuple = (struct port_tuple *) &response->port;
> int rc = 0, column_count = sqlite3_column_count(stmt);
> if (column_count > 0) {
> - if (sql_get_description(stmt, out, column_count) != 0) {
> + if (sql_get_description(stmt, stream, column_count) != 0) {
> err:
> rc = -1;
> goto finish;
> }
> *keys = 2;
> - int size = mp_sizeof_uint(IPROTO_DATA) +
> - mp_sizeof_array(port_tuple->size);
> - char *pos = (char *) obuf_alloc(out, size);
> - if (pos == NULL) {
> - diag_set(OutOfMemory, size, "obuf_alloc", "pos");
> - goto err;
> - }
> - pos = mp_encode_uint(pos, IPROTO_DATA);
> - pos = mp_encode_array(pos, port_tuple->size);
> - /*
> - * Just like SELECT, SQL uses output format compatible
> - * with Tarantool 1.6
> - */
> - if (port_dump_msgpack_16(&response->port, out) < 0) {
> + mpstream_encode_uint(stream, IPROTO_DATA);
> + mpstream_flush(stream);
> + if (port_dump_msgpack(&response->port, stream->ctx) < 0) {
Still, I'm quite convinced that we need to pass alloc/reserve methods
along with ctx to port_dump_msgpack(), because implicitly assumping that
mpstream->ctx is, in fact, an obuf looks very fragile. However, Vlad is
right that it may indeed affect performance in a negative way. So let's
perhaps do the following:
1. Run cbench to see how badly indirect obuf_alloc/reserve slows
things down.
2. Consider the possibility of using templates or macro definitions
instead of function pointers.
What do you think?
next prev parent reply other threads:[~2018-12-03 15:21 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-30 19:00 [PATCH v4 0/5] Remove box.sql.execute() imeevma
2018-11-30 19:01 ` [PATCH v4 1/5] box: move port to src/ imeevma
2018-12-03 9:22 ` Vladimir Davydov
2018-11-30 19:01 ` [tarantool-patches] [PATCH v4 2/5] iproto: replace obuf by mpstream in execute.c imeevma
2018-11-30 19:01 ` [tarantool-patches] [PATCH v4 3/5] sql: create interface vstream imeevma
2018-11-30 19:01 ` [tarantool-patches] [PATCH v4 4/5] lua: create vstream implementation for Lua imeevma
2018-11-30 19:01 ` [tarantool-patches] [PATCH v4 5/5] sql: check new box.sql.execute() imeevma
2018-12-02 11:03 ` [PATCH v4 2/5] iproto: replace obuf by mpstream in execute.c imeevma
2018-12-03 15:21 ` Vladimir Davydov [this message]
2018-12-03 20:48 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-04 8:26 ` Vladimir Davydov
2018-12-04 11:28 ` Vladislav Shpilevoy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181203152138.p2uclz5p2pfkigup@esperanza \
--to=vdavydov.dev@gmail.com \
--cc=imeevma@tarantool.org \
--cc=kostja@tarantool.org \
--cc=tarantool-patches@freelists.org \
--cc=v.shpilevoy@tarantool.org \
--subject='Re: [PATCH v4 2/5] iproto: replace obuf by mpstream in execute.c' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox