From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [94.100.177.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 47F794765E0 for ; Thu, 24 Dec 2020 17:08:28 +0300 (MSK) References: From: Leonid Vasiliev Message-ID: <810f25cf-1c5e-3358-2050-4b0ce2f6ece4@tarantool.org> Date: Thu, 24 Dec 2020 17:08:21 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Nikiforov , tarantool-patches@dev.tarantool.org Cc: Vladislav Shpilevoy 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; > } >