-
Notifications
You must be signed in to change notification settings - Fork 28.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
buffer: improve atob
performance
#52381
Conversation
Review requested:
|
The
notable-change
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section. |
nice |
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.
Awesome 🚀
I notice that a comment referencing #38433 (comment) was removed.
Has the position changed, that these should now be optimized? |
@MattIPv4 I won't comment directly on your objection, but I'd like to point out that the same approach can be used, and will be used, to accelerate the recommended Buffer functions. There is, however, a significant difference between atob and the recommended Buffer functions: atob validates the input. The recommended Buffer functions offer no validation and are underspecified. |
To be clear, I don't object, I'm all for performance increase! Just wanted to check the overall position of Node was that this should be optimized now, when before it seems it wasn't |
atob is deprecated and not recommended to be used (don't know why, but the reasoning doesn't matter for the purpose of this PR). |
It was discussed in a TSC meeting -- nodejs/TSC#1503, recording: https://www.youtube.com/watch?v=B5phSZ7jjwk. I can't seem to find the meeting minutes in the TSC repo -- the linked PR in the issue is for the following week's meeting. My recollection is there was consensus in the meeting that #51670 could land (not sure why it hasn't) on the basis that it was purely JS and fairly minimal. Use of atob is still discouraged. |
Can you expand on/explain "more secure"? |
@richardlau From the linked issue:
|
(Note that the above comment is quote from the linked issue, not something @anonrig wrote.) |
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.
lgtm, but not much useful in practice.
Commit Queue failed- Loading data for nodejs/node/pull/52381 ✔ Done loading data for nodejs/node/pull/52381 ----------------------------------- PR info ------------------------------------ Title buffer: improve `atob` performance (#52381) Author Yagiz Nizipli (@anonrig) Branch anonrig:deps/simdutf -> nodejs:main Labels buffer, lib / src, author ready, needs-ci, commit-queue-rebase Commits 2 - deps: update simdutf to 5.2.3 - buffer: use simdutf for `atob` implementation Committers 1 - Yagiz Nizipli PR-URL: https://github.com/nodejs/node/pull/52381 Fixes: https://github.com/nodejs/node/pull/51670 Reviewed-By: Daniel Lemire Reviewed-By: Vinícius Lourenço Claro Cardoso Reviewed-By: Matteo Collina Reviewed-By: Robert Nagy Reviewed-By: Benjamin Gruenbaum ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/52381 Fixes: https://github.com/nodejs/node/pull/51670 Reviewed-By: Daniel Lemire Reviewed-By: Vinícius Lourenço Claro Cardoso Reviewed-By: Matteo Collina Reviewed-By: Robert Nagy Reviewed-By: Benjamin Gruenbaum -------------------------------------------------------------------------------- ℹ This PR was created on Fri, 05 Apr 2024 21:43:53 GMT ✔ Approvals: 5 ✔ - Daniel Lemire (@lemire): https://github.com/nodejs/node/pull/52381#pullrequestreview-1984251249 ✔ - Vinícius Lourenço Claro Cardoso (@H4ad): https://github.com/nodejs/node/pull/52381#pullrequestreview-1984254833 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/52381#pullrequestreview-1984441422 ✔ - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/52381#pullrequestreview-1984457362 ✔ - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/52381#pullrequestreview-1984572522 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-04-08T01:47:22Z: https://ci.nodejs.org/job/node-test-pull-request/58199/ - Querying data for job/node-test-pull-request/58199/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 52381 From https://github.com/nodejs/node * branch refs/pull/52381/merge -> FETCH_HEAD ✔ Fetched commits as 128c60d90609..a790e5857196 -------------------------------------------------------------------------------- [main 1b747b2803] deps: update simdutf to 5.2.3 Author: Yagiz Nizipli Date: Thu Apr 4 17:53:41 2024 -0400 2 files changed, 2204 insertions(+), 959 deletions(-) Auto-merging src/node_buffer.cc [main 225c7e7795] buffer: use simdutf for `atob` implementation Author: Yagiz Nizipli Date: Thu Apr 4 18:03:17 2024 -0400 3 files changed, 93 insertions(+), 73 deletions(-) create mode 100644 benchmark/buffers/buffer-atob.js ✔ Patches applied There are 2 commits in the PR. Attempting autorebase. Rebasing (2/4)https://github.com/nodejs/node/actions/runs/8593473401 |
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.
LGTM
Landed in 08609b5...6f504b7 |
PR-URL: #52381 Refs: #51670 Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Co-authored-by: Daniel Lemire <daniel@lemire.me> PR-URL: #52381 Refs: #51670 Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
PR-URL: #52381 Refs: #51670 Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
PR-URL: #52381 Refs: #51670 Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Co-authored-by: Daniel Lemire <daniel@lemire.me> PR-URL: #52381 Refs: #51670 Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
PR-URL: #52381 Refs: #51670 Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Co-authored-by: Daniel Lemire <daniel@lemire.me> PR-URL: #52381 Refs: #51670 Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
This pull-request introduces a much faster atob implementation, and solves solves some security aspects of the previous implementation. Usage of atob is still discouraged.
Refs: #51670
Benchmarks