[Tarantool-patches] [PATCH 1/1] json: use cord_ibuf for encoding and decoding
Serge Petrenko
sergepetrenko at tarantool.org
Mon May 24 16:01:25 MSK 2021
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
More information about the Tarantool-patches
mailing list