Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: tarantool-patches@freelists.org,
	Kirill Shcherbatov <kshcherbatov@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH v2 1/2] box: export mpstream methods to core
Date: Mon, 27 Aug 2018 22:43:04 -0300	[thread overview]
Message-ID: <53a9fff6-61ff-cffc-6f9d-073cd38d6928@tarantool.org> (raw)
In-Reply-To: <426c34291ce7834cf224eaed82506564c066ff8e.1535367103.git.kshcherbatov@tarantool.org>

Thanks for the patch! See 2 comments below.

On 27/08/2018 08:11, Kirill Shcherbatov wrote:
> As we going to use mpstream not only in LUA, let's move this
> API in tarantool core.
> 
> Part of #3545.
> ---
>   src/CMakeLists.txt    |   1 +
>   src/box/lua/call.c    |  11 +--
>   src/box/lua/misc.cc   |   1 +
>   src/box/lua/net_box.c | 127 +++++++++++++++----------------
>   src/box/lua/tuple.c   |  23 +++---
>   src/lua/msgpack.c     | 166 ++++------------------------------------
>   src/lua/msgpack.h     | 102 +------------------------
>   src/mpstream.c        | 205 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   src/mpstream.h        | 124 ++++++++++++++++++++++++++++++
>   9 files changed, 429 insertions(+), 331 deletions(-)
>   create mode 100644 src/mpstream.c
>   create mode 100644 src/mpstream.h
> > diff --git a/src/mpstream.c b/src/mpstream.c
> new file mode 100644
> index 0000000..23d27f9
> --- /dev/null
> +++ b/src/mpstream.c
> @@ -0,0 +1,205 @@
> +/*
> + * Copyright 2010-2018, Tarantool AUTHORS, please see AUTHORS file.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + *    copyright notice, this list of conditions and the
> + *    following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + *    copyright notice, this list of conditions and the following
> + *    disclaimer in the documentation and/or other materials
> + *    provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> +#include "mpstream.h"
> +#include <assert.h>
> +#include <stdint.h>
> +#include "msgpuck.h"
> +
> +void
> +mpstream_reserve_slow(struct mpstream *stream, size_t size)
> +{
> +    stream->alloc(stream->ctx, stream->pos - stream->buf);
> +    stream->buf = (char *) stream->reserve(stream->ctx, &size);
> +    if (stream->buf == NULL) {
> +        diag_set(OutOfMemory, size, "mpstream", "reserve");
> +        stream->error(stream->error_ctx);
> +    }
> +    stream->pos = stream->buf;
> +    stream->end = stream->pos + size;
> +}
> +
> +void
> +mpstream_reset(struct mpstream *stream)
> +{
> +    size_t size = 0;
> +    stream->buf = (char *) stream->reserve(stream->ctx, &size);
> +    if (stream->buf == NULL) {
> +        diag_set(OutOfMemory, size, "mpstream", "reset");
> +        stream->error(stream->error_ctx);
> +    }
> +    stream->pos = stream->buf;
> +    stream->end = stream->pos + size;
> +}
> +
> +/**
> + * A streaming API so that it's possible to encode to any output
> + * stream.
> + */
> +void
> +mpstream_init(struct mpstream *stream, void *ctx,
> +              mpstream_reserve_f reserve, mpstream_alloc_f alloc,
> +              mpstream_error_f error, void *error_ctx)
> +{
> +    stream->ctx = ctx;
> +    stream->reserve = reserve;
> +    stream->alloc = alloc;
> +    stream->error = error;
> +    stream->error_ctx = error_ctx;
> +    mpstream_reset(stream);
> +}
> +
> +void
> +mpstream_flush(struct mpstream *stream)
> +{
> +    stream->alloc(stream->ctx, stream->pos - stream->buf);
> +    stream->buf = stream->pos;
> +}
> +
> +char *
> +mpstream_reserve(struct mpstream *stream, size_t size)
> +{
> +    if (stream->pos + size > stream->end)
> +        mpstream_reserve_slow(stream, size);
> +    return stream->pos;
> +}
> +
> +void
> +mpstream_advance(struct mpstream *stream, size_t size)
> +{
> +    assert(stream->pos + size <= stream->end);
> +    stream->pos += size;
> +}

1. As I said in the previous review, reserve and
reserve_slow are not split in two functions just
for case. There is a reason. And your patch
destroys the benefit of reserve/reserve_slow
splitting. I paste it here:

"There was a reason, why reserve_slow is an independent function
and reserve is static inline. This is why in most cases reserve
just returns a current position, and rarely it falls down to
alloc(). Please, return as it was. Same for advance."

Keywords:
*"return as it was",
* "reserve_slow is independent, reserve is static inline",
* "same for advance".

Static inline reserve in a header provides a benefit of
inlining in most cases and rarely falls down to reserve_slow,
that is not static inline.

> diff --git a/src/mpstream.h b/src/mpstream.h
> new file mode 100644
> index 0000000..789bd74
> --- /dev/null
> +++ b/src/mpstream.h
> @@ -0,0 +1,124 @@
> +#ifndef TARANTOOL_LUA_MPSTREAM_H_INCLUDED
> +#define TARANTOOL_LUA_MPSTREAM_H_INCLUDED

2. It is not 'LUA' anymore.

  reply	other threads:[~2018-08-28  1:43 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-27 11:11 [tarantool-patches] [PATCH v2 0/2] sql: remove struct Enc Kirill Shcherbatov
2018-08-27 11:11 ` [tarantool-patches] [PATCH v2 1/2] box: export mpstream methods to core Kirill Shcherbatov
2018-08-28  1:43   ` Vladislav Shpilevoy [this message]
2018-08-28  6:46     ` [tarantool-patches] " Kirill Shcherbatov
2018-08-27 11:11 ` [tarantool-patches] [PATCH v2 2/2] sql: remove struct Enc Kirill Shcherbatov
2018-08-28  1:43   ` [tarantool-patches] " Vladislav Shpilevoy
2018-08-28  6:46     ` Kirill Shcherbatov
2018-08-28 23:21       ` Vladislav Shpilevoy
2018-08-28  1:43 ` [tarantool-patches] Re: [PATCH v2 0/2] " Vladislav Shpilevoy
2018-08-29 14:12 ` Kirill Yukhin

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=53a9fff6-61ff-cffc-6f9d-073cd38d6928@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=kshcherbatov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH v2 1/2] box: export mpstream methods to core' \
    /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