[Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance

[Leonid Vasiliev]

Hi! Thank you for the patch.
AFAIU the status of the patch is follows:

 > But I see we are not going anywhere here. You don't really need LGTM
 > from me on this patch, if you don't want to finish it. I am not
 > strictly against these changes, because *probably* they don't add new
 > bugs, and seem to be a tiny bit better for perf. I only don't like it
 > being not finished.

I think the changes are ok, because they are good for perf (and we have
confirmation) and don't add degradation (our tests should guarantee
this).

See some comments below:

What about a benchmark. AFAIK A. Lyapunov propose saving all benchmarks
that we used. Did you have a conversation with him?

On 22.12.2020 13:41, Sergey Nikiforov wrote:
> Unnecessary checks were removed from internal loops.
> Benchmark shows that performance is now ~1.19 times higher
> (release build, Intel Core I7-9700K, only one thread).
> ---
>
> Branch:  https://github.com/tarantool/tarantool/tree/void234/gh-3069-fix-base64-memory-overrun-v3
>
> test/unit/base64.c | 7 +++-
> test/unit/base64.result | 84 +++++++++++++++++++++++++++--------------
> third_party/base64.c | 36 +++++++++++++-----
> 3 files changed, 89 insertions(+), 38 deletions(-)
>

I left my questions about the test in the review of the previous patch.

> diff --git a/test/unit/base64.c b/test/unit/base64.c
> index ada497adf..76db7d782 100644
> --- a/test/unit/base64.c
> +++ b/test/unit/base64.c
> @@ -7,7 +7,7 @@ static void
> base64_test(const char *str, int options, const char *no_symbols,
> int no_symbols_len)
> {
> - plan(3 + no_symbols_len);
> + plan(4 + no_symbols_len);
>
> int len = strlen(str);
> int base64_buflen = base64_bufsize(len + 1, options);
> @@ -34,6 +34,11 @@ base64_test(const char *str, int options, const char *no_symbols,
> free(base64_buf);
> free(strbuf);
>
> + const char *in = "sIIpHw==";
> + int in_len = strlen(in);
> + rc = base64_decode(in, in_len, NULL, 0);
> + is(rc, 0, "no space in out buffer");
> +
> check_plan();
> }
>
> diff --git a/test/unit/base64.result b/test/unit/base64.result
> index cd1f2b3f6..d606772ea 100644
> --- a/test/unit/base64.result
> +++ b/test/unit/base64.result
> @@ -1,178 +1,206 @@
> 1..28
> *** main ***
> - 1..3
> + 1..4
> ok 1 - length
> ok 2 - decode length ok
> ok 3 - encode/decode
> + ok 4 - no space in out buffer
> ok 1 - subtests
> - 1..6
> + 1..7
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - no + symbols
> ok 4 - no = symbols
> ok 5 - decode length ok
> ok 6 - encode/decode
> + ok 7 - no space in out buffer
> ok 2 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no = symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 3 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 4 - subtests
> - 1..3
> + 1..4
> ok 1 - length
> ok 2 - decode length ok
> ok 3 - encode/decode
> + ok 4 - no space in out buffer
> ok 5 - subtests
> - 1..6
> + 1..7
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - no + symbols
> ok 4 - no = symbols
> ok 5 - decode length ok
> ok 6 - encode/decode
> + ok 7 - no space in out buffer
> ok 6 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no = symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 7 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 8 - subtests
> - 1..3
> + 1..4
> ok 1 - length
> ok 2 - decode length ok
> ok 3 - encode/decode
> + ok 4 - no space in out buffer
> ok 9 - subtests
> - 1..6
> + 1..7
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - no + symbols
> ok 4 - no = symbols
> ok 5 - decode length ok
> ok 6 - encode/decode
> + ok 7 - no space in out buffer
> ok 10 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no = symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 11 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 12 - subtests
> - 1..3
> + 1..4
> ok 1 - length
> ok 2 - decode length ok
> ok 3 - encode/decode
> + ok 4 - no space in out buffer
> ok 13 - subtests
> - 1..6
> + 1..7
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - no + symbols
> ok 4 - no = symbols
> ok 5 - decode length ok
> ok 6 - encode/decode
> + ok 7 - no space in out buffer
> ok 14 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no = symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 15 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 16 - subtests
> - 1..3
> + 1..4
> ok 1 - length
> ok 2 - decode length ok
> ok 3 - encode/decode
> + ok 4 - no space in out buffer
> ok 17 - subtests
> - 1..6
> + 1..7
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - no + symbols
> ok 4 - no = symbols
> ok 5 - decode length ok
> ok 6 - encode/decode
> + ok 7 - no space in out buffer
> ok 18 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no = symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 19 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 20 - subtests
> - 1..3
> + 1..4
> ok 1 - length
> ok 2 - decode length ok
> ok 3 - encode/decode
> + ok 4 - no space in out buffer
> ok 21 - subtests
> - 1..6
> + 1..7
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - no + symbols
> ok 4 - no = symbols
> ok 5 - decode length ok
> ok 6 - encode/decode
> + ok 7 - no space in out buffer
> ok 22 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no = symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 23 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 24 - subtests
> - 1..3
> + 1..4
> ok 1 - length
> ok 2 - decode length ok
> ok 3 - encode/decode
> + ok 4 - no space in out buffer
> ok 25 - subtests
> - 1..6
> + 1..7
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - no + symbols
> ok 4 - no = symbols
> ok 5 - decode length ok
> ok 6 - encode/decode
> + ok 7 - no space in out buffer
> ok 26 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no = symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 27 - subtests
> - 1..4
> + 1..5
> ok 1 - length
> ok 2 - no \n symbols
> ok 3 - decode length ok
> ok 4 - encode/decode
> + ok 5 - no space in out buffer
> ok 28 - subtests
> *** main: done ***
> diff --git a/third_party/base64.c b/third_party/base64.c
> index 3350a98ff..93442c04b 100644
> --- a/third_party/base64.c
> +++ b/third_party/base64.c
> @@ -257,10 +257,11 @@ base64_decode_block(const char *in_base64, int in_len,
> {
> case step_a:
> do {
> - if (in_pos == in_end || out_pos >= out_end)
> + if (in_pos >= in_end)
> {
> state->step = step_a;
> - state->result = curr_byte;
> + /* curr_byte is useless now */
> + /* state->result = curr_byte; */

For multi-line comment, we use the following format:
/*
  * First line
  * Second line
  */

And leaving a commented code is not best practice.

> return out_pos - out_bin;
> }
> fragment = base64_decode_value(*in_pos++);
> @@ -268,7 +269,7 @@ base64_decode_block(const char *in_base64, int in_len,
> curr_byte = (fragment & 0x03f) << 2;
> case step_b:
> do {
> - if (in_pos == in_end || out_pos >= out_end)
> + if (in_pos >= in_end)
> {
> state->step = step_b;
> state->result = curr_byte;
> @@ -276,14 +277,19 @@ base64_decode_block(const char *in_base64, int in_len,
> }
> fragment = base64_decode_value(*in_pos++);
> } while (fragment < 0);
> + if (out_pos >= out_end)
> + {
> + /* We are losing some data */

According to
https://github.com/tarantool/tarantool/wiki/Code-review-procedure :
"Start sentences from a capital letter, end with a dot."
The same for the comments bellow.

> + state->step = step_b;
> + state->result = curr_byte;
> + return out_pos - out_bin;
> + }
> curr_byte |= (fragment & 0x030) >> 4;
> *out_pos++ = curr_byte;
> curr_byte = (fragment & 0x00f) << 4;
> - if (out_pos < out_end)
> - *out_pos = curr_byte;
> case step_c:
> do {
> - if (in_pos == in_end || out_pos >= out_end)
> + if (in_pos >= in_end)
> {
> state->step = step_c;
> state->result = curr_byte;
> @@ -291,14 +297,19 @@ base64_decode_block(const char *in_base64, int in_len,
> }
> fragment = base64_decode_value(*in_pos++);
> } while (fragment < 0);
> + if (out_pos >= out_end)
> + {
> + /* We are losing some data */
> + state->step = step_c;
> + state->result = curr_byte;
> + return out_pos - out_bin;
> + }
> curr_byte |= (fragment & 0x03c) >> 2;
> *out_pos++ = curr_byte;
> curr_byte = (fragment & 0x003) << 6;
> - if (out_pos < out_end)
> - *out_pos = curr_byte;
> case step_d:
> do {
> - if (in_pos == in_end || out_pos >= out_end)
> + if (in_pos >= in_end)
> {
> state->step = step_d;
> state->result = curr_byte;
> @@ -306,6 +317,13 @@ base64_decode_block(const char *in_base64, int in_len,
> }
> fragment = base64_decode_value(*in_pos++);
> } while (fragment < 0);
> + if (out_pos >= out_end)
> + {
> + /* We are losing some data */
> + state->step = step_d;
> + state->result = curr_byte;
> + return out_pos - out_bin;
> + }
> curr_byte |= (fragment & 0x03f);
> *out_pos++ = curr_byte;
> }
>
 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20201224/d4d3e282/attachment.html>