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

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

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