| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
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
| @@ -0,0 +1,44 @@ | |||
| // Copyright Joyent, Inc. and other Node contributors. | |||
There was a problem hiding this comment.
New files added should never have a copyright header added to them.
Sorry, something went wrong.
There was a problem hiding this comment.
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 a problem hiding this comment.
Agreed, this is far too much optimization for methods that should never be used.
Sorry, something went wrong.
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.
|
According to https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#dependencies policy, changes on deps/* should be sent to upstream first. In that case, as Node.js already have their own implementation of base64 functionality, adding a new dependency for base64 solely doesn't seem to be the best effort to reduce the burden of collaborators. While the base64 codebase isn't quite large, are there any improvements that are worth to be made on Node.js own base64 implementation? |
Sorry, something went wrong.
|
@addaleax @jasnell I've updated, removing modp_b64 and using base64_encode. And got the new benchmark: $ ./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.175950365997And 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.
|
@XadillaX In order to merge this, I think we should:
|
Sorry, something went wrong.
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 base64_encode directly to avoid do unnecessary Buffer call. |
Sorry, something went wrong.
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.
| 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.
DOMException are expected in atob/btoa per https://html.spec.whatwg.org/multipage/webappapis.html#dom-atob
Sorry, something went wrong.
There was a problem hiding this comment.
It's wrapped in JavaScript side.
btw, this PR seems won't be merged according to @addaleax's suggestion.
Sorry, something went wrong.
|
@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 atob() / btoa() using base64_encode / base64_decode directly. 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.
Refs: nodejs#38433 (comment) PR-URL: nodejs#38548 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Zijian Liu <lxxyxzj@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Refs: #38433 (comment) PR-URL: #38548 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Zijian Liu <lxxyxzj@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Refs: #38433 (comment) PR-URL: #38548 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Zijian Liu <lxxyxzj@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Refs: #38433 (comment) PR-URL: #38548 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Zijian Liu <lxxyxzj@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Refs: #38433 (comment) PR-URL: #38548 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Zijian Liu <lxxyxzj@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Refs: #38433 (comment) PR-URL: #38548 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Zijian Liu <lxxyxzj@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
|
We are thinking about using atob and btoa in feross/buffer#339 for web compatibility. Although the buffer module is directed at browsers, if user code unintentionally finds it's way to using the buffer module in a node environment and subsequently ends up using atob or btoa, this is only a loss for users of node, not our library. |
Sorry, something went wrong.
|
Note that it is planned that simdutf could provide fast routines for base64 in the near future. |
Sorry, something went wrong.
|
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.
|
This pull request has been marked as stale due to 90 days of inactivity. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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
Old logic
Logic of this PR