-
-
Notifications
You must be signed in to change notification settings - Fork 233
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
Drop base64-js dependency, and optimise Buffer.write #349
base: master
Are you sure you want to change the base?
Conversation
Added a new commit to further optimize base64Write. This logic is basically how the node.js c++ code works internally. It requires no string preprocessing ( New Numbers:
|
@chjj can you break this into two pull requests? |
b6021f4
to
42bc6ba
Compare
Done. Force pushed so this is now the isolated PR. edit: I can rebase on master and force push again once you merge #352 if need be. |
42bc6ba
to
5be2b22
Compare
New base64 implementation using atob/btoa pushed. |
5be2b22
to
61582b4
Compare
if (typeof global !== 'undefined' && global && global.Math === Math && | ||
typeof global.Buffer === 'function' && global.Buffer.prototype && | ||
typeof global.Buffer.prototype.asciiSlice === 'function') { | ||
const NodeBuffer = global.Buffer | ||
|
||
_atob = function atob (str) { | ||
return NodeBuffer.from(str, 'base64').toString('binary') | ||
} | ||
|
||
_btoa = function btoa (str) { | ||
return NodeBuffer.from(str, 'binary').toString('base64') | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shim leads to some untested code paths when testing in node.js (e.g. atob
won't throw on a base64url string like it should).
Unfortunately there's no feature in tape to set a global or somehow signal to this piece of code that we are testing and it should use the atob/btoa
functions provided by node.js instead of shimming.
I see a few solutions:
- Create
tape-wrapper.js
which does something like:global.BUFFER_TEST = true; require('tape/bin/tape');
and add&& !global.BUFFER_TEST
to the already-gnarly if statement. Run tests with./tape-wrapper.js
instead oftape
. - Rewrite the tests in mocha which allows you to assign globals on boot.
- Only shim
atob/btoa
iftypeof atob === 'undefined'
. This fixes the tests and maintains functionality for pre-16.0.0 nodes, but creates a weird situation where older versions of node are actually faster than newer ones. - Remove this entire thing. Always use
atob/btoa
. Tests will be fixed. Older nodes will break. Newer nodes will be slow. Benchmarks run inside node.js will be inaccurate. - Beseech the node.js devs to fix
atob
/btoa
which eventually does away with all the problems I just listed. - Switch back to the custom base64 implementation which is almost as fast as
atob
in browsers and replicates the exact node.js behavior with no downgrade in perf on base64url etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just occurred to me that we could do something hacky like checking:
&& global.process.argv[1].slice(-4) !== 'tape'
to exclude testing from the shims.
After thinking about it, all of this |
@dcousens, I think I will switch this PR back to my custom base64 implementation. The more I think about it, the more I don't like
Number 7 is one I recently thought of. I really really like the idea of compiling bundles for quickjs (I'm thinking of adding a quickjs target to my bundler to convert the quickjs api to the node.js api, but that's a different story). The benefits of a custom implementation are as follows:
The downsides:
|
I understand your plight for each of the points, but I don't know if this library should be a reference for this kind of functionality. The primary reason this library exists is to support cross-compatibility of the native API's, not to be an improved implementation of their shared functionality.
I don't understand this, we should know that the input string is This pull request is difficult to quickly review in the limited time I have, when you have touched and moved the |
Fair enough.
We can use that as a hint, but the
Yeah, I know I've been bugging you a lot lately. Sorry about that. Thank you for your attentiveness this past week. I find it's best to look at the diff of each individual commit, but I see your point. I'll try to get it looking better. |
Most string
write
functions (with the exception ofhexWrite
) were allocating arrays before copying them onto the buffer. This resulted in significant performance overhead.This PR modifies said functions to write directly to the buffer. In addition, it drops the dependency on base64-js and adds support for the
base64url
encoding.The numbers are as follows:
There are some massive speedups here, particularly with
asciiWrite
,ucs2Write
, andutf8Write
.I was disappointed to see base64 only has a marginal speedup, however, there is a significant speedup for
base64Slice
andbyteLength('base64')
. Likewise,byteLength('utf8')
also received a significant speedup.