Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: make base64 decoding 10-15% faster #2193

Merged
merged 2 commits into from
Jul 25, 2015

Conversation

bnoordhuis
Copy link
Member

Make the inner loop execute fewer compare-and-branch executions per
processed byte, resulting in a 10-15% speedup.

This coincidentally fixes an out-of-bounds read:

while (unbase64(*src) < 0 && src < srcEnd)

Should have read:

while (src < srcEnd && unbase64(*src) < 0)

But this commit removes the offending code altogether.

Fixes: #2166

R=@trevnorris?

CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/155/

@mscdex mscdex added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 16, 2015
@@ -150,62 +150,83 @@ static const int unbase64_table[] =
-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1
};
#define unbase64(x) unbase64_table[(uint8_t)(x)]
#define unbase64(x) \
static_cast<uint8_t>(unbase64_table[static_cast<uint8_t>(x)])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very minor nit. Is four spaces here, okay?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we really cast the result to uint8_t when the actual data is int8_t?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The alternative was to change the type of the list elements to uint8_t but then I'd also have to change all the -1 to 255 to squelch compiler warnings. It would make the diff a lot noisier for no reason; conversion from signed to unsigned is well-defined.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is four spaces here, okay?

I don't think we really have a convention for that but I'll change it to two spaces before landing.

@bnoordhuis bnoordhuis force-pushed the optimize-base64-decode branch from 7f8acb6 to a5df468 Compare July 17, 2015 08:42
@bnoordhuis
Copy link
Member Author

Incorporated feedback, PTAL.

@trevnorris Good suggestion about force-flattening the string first. I'm confident saying now that it's actually 50% faster. :-)

@trevnorris
Copy link
Contributor

LGTM

@ronkorving
Copy link
Contributor

@bnoordhuis then I guess you can rename this PR :) great job!

Make the inner loop execute fewer compare-and-branch executions per
processed byte, resulting in a 50% or more speedup.

This coincidentally fixes an out-of-bounds read:

    while (unbase64(*src) < 0 && src < srcEnd)

Should have read:

    while (src < srcEnd && unbase64(*src) < 0)

But this commit removes the offending code altogether.

Fixes: nodejs#2166
PR-URL: nodejs#2193
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
parallel/test-buffer called `Buffer.prototype.toString()` on a buffer
with uninitialized memory.  Call `Buffer.prototype.fill()` on it first.

PR-URL: nodejs#2193
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@bnoordhuis bnoordhuis force-pushed the optimize-base64-decode branch from a5df468 to ac70bc8 Compare July 25, 2015 17:13
@bnoordhuis bnoordhuis closed this Jul 25, 2015
@bnoordhuis bnoordhuis deleted the optimize-base64-decode branch July 25, 2015 17:13
@bnoordhuis bnoordhuis merged commit ac70bc8 into nodejs:master Jul 25, 2015
@bnoordhuis
Copy link
Member Author

Landed in 8fd3ce1 and ac70bc8 with hex values, thanks everyone.

I only added @trevnorris in the Reviewed-By because he was the only one to formally LGTM it.

@YurySolovyov
Copy link

Is it going to be in 2.5.0, 3.0 or both?

@Fishrock123
Copy link
Contributor

@YuriSolovyov 2.5.0+ (both) :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid read in node::base64_decode<char>(char*, unsigned long, char const*, unsigned long)
10 participants