[tarantool-patches] Re: [PATCH v2 1/2] box: export mpstream methods to core

[Vladislav Shpilevoy <v.shpilevoy@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.