Re: [Tarantool-patches] [PATCH 1/1] json: use cord_ibuf for encoding and decoding

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

23.05.2021 17:06, Vladislav Shpilevoy пишет:
> Lua json module used to have a global buffer for all encodings. It
> was reused by each next encode().
>
> This was not correct, because during encode() might happen a GC
> step, which might call encode() again and spoil the global buffer.
>
> The same problem was already fixed for the global static buffer in
> scope of #5632. Similarly to that time, the patch makes Lua json
> module use cord_ibuf to prevent "concurrent" usage of the buffer
> data. The global buffer is deleted.
>
> According to a few microbenchmarks it didn't affect the perf
> anyhow.
>
> Core part of the patch is strbuf changes. Firstly, its destruction
> is now optional, cord_ibuf can free itself on a next yield.
> Secondly, its reallocation algorithm is kept intact - ibuf is used
> as an allocator, not as the buffer itself. This is done so as not
> to be too intrusive in the third party module which might need an
> upgrade to the upstream in the future.
>
> Closes #6050
> ---
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-6050-lua-json-static-buf-gc
> Issue: https://github.com/tarantool/tarantool/issues/6050

Hi! Thanks for the patch!
LGTM with one nit below.


...

> diff --git a/third_party/lua-cjson/strbuf.c b/third_party/lua-cjson/strbuf.c
> index f0f7f4b9a..22c1f2093 100644
> --- a/third_party/lua-cjson/strbuf.c
> +++ b/third_party/lua-cjson/strbuf.c
> @@ -28,6 +28,7 @@
>   #include <string.h>
>   
>   #include "strbuf.h"
> +#include "small/ibuf.h"
>   
>   static void die(const char *fmt, ...)
>   {
> @@ -41,7 +42,7 @@ static void die(const char *fmt, ...)
>       exit(-1);
>   }
>   
> -void strbuf_init(strbuf_t *s, int len)
> +void strbuf_create(strbuf_t *s, int len, struct ibuf *ibuf)
>   {
>       int size;
>   
> @@ -50,37 +51,19 @@ void strbuf_init(strbuf_t *s, int len)
>       else
>           size = len + 1;         /* \0 terminator */
>   
> -    s->buf = NULL;
> +    s->buf = ibuf_reserve(ibuf, size);
>       s->size = size;
>       s->length = 0;
>       s->increment = STRBUF_DEFAULT_INCREMENT;
> -    s->dynamic = 0;
>       s->reallocs = 0;
>       s->debug = 0;
> -
> -    s->buf = malloc(size);
> +    s->ibuf = ibuf;
>       if (!s->buf)
>           die("Out of memory");
>   
>       strbuf_ensure_null(s);
>   }
>   
> -strbuf_t *strbuf_new(int len)
> -{
> -    strbuf_t *s;
> -
> -    s = malloc(sizeof(strbuf_t));
> -    if (!s)
> -        die("Out of memory");
> -
> -    strbuf_init(s, len);
> -
> -    /* Dynamic strbuf allocation / deallocation */
> -    s->dynamic = 1;
> -
> -    return s;
> -}
> -
>   void strbuf_set_increment(strbuf_t *s, int increment)
>   {
>       /* Increment > 0:  Linear buffer growth rate
> @@ -99,36 +82,9 @@ static inline void debug_stats(strbuf_t *s)
>       }
>   }
>   
> -/* If strbuf_t has not been dynamically allocated, strbuf_free() can
> - * be called any number of times strbuf_init() */
> -void strbuf_free(strbuf_t *s)
> -{
> -    debug_stats(s);
> -
> -    if (s->buf) {
> -        free(s->buf);
> -        s->buf = NULL;
> -    }
> -    if (s->dynamic)
> -        free(s);
> -}
> -
> -char *strbuf_free_to_string(strbuf_t *s, int *len)
> +void strbuf_destroy(strbuf_t *s)
>   {
> -    char *buf;
> -
>       debug_stats(s);
> -
> -    strbuf_ensure_null(s);
> -
> -    buf = s->buf;
> -    if (len)
> -        *len = s->length;
> -
> -    if (s->dynamic)
> -        free(s);
> -
> -    return buf;
>   }
>   
>   static int calculate_new_size(strbuf_t *s, int len)
> @@ -171,9 +127,21 @@ void strbuf_resize(strbuf_t *s, int len)
>           fprintf(stderr, "strbuf(%lx) resize: %d => %d\n",
>                   (long)s, s->size, newsize);
>       }
> -
>       s->size = newsize;

  ^ Extraneous change

> -    s->buf = realloc(s->buf, s->size);
> +
> +    struct ibuf *ibuf = s->ibuf;
> +    /*
> +     * Propagate the write position to enable memcpy() when realloc happens
> +     * inside of the ibuf.
> +     */
> +    ibuf->wpos += s->length;
> +    s->buf = ibuf_reserve(ibuf, newsize - s->length) - s->length;
> +    /*
> +     * But then it is reverted because memcpy() of the old data is done, and the
> +     * write position + capacity are managed by strbuf itself.
> +     */
> +    ibuf->wpos = s->buf;
> +
>       if (!s->buf)
>           die("Out of memory");
>       s->reallocs++;

...

-- 
Serge Petrenko