From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id CE1B32A612 for ; Mon, 27 Aug 2018 21:43:13 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id o1fa_tXgK0Pg for ; Mon, 27 Aug 2018 21:43:13 -0400 (EDT) Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 89C6429D14 for ; Mon, 27 Aug 2018 21:43:13 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 1/2] box: export mpstream methods to core References: <426c34291ce7834cf224eaed82506564c066ff8e.1535367103.git.kshcherbatov@tarantool.org> From: Vladislav Shpilevoy Message-ID: <53a9fff6-61ff-cffc-6f9d-073cd38d6928@tarantool.org> Date: Mon, 27 Aug 2018 22:43:04 -0300 MIME-Version: 1.0 In-Reply-To: <426c34291ce7834cf224eaed82506564c066ff8e.1535367103.git.kshcherbatov@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, Kirill Shcherbatov 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 ``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 > + * 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 > +#include > +#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.