From: Leonid Vasiliev <lvasiliev@tarantool.org> To: Sergey Nikiforov <void@tarantool.org>, tarantool-patches@dev.tarantool.org Cc: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance Date: Thu, 24 Dec 2020 17:08:21 +0300 [thread overview] Message-ID: <810f25cf-1c5e-3358-2050-4b0ce2f6ece4@tarantool.org> (raw) In-Reply-To: <f4e630f7f591fba4001a9edd35ecf6c0b490811a.1608633086.git.void@tarantool.org> Hi! Thank you for the patch. AFAIU the status of the patch is follows: > But I see we are not going anywhere here. You don't really need LGTM > from me on this patch, if you don't want to finish it. I am not > strictly against these changes, because *probably* they don't add new > bugs, and seem to be a tiny bit better for perf. I only don't like it > being not finished. I think the changes are ok, because they are good for perf (and we have confirmation) and don't add degradation (our tests should guarantee this). See some comments below: What about a benchmark. AFAIK A. Lyapunov propose saving all benchmarks that we used. Did you have a conversation with him? On 22.12.2020 13:41, Sergey Nikiforov wrote: > Unnecessary checks were removed from internal loops. > Benchmark shows that performance is now ~1.19 times higher > (release build, Intel Core I7-9700K, only one thread). > --- > > Branch: https://github.com/tarantool/tarantool/tree/void234/gh-3069-fix-base64-memory-overrun-v3 > > test/unit/base64.c | 7 +++- > test/unit/base64.result | 84 +++++++++++++++++++++++++++-------------- > third_party/base64.c | 36 +++++++++++++----- > 3 files changed, 89 insertions(+), 38 deletions(-) > I left my questions about the test in the review of the previous patch. > diff --git a/test/unit/base64.c b/test/unit/base64.c > index ada497adf..76db7d782 100644 > --- a/test/unit/base64.c > +++ b/test/unit/base64.c > @@ -7,7 +7,7 @@ static void > base64_test(const char *str, int options, const char *no_symbols, > int no_symbols_len) > { > - plan(3 + no_symbols_len); > + plan(4 + no_symbols_len); > > int len = strlen(str); > int base64_buflen = base64_bufsize(len + 1, options); > @@ -34,6 +34,11 @@ base64_test(const char *str, int options, const char *no_symbols, > free(base64_buf); > free(strbuf); > > + const char *in = "sIIpHw=="; > + int in_len = strlen(in); > + rc = base64_decode(in, in_len, NULL, 0); > + is(rc, 0, "no space in out buffer"); > + > check_plan(); > } > > diff --git a/test/unit/base64.result b/test/unit/base64.result > index cd1f2b3f6..d606772ea 100644 > --- a/test/unit/base64.result > +++ b/test/unit/base64.result > @@ -1,178 +1,206 @@ > 1..28 > *** main *** > - 1..3 > + 1..4 > ok 1 - length > ok 2 - decode length ok > ok 3 - encode/decode > + ok 4 - no space in out buffer > ok 1 - subtests > - 1..6 > + 1..7 > ok 1 - length > ok 2 - no \n symbols > ok 3 - no + symbols > ok 4 - no = symbols > ok 5 - decode length ok > ok 6 - encode/decode > + ok 7 - no space in out buffer > ok 2 - subtests > - 1..4 > + 1..5 > ok 1 - length > ok 2 - no = symbols > ok 3 - decode length ok > ok 4 - encode/decode > + ok 5 - no space in out buffer > ok 3 - subtests > - 1..4 > + 1..5 > ok 1 - length > ok 2 - no \n symbols > ok 3 - decode length ok > ok 4 - encode/decode > + ok 5 - no space in out buffer > ok 4 - subtests > - 1..3 > + 1..4 > ok 1 - length > ok 2 - decode length ok > ok 3 - encode/decode > + ok 4 - no space in out buffer > ok 5 - subtests > - 1..6 > + 1..7 > ok 1 - length > ok 2 - no \n symbols > ok 3 - no + symbols > ok 4 - no = symbols > ok 5 - decode length ok > ok 6 - encode/decode > + ok 7 - no space in out buffer > ok 6 - subtests > - 1..4 > + 1..5 > ok 1 - length > ok 2 - no = symbols > ok 3 - decode length ok > ok 4 - encode/decode > + ok 5 - no space in out buffer > ok 7 - subtests > - 1..4 > + 1..5 > ok 1 - length > ok 2 - no \n symbols > ok 3 - decode length ok > ok 4 - encode/decode > + ok 5 - no space in out buffer > ok 8 - subtests > - 1..3 > + 1..4 > ok 1 - length > ok 2 - decode length ok > ok 3 - encode/decode > + ok 4 - no space in out buffer > ok 9 - subtests > - 1..6 > + 1..7 > ok 1 - length > ok 2 - no \n symbols > ok 3 - no + symbols > ok 4 - no = symbols > ok 5 - decode length ok > ok 6 - encode/decode > + ok 7 - no space in out buffer > ok 10 - subtests > - 1..4 > + 1..5 > ok 1 - length > ok 2 - no = symbols > ok 3 - decode length ok > ok 4 - encode/decode > + ok 5 - no space in out buffer > ok 11 - subtests > - 1..4 > + 1..5 > ok 1 - length > ok 2 - no \n symbols > ok 3 - decode length ok > ok 4 - encode/decode > + ok 5 - no space in out buffer > ok 12 - subtests > - 1..3 > + 1..4 > ok 1 - length > ok 2 - decode length ok > ok 3 - encode/decode > + ok 4 - no space in out buffer > ok 13 - subtests > - 1..6 > + 1..7 > ok 1 - length > ok 2 - no \n symbols > ok 3 - no + symbols > ok 4 - no = symbols > ok 5 - decode length ok > ok 6 - encode/decode > + ok 7 - no space in out buffer > ok 14 - subtests > - 1..4 > + 1..5 > ok 1 - length > ok 2 - no = symbols > ok 3 - decode length ok > ok 4 - encode/decode > + ok 5 - no space in out buffer > ok 15 - subtests > - 1..4 > + 1..5 > ok 1 - length > ok 2 - no \n symbols > ok 3 - decode length ok > ok 4 - encode/decode > + ok 5 - no space in out buffer > ok 16 - subtests > - 1..3 > + 1..4 > ok 1 - length > ok 2 - decode length ok > ok 3 - encode/decode > + ok 4 - no space in out buffer > ok 17 - subtests > - 1..6 > + 1..7 > ok 1 - length > ok 2 - no \n symbols > ok 3 - no + symbols > ok 4 - no = symbols > ok 5 - decode length ok > ok 6 - encode/decode > + ok 7 - no space in out buffer > ok 18 - subtests > - 1..4 > + 1..5 > ok 1 - length > ok 2 - no = symbols > ok 3 - decode length ok > ok 4 - encode/decode > + ok 5 - no space in out buffer > ok 19 - subtests > - 1..4 > + 1..5 > ok 1 - length > ok 2 - no \n symbols > ok 3 - decode length ok > ok 4 - encode/decode > + ok 5 - no space in out buffer > ok 20 - subtests > - 1..3 > + 1..4 > ok 1 - length > ok 2 - decode length ok > ok 3 - encode/decode > + ok 4 - no space in out buffer > ok 21 - subtests > - 1..6 > + 1..7 > ok 1 - length > ok 2 - no \n symbols > ok 3 - no + symbols > ok 4 - no = symbols > ok 5 - decode length ok > ok 6 - encode/decode > + ok 7 - no space in out buffer > ok 22 - subtests > - 1..4 > + 1..5 > ok 1 - length > ok 2 - no = symbols > ok 3 - decode length ok > ok 4 - encode/decode > + ok 5 - no space in out buffer > ok 23 - subtests > - 1..4 > + 1..5 > ok 1 - length > ok 2 - no \n symbols > ok 3 - decode length ok > ok 4 - encode/decode > + ok 5 - no space in out buffer > ok 24 - subtests > - 1..3 > + 1..4 > ok 1 - length > ok 2 - decode length ok > ok 3 - encode/decode > + ok 4 - no space in out buffer > ok 25 - subtests > - 1..6 > + 1..7 > ok 1 - length > ok 2 - no \n symbols > ok 3 - no + symbols > ok 4 - no = symbols > ok 5 - decode length ok > ok 6 - encode/decode > + ok 7 - no space in out buffer > ok 26 - subtests > - 1..4 > + 1..5 > ok 1 - length > ok 2 - no = symbols > ok 3 - decode length ok > ok 4 - encode/decode > + ok 5 - no space in out buffer > ok 27 - subtests > - 1..4 > + 1..5 > ok 1 - length > ok 2 - no \n symbols > ok 3 - decode length ok > ok 4 - encode/decode > + ok 5 - no space in out buffer > ok 28 - subtests > *** main: done *** > diff --git a/third_party/base64.c b/third_party/base64.c > index 3350a98ff..93442c04b 100644 > --- a/third_party/base64.c > +++ b/third_party/base64.c > @@ -257,10 +257,11 @@ base64_decode_block(const char *in_base64, int in_len, > { > case step_a: > do { > - if (in_pos == in_end || out_pos >= out_end) > + if (in_pos >= in_end) > { > state->step = step_a; > - state->result = curr_byte; > + /* curr_byte is useless now */ > + /* state->result = curr_byte; */ For multi-line comment, we use the following format: /* * First line * Second line */ And leaving a commented code is not best practice. > return out_pos - out_bin; > } > fragment = base64_decode_value(*in_pos++); > @@ -268,7 +269,7 @@ base64_decode_block(const char *in_base64, int in_len, > curr_byte = (fragment & 0x03f) << 2; > case step_b: > do { > - if (in_pos == in_end || out_pos >= out_end) > + if (in_pos >= in_end) > { > state->step = step_b; > state->result = curr_byte; > @@ -276,14 +277,19 @@ base64_decode_block(const char *in_base64, int in_len, > } > fragment = base64_decode_value(*in_pos++); > } while (fragment < 0); > + if (out_pos >= out_end) > + { > + /* We are losing some data */ According to https://github.com/tarantool/tarantool/wiki/Code-review-procedure : "Start sentences from a capital letter, end with a dot." The same for the comments bellow. > + state->step = step_b; > + state->result = curr_byte; > + return out_pos - out_bin; > + } > curr_byte |= (fragment & 0x030) >> 4; > *out_pos++ = curr_byte; > curr_byte = (fragment & 0x00f) << 4; > - if (out_pos < out_end) > - *out_pos = curr_byte; > case step_c: > do { > - if (in_pos == in_end || out_pos >= out_end) > + if (in_pos >= in_end) > { > state->step = step_c; > state->result = curr_byte; > @@ -291,14 +297,19 @@ base64_decode_block(const char *in_base64, int in_len, > } > fragment = base64_decode_value(*in_pos++); > } while (fragment < 0); > + if (out_pos >= out_end) > + { > + /* We are losing some data */ > + state->step = step_c; > + state->result = curr_byte; > + return out_pos - out_bin; > + } > curr_byte |= (fragment & 0x03c) >> 2; > *out_pos++ = curr_byte; > curr_byte = (fragment & 0x003) << 6; > - if (out_pos < out_end) > - *out_pos = curr_byte; > case step_d: > do { > - if (in_pos == in_end || out_pos >= out_end) > + if (in_pos >= in_end) > { > state->step = step_d; > state->result = curr_byte; > @@ -306,6 +317,13 @@ base64_decode_block(const char *in_base64, int in_len, > } > fragment = base64_decode_value(*in_pos++); > } while (fragment < 0); > + if (out_pos >= out_end) > + { > + /* We are losing some data */ > + state->step = step_d; > + state->result = curr_byte; > + return out_pos - out_bin; > + } > curr_byte |= (fragment & 0x03f); > *out_pos++ = curr_byte; > } >
next prev parent reply other threads:[~2020-12-24 14:08 UTC|newest] Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top 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 [this message] 2020-12-25 10:39 ` Sergey Nikiforov 2020-12-25 13:10 ` Leonid Vasiliev 2020-12-24 14:14 Leonid Vasiliev
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=810f25cf-1c5e-3358-2050-4b0ce2f6ece4@tarantool.org \ --to=lvasiliev@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --cc=void@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v3 2/2] base64: improve decoder performance' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox