[Tarantool-patches] [PATCH 1/1] json: use cord_ibuf for encoding and decoding
Serge Petrenko
sergepetrenko at tarantool.org
Mon May 24 16:05:40 MSK 2021
24.05.2021 16:01, Serge Petrenko via Tarantool-patches пишет:
>
>
> 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.
Sorry, I noticed this too late: luacheck has one warning on your branch:
Checking test/app-tap/json.test.lua 1 warning
422 <https://github.com/tarantool/tarantool/runs/2650197964#step:4:422>
423
<https://github.com/tarantool/tarantool/runs/2650197964#step:4:423>test/app-tap/json.test.lua:210:9:
(W213) unused loop variable i
424 <https://github.com/tarantool/tarantool/runs/2650197964#step:4:424>
>
>
> ...
>
>> 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