FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: make base64 decoding 10-15% faster by bnoordhuis · Pull Request #2193 · nodejs/node · GitHub

/ node Public

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

Merged
bnoordhuis merged 2 commits into
nodejs:masterfrom
bnoordhuis:optimize-base64-decode
Jul 25, 2015
ConversationCommits (2)ChecksFiles changed
Merged

src: make base64 decoding 10-15% faster#2193
bnoordhuis merged 2 commits into
nodejs:masterfrom
bnoordhuis:optimize-base64-decode

Conversation

bnoordhuis commented Jul 16, 2015

Copy link
Copy Markdown
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 added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 16, 2015
thefourtheye reviewed Jul 16, 2015
View reviewed changes
Comment thread src/string_bytes.cc Outdated

thefourtheye Jul 16, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

very minor nit. Is four spaces here, okay?

thefourtheye Jul 16, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

bnoordhuis Jul 16, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

bnoordhuis Jul 17, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 force-pushed the optimize-base64-decode branch from 7f8acb6 to a5df468 Compare July 17, 2015 08:42

bnoordhuis commented Jul 17, 2015

Copy link
Copy Markdown
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 commented Jul 17, 2015

Copy link
Copy Markdown
Contributor

LGTM

ronkorving commented Jul 21, 2015

Copy link
Copy Markdown
Contributor

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

src: make base64 decoding 50% faster
8fd3ce1 
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>
test: fix valgrind uninitialized memory warning
ac70bc8 
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 force-pushed the optimize-base64-decode branch from a5df468 to ac70bc8 Compare July 25, 2015 17:13
bnoordhuis closed this Jul 25, 2015
bnoordhuis deleted the optimize-base64-decode branch July 25, 2015 17:13
bnoordhuis merged commit ac70bc8 into nodejs:master Jul 25, 2015

bnoordhuis commented Jul 25, 2015

Copy link
Copy Markdown
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 commented Jul 25, 2015

Copy link
Copy Markdown

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

Fishrock123 commented Jul 26, 2015

Copy link
Copy Markdown
Contributor

@YuriSolovyov 2.5.0+ (both) :)

This was referenced Jul 27, 2015
Merged
Closed
aqrln mentioned this pull request Apr 4, 2017
Closed
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Milestone

No milestone

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

Back | FazBrowse Home | New Git URL