* [Tarantool-patches] [PATCH v3 0/2] base64: fix decoder, improve its performance @ 2020-12-22 10:41 Sergey Nikiforov 2020-12-22 10:41 ` [Tarantool-patches] [PATCH v3 1/2] base64: fix decoder output buffer overrun (reads) Sergey Nikiforov 2020-12-22 10:41 ` [Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance Sergey Nikiforov 0 siblings, 2 replies; 10+ messages in thread From: Sergey Nikiforov @ 2020-12-22 10:41 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladislav Shpilevoy First patch fixes #3069, second one improves base64 decoder performance Branch: https://github.com/tarantool/tarantool/tree/void234/gh-3069-fix-base64-memory-overrun-v3 Issue: https://github.com/tarantool/tarantool/issues/3069 Sergey Nikiforov (2): base64: fix decoder output buffer overrun (reads) base64: improve decoder performance test/unit/base64.c | 7 +++- test/unit/base64.result | 84 +++++++++++++++++++++++++++-------------- third_party/base64.c | 58 +++++++++++++++++++--------- 3 files changed, 103 insertions(+), 46 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Tarantool-patches] [PATCH v3 1/2] base64: fix decoder output buffer overrun (reads) 2020-12-22 10:41 [Tarantool-patches] [PATCH v3 0/2] base64: fix decoder, improve its performance Sergey Nikiforov @ 2020-12-22 10:41 ` Sergey Nikiforov 2020-12-24 12:28 ` Leonid Vasiliev 2020-12-22 10:41 ` [Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance Sergey Nikiforov 1 sibling, 1 reply; 10+ messages in thread From: Sergey Nikiforov @ 2020-12-22 10:41 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladislav Shpilevoy Was caught by base64 test with enabled ASAN. It also caused data corruption - garbage instead of "extra bits" was saved into state->result if there was no space in output buffer. Fixes: #3069 --- Branch: https://github.com/tarantool/tarantool/tree/void234/gh-3069-fix-base64-memory-overrun-v3 Issue: https://github.com/tarantool/tarantool/issues/3069 third_party/base64.c | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/third_party/base64.c b/third_party/base64.c index 8ecab23eb..3350a98ff 100644 --- a/third_party/base64.c +++ b/third_party/base64.c @@ -247,8 +247,9 @@ base64_decode_block(const char *in_base64, int in_len, char *out_pos = out_bin; char *out_end = out_bin + out_len; int fragment; + char curr_byte; - *out_pos = state->result; + curr_byte = state->result; switch (state->step) { @@ -259,49 +260,54 @@ base64_decode_block(const char *in_base64, int in_len, if (in_pos == in_end || out_pos >= out_end) { state->step = step_a; - state->result = *out_pos; + state->result = curr_byte; return out_pos - out_bin; } fragment = base64_decode_value(*in_pos++); } while (fragment < 0); - *out_pos = (fragment & 0x03f) << 2; + curr_byte = (fragment & 0x03f) << 2; case step_b: do { if (in_pos == in_end || out_pos >= out_end) { state->step = step_b; - state->result = *out_pos; + state->result = curr_byte; return out_pos - out_bin; } fragment = base64_decode_value(*in_pos++); } while (fragment < 0); - *out_pos++ |= (fragment & 0x030) >> 4; + curr_byte |= (fragment & 0x030) >> 4; + *out_pos++ = curr_byte; + curr_byte = (fragment & 0x00f) << 4; if (out_pos < out_end) - *out_pos = (fragment & 0x00f) << 4; + *out_pos = curr_byte; case step_c: do { if (in_pos == in_end || out_pos >= out_end) { state->step = step_c; - state->result = *out_pos; + state->result = curr_byte; return out_pos - out_bin; } fragment = base64_decode_value(*in_pos++); } while (fragment < 0); - *out_pos++ |= (fragment & 0x03c) >> 2; + curr_byte |= (fragment & 0x03c) >> 2; + *out_pos++ = curr_byte; + curr_byte = (fragment & 0x003) << 6; if (out_pos < out_end) - *out_pos = (fragment & 0x003) << 6; + *out_pos = curr_byte; case step_d: do { if (in_pos == in_end || out_pos >= out_end) { state->step = step_d; - state->result = *out_pos; + state->result = curr_byte; return out_pos - out_bin; } fragment = base64_decode_value(*in_pos++); } while (fragment < 0); - *out_pos++ |= (fragment & 0x03f); + curr_byte |= (fragment & 0x03f); + *out_pos++ = curr_byte; } } /* control should not reach here */ -- 2.25.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 1/2] base64: fix decoder output buffer overrun (reads) 2020-12-22 10:41 ` [Tarantool-patches] [PATCH v3 1/2] base64: fix decoder output buffer overrun (reads) Sergey Nikiforov @ 2020-12-24 12:28 ` Leonid Vasiliev 0 siblings, 0 replies; 10+ messages in thread From: Leonid Vasiliev @ 2020-12-24 12:28 UTC (permalink / raw) To: Sergey Nikiforov, tarantool-patches; +Cc: Vladislav Shpilevoy Hi! Thank you for the patch. I have some comments: - Why are you adding a test in the next commit? Seems like this test is validating this fix. - I agree with Vlad that the test should be moved to a separate test function. The changes in third_party/base64.c - LGTM. On 22.12.2020 13:41, Sergey Nikiforov wrote: > Was caught by base64 test with enabled ASAN. > > It also caused data corruption - garbage instead of "extra bits" was > saved into state->result if there was no space in output buffer. > > Fixes: #3069 > --- > > Branch: https://github.com/tarantool/tarantool/tree/void234/gh-3069-fix-base64-memory-overrun-v3 > Issue: https://github.com/tarantool/tarantool/issues/3069 > > third_party/base64.c | 28 +++++++++++++++++----------- > 1 file changed, 17 insertions(+), 11 deletions(-) > > diff --git a/third_party/base64.c b/third_party/base64.c > index 8ecab23eb..3350a98ff 100644 > --- a/third_party/base64.c > +++ b/third_party/base64.c > @@ -247,8 +247,9 @@ base64_decode_block(const char *in_base64, int in_len, > char *out_pos = out_bin; > char *out_end = out_bin + out_len; > int fragment; > + char curr_byte; > > - *out_pos = state->result; > + curr_byte = state->result; > > switch (state->step) > { > @@ -259,49 +260,54 @@ base64_decode_block(const char *in_base64, int in_len, > if (in_pos == in_end || out_pos >= out_end) > { > state->step = step_a; > - state->result = *out_pos; > + state->result = curr_byte; > return out_pos - out_bin; > } > fragment = base64_decode_value(*in_pos++); > } while (fragment < 0); > - *out_pos = (fragment & 0x03f) << 2; > + curr_byte = (fragment & 0x03f) << 2; > case step_b: > do { > if (in_pos == in_end || out_pos >= out_end) > { > state->step = step_b; > - state->result = *out_pos; > + state->result = curr_byte; > return out_pos - out_bin; > } > fragment = base64_decode_value(*in_pos++); > } while (fragment < 0); > - *out_pos++ |= (fragment & 0x030) >> 4; > + curr_byte |= (fragment & 0x030) >> 4; > + *out_pos++ = curr_byte; > + curr_byte = (fragment & 0x00f) << 4; > if (out_pos < out_end) > - *out_pos = (fragment & 0x00f) << 4; > + *out_pos = curr_byte; > case step_c: > do { > if (in_pos == in_end || out_pos >= out_end) > { > state->step = step_c; > - state->result = *out_pos; > + state->result = curr_byte; > return out_pos - out_bin; > } > fragment = base64_decode_value(*in_pos++); > } while (fragment < 0); > - *out_pos++ |= (fragment & 0x03c) >> 2; > + curr_byte |= (fragment & 0x03c) >> 2; > + *out_pos++ = curr_byte; > + curr_byte = (fragment & 0x003) << 6; > if (out_pos < out_end) > - *out_pos = (fragment & 0x003) << 6; > + *out_pos = curr_byte; > case step_d: > do { > if (in_pos == in_end || out_pos >= out_end) > { > state->step = step_d; > - state->result = *out_pos; > + state->result = curr_byte; > return out_pos - out_bin; > } > fragment = base64_decode_value(*in_pos++); > } while (fragment < 0); > - *out_pos++ |= (fragment & 0x03f); > + curr_byte |= (fragment & 0x03f); > + *out_pos++ = curr_byte; > } > } > /* control should not reach here */ > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance 2020-12-22 10:41 [Tarantool-patches] [PATCH v3 0/2] base64: fix decoder, improve its performance Sergey Nikiforov 2020-12-22 10:41 ` [Tarantool-patches] [PATCH v3 1/2] base64: fix decoder output buffer overrun (reads) Sergey Nikiforov @ 2020-12-22 10:41 ` Sergey Nikiforov 2020-12-22 15:05 ` Vladislav Shpilevoy 2020-12-24 14:08 ` Leonid Vasiliev 1 sibling, 2 replies; 10+ messages in thread From: Sergey Nikiforov @ 2020-12-22 10:41 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladislav Shpilevoy 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(-) 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; */ 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 */ + 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; } -- 2.25.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance 2020-12-22 10:41 ` [Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance Sergey Nikiforov @ 2020-12-22 15:05 ` Vladislav Shpilevoy 2020-12-22 16:16 ` Sergey Nikiforov 2020-12-24 14:08 ` Leonid Vasiliev 1 sibling, 1 reply; 10+ messages in thread From: Vladislav Shpilevoy @ 2020-12-22 15:05 UTC (permalink / raw) To: Sergey Nikiforov, tarantool-patches Thanks for the 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 > @@ -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"); From the .result file it is clear this test is done multiple times. And it does not depend on base64_test() arguments. So why do you run exactly the same test again and again? Maybe move it to a separate test function which is run only once? > + > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance 2020-12-22 15:05 ` Vladislav Shpilevoy @ 2020-12-22 16:16 ` Sergey Nikiforov 2020-12-22 16:40 ` Vladislav Shpilevoy 0 siblings, 1 reply; 10+ messages in thread From: Sergey Nikiforov @ 2020-12-22 16:16 UTC (permalink / raw) To: Vladislav Shpilevoy, tarantool-patches Hi! On 22.12.2020 18:05, Vladislav Shpilevoy wrote: > Thanks for the 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 >> @@ -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"); > > From the .result file it is clear this test is done multiple > times. And it does not depend on base64_test() arguments. So why > do you run exactly the same test again and again? Maybe move it > to a separate test function which is run only once? Ok, I will do this if there will be another patch revision. You are, however, reviewing your own code - I have just used your patch from earlier review iteration (17 Dec 2020 00:22:08 +0100) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance 2020-12-22 16:16 ` Sergey Nikiforov @ 2020-12-22 16:40 ` Vladislav Shpilevoy 0 siblings, 0 replies; 10+ messages in thread From: Vladislav Shpilevoy @ 2020-12-22 16:40 UTC (permalink / raw) To: Sergey Nikiforov, tarantool-patches >>> 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 >>> @@ -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"); >> >> From the .result file it is clear this test is done multiple >> times. And it does not depend on base64_test() arguments. So why >> do you run exactly the same test again and again? Maybe move it >> to a separate test function which is run only once? > > Ok, I will do this if there will be another patch revision. > You are, however, reviewing your own code - I have just used your patch from earlier review iteration (17 Dec 2020 00:22:08 +0100) I used it as an example. You don't need to blindly merge everything I post here. Because the patch is yours, not mine. And I may be wrong quite often. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance 2020-12-22 10:41 ` [Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance Sergey Nikiforov 2020-12-22 15:05 ` Vladislav Shpilevoy @ 2020-12-24 14:08 ` Leonid Vasiliev 2020-12-25 10:39 ` Sergey Nikiforov 1 sibling, 1 reply; 10+ messages in thread From: Leonid Vasiliev @ 2020-12-24 14:08 UTC (permalink / raw) To: Sergey Nikiforov, tarantool-patches; +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; > } > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance 2020-12-24 14:08 ` Leonid Vasiliev @ 2020-12-25 10:39 ` Sergey Nikiforov 2020-12-25 13:10 ` Leonid Vasiliev 0 siblings, 1 reply; 10+ messages in thread From: Sergey Nikiforov @ 2020-12-25 10:39 UTC (permalink / raw) To: Leonid Vasiliev, tarantool-patches; +Cc: Vladislav Shpilevoy Hi! On 24.12.2020 17:08, Leonid Vasiliev wrote: > 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? Yes, I had. "perf" in tarantool. Benchmark framework is not yet merged (https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5385-tiny-tuples-with-perf-test/perf) so I plan to commit my microbenchmark a little later to avoid duplication. > 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. Fixed. >> 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. Fixed. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance 2020-12-25 10:39 ` Sergey Nikiforov @ 2020-12-25 13:10 ` Leonid Vasiliev 0 siblings, 0 replies; 10+ messages in thread From: Leonid Vasiliev @ 2020-12-25 13:10 UTC (permalink / raw) To: Sergey Nikiforov, tarantool-patches; +Cc: Vladislav Shpilevoy Hi! On 25.12.2020 13:39, Sergey Nikiforov wrote: > Hi! > > On 24.12.2020 17:08, Leonid Vasiliev wrote: >> 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? > > Yes, I had. "perf" in tarantool. Benchmark framework is not yet merged > (https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5385-tiny-tuples-with-perf-test/perf) > so I plan to commit my microbenchmark a little later to avoid duplication. > OK. >> 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. > > Fixed. > >>> 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. > > Fixed. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-12-25 13:10 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-22 10:41 [Tarantool-patches] [PATCH v3 0/2] base64: fix decoder, improve its performance Sergey Nikiforov 2020-12-22 10:41 ` [Tarantool-patches] [PATCH v3 1/2] base64: fix decoder output buffer overrun (reads) Sergey Nikiforov 2020-12-24 12:28 ` Leonid Vasiliev 2020-12-22 10:41 ` [Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance Sergey Nikiforov 2020-12-22 15:05 ` Vladislav Shpilevoy 2020-12-22 16:16 ` Sergey Nikiforov 2020-12-22 16:40 ` Vladislav Shpilevoy 2020-12-24 14:08 ` Leonid Vasiliev 2020-12-25 10:39 ` Sergey Nikiforov 2020-12-25 13:10 ` Leonid Vasiliev
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox