* [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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance
@ 2020-12-24 14:14 Leonid Vasiliev
0 siblings, 0 replies; 11+ messages in thread
From: Leonid Vasiliev @ 2020-12-24 14:14 UTC (permalink / raw)
To: Sergey Nikiforov, tarantool-patches; +Cc: Vladislav Shpilevoy
[-- Attachment #1: Type: text/plain, Size: 9588 bytes --]
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;
> }
>
[-- Attachment #2: Type: text/html, Size: 12444 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-12-25 13:10 UTC | newest]
Thread overview: 11+ 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
2020-12-24 14:14 Leonid Vasiliev
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox