GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||
GeezerVison is showing github.com — src,lib,buffer: improve atob / btoa performance by XadillaX · Pull Request #38433 · nodejs/node Conversation
| ||||||||||||||||||||||||||||||
| @@ -0,0 +1,44 @@ | |||
| // Copyright Joyent, Inc. and other Node contributors. | |||
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
I think I can try to use modp b64 in Buffer's Base64 if it's really faster. This dep is extracted from Chromium. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
According to https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#dependencies policy, changes on |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@XadillaX In order to merge this, I think we should:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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.
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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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]>
There was an error while loading. Please reload this page.
|
We are thinking about using Although the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Note that it is planned that simdutf could provide fast routines for base64 in the near future. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
This pull request has been marked as stale due to 90 days of inactivity. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.