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

Optimize the base64.decode function. #185

Merged
merged 1 commit into from Jun 26, 2015
Merged

Conversation

@fitzgen
Copy link
Contributor

@fitzgen fitzgen commented Jun 17, 2015

This function is incredibly hot while parsing mappings. By avoiding property
gets and throwing errors in this function, we get about 2000 millisecond
improvement on the mean time to parse our source map in bench.html.

r? @ejpbruel

@ai
Copy link

@ai ai commented Jun 17, 2015

BTW, why you didn’t use base64 function from some npm library?

@fitzgen
Copy link
Contributor Author

@fitzgen fitzgen commented Jun 17, 2015

At the time this lib was originally written, I'm not sure there was a good solution on npm. It was easy to implement it myself.

The requirement of running on node, in browsers, and within Firefox is another consideration, but not a show stopper.

if (aChar in charToIntMap) {
return charToIntMap[aChar];
exports.decode = function (charCode) {
var bigA = 65; // 'A'

This comment has been minimized.

@ejpbruel

ejpbruel Jun 24, 2015
Contributor

Is the JIT clever enough not to reinitialize these variables on each call to decode? If not, these constants should be moved outside the function


// 26 - 51: abcdefghijklmnopqrstuvwxyz
if (littleA <= charCode && charCode <= littleZ) {
return (charCode - littleA + 26);

This comment has been minimized.

@ejpbruel

ejpbruel Jun 24, 2015
Contributor

You introduced several named variables for magic constants above, only to use an unnamed magic number here. Would be nice if we could come up with a name for these as well.

@ejpbruel
Copy link
Contributor

@ejpbruel ejpbruel commented Jun 24, 2015

Patch looks good to me. r+

It's a bit surprising that a property lookup is so much slower than a bunch of hardcoded if blocks. Intuitively, I'd expect table lookup to always win out over explicit branching (when not taking cache effects into account).

In any case, I wonder if doing a lookup based on the character's charCode, rather than it's string representation, would be any faster. Array element lookup is supposed to be faster than object property lookup, so I guess it depends on what the JIT can optimize. I expect it won't be faster than the inline code you just wrote, but it might be worth looking into at some point.

This function is incredibly hot while parsing mappings. By avoiding property
gets and throwing errors in this function, we get about 2000 millisecond
improvement on the mean time to parse our source map in bench.html.
@fitzgen fitzgen force-pushed the fitzgen:decode-base64 branch from a5f0113 to bc84adb Jun 26, 2015
fitzgen added a commit that referenced this pull request Jun 26, 2015
Optimize the base64.decode function.
@fitzgen fitzgen merged commit fb5d2b3 into mozilla:master Jun 26, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fitzgen fitzgen deleted the fitzgen:decode-base64 branch Dec 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.