-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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,lib,buffer: improve atob / btoa performance #38433
base: main
Are you sure you want to change the base?
Conversation
Extract `modp_b64` dependence from Chromium to improve performance of `atob()` and `btoa()`. Refs: https://github.com/chromium/chromium/tree/92.0.4490.1/third_party/modp_b64
benchmark/buffers/buffer-atob.js
Outdated
@@ -0,0 +1,44 @@ | |||
// Copyright Joyent, Inc. and other Node contributors. |
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.
New files added should never have a copyright header added to them.
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.
Despite the performance boost, I'm generally -1 on introducing a new dependency and a different codepath for base64 encoding/decoding only for atob/btoa. If anything, we should be looking at improving the base64/base64url encoding performance for Buffer
. /cc @addaleax
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.
Agreed, this is far too much optimization for methods that should never be used.
I think I can try to use modp b64 in Buffer's Base64 if it's really faster. This dep is extracted from Chromium. |
According to https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#dependencies policy, changes on |
@addaleax @jasnell I've updated, removing modp_b64 and using $ ./node benchmark/run.js --filter buffer-btoa.js buffers
buffers/buffer-btoa.js
buffers/buffer-btoa.js n=32 len=67108864: 3.3501380480392817
$ ./node benchmark/run.js --filter buffer-atob.js buffers
buffers/buffer-atob.js
buffers/buffer-atob.js n=32 len=65536: 6,966.175950365997 And I've found that modp_b64's decoding is faster then code in Node.js'. Maybe we can merge this PR first (if possible), and then use modp_b64 to instead of Node.js' current Base64 code. |
@XadillaX In order to merge this, I think we should:
|
The C++-backed implementations is just fasten the function to resolve: // TODO(@jasnell): The implementation here has not been performance
// optimized in any way. And this is not a large change, just call |
Then let’s remove the TODO comment, or edit it to make clear that there’s not actually anything to do there. Making these functions faster tells people that it’s okay to use them. It’s not, therefore let’s not worry about their performance at all. |
@@ -60,6 +60,7 @@ void OnFatalError(const char* location, const char* message); | |||
V(ERR_INVALID_ARG_VALUE, TypeError) \ | |||
V(ERR_OSSL_EVP_INVALID_DIGEST, Error) \ | |||
V(ERR_INVALID_ARG_TYPE, TypeError) \ | |||
V(ERR_INVALID_CHARACTER, RangeError) \ |
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.
DOMException are expected in atob/btoa per https://html.spec.whatwg.org/multipage/webappapis.html#dom-atob
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.
It's wrapped in JavaScript side.
btw, this PR seems won't be merged according to @addaleax's suggestion.
@addaleax I've tried to use modp_b64 to instead of current and found the performance is almost the same (c975dff...XadillaX:base64-perf). So the way to improve this PR is just to make But as you said before, there's no need to improve them. That means the only thing I should do is to remove @jasnell's TODO comment? /cc @jasnell |
Refs: nodejs#38433 (comment) PR-URL: nodejs#38548 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Zijian Liu <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Refs: #38433 (comment) PR-URL: #38548 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Zijian Liu <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Refs: #38433 (comment) PR-URL: #38548 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Zijian Liu <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Refs: #38433 (comment) PR-URL: #38548 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Zijian Liu <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Refs: #38433 (comment) PR-URL: #38548 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Zijian Liu <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Refs: #38433 (comment) PR-URL: #38548 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Zijian Liu <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
We are thinking about using Although the |
Note that it is planned that simdutf could provide fast routines for base64 in the near future. |
The simdutf library (already part of Node.js) will have accelerated base64 support in the next release. Decoding will follow the WHATWG forgiving-base64 specification so that ASCII white spaces are allowed in base64 content. |
Extract
modp_b64
dependence from Chromium to improve performance ofatob()
andbtoa()
.Refs: https://github.com/chromium/chromium/tree/92.0.4490.1/third_party/modp_b64
Benchmark
Old logic
Logic of this PR