buffer: improve `atob` performance by anonrig · Pull Request #52381 · nodejs/node

anonrig · 2024-04-05T21:43:53Z

The code is 99% written by @lemire, I just glued them all together.

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

 confidence improvement accuracy (*) (**) (***)
buffers/buffer-atob.js n=1000000 size=16 *** 457.77 % ±3.28% ±4.42% ±5.86%
buffers/buffer-atob.js n=1000000 size=256 *** 2505.57 % ±15.69% ±21.14% ±28.07%
buffers/buffer-atob.js n=1000000 size=64 *** 1213.33 % ±3.98% ±5.36% ±7.11%
Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 3 comparisons, you can thus expect the following amount of false-positive results:
0.15 false positives, when considering a 5% risk acceptance (*, **, ***),
0.03 false positives, when considering a 1% risk acceptance (**, ***),
0.00 false positives, when considering a 0.1% risk acceptance (***)

nodejs-github-bot · 2024-04-05T21:43:58Z

Review requested:

@nodejs/security-wg

github-actions · 2024-04-05T21:49:16Z

The notable-changePRs with changes that should be highlighted in changelogs. label has been added by @anonrig.

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.

Jarred-Sumner · 2024-04-05T21:52:18Z

nice

H4ad

Awesome 🚀

nodejs-github-bot · 2024-04-05T22:02:40Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58146/

nodejs-github-bot · 2024-04-05T22:43:03Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58147/

MattIPv4 · 2024-04-05T23:00:29Z

I notice that a comment referencing #38433 (comment) was removed.

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.

Has the position changed, that these should now be optimized?

lemire · 2024-04-05T23:37:57Z

@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.

MattIPv4 · 2024-04-05T23:39:36Z

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

anonrig · 2024-04-05T23:42:30Z

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).

richardlau · 2024-04-06T00:13:42Z

I notice that a comment referencing #38433 (comment) was removed.
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.
Has the position changed, that these should now be optimized?

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.

richardlau · 2024-04-06T00:14:25Z

This pull-request introduces blazing fast atob implementation which is more secure and faster.

Can you expand on/explain "more secure"?

lemire · 2024-04-06T00:41:37Z

@richardlau From the linked issue:

This is a DoS vector to the unaware user and irresponsible IMO.

lemire · 2024-04-06T00:42:06Z

(Note that the above comment is quote from the linked issue, not something @anonrig wrote.)

mcollina

lgtm, but not much useful in practice.

nodejs-github-bot · 2024-04-08T00:26:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58198/

nodejs-github-bot · 2024-04-08T01:47:22Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58199/

nodejs-github-bot · 2024-04-08T02:02:44Z

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)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
deps: update simdutf to 5.2.3

PR-URL: #52381
Fixes: #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

[detached HEAD ce2fd04108] deps: update simdutf to 5.2.3
Author: Yagiz Nizipli yagiz@nizipli.com
Date: Thu Apr 4 17:53:41 2024 -0400
2 files changed, 2204 insertions(+), 959 deletions(-)
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
buffer: use simdutf for atob implementation

Co-authored-by: Daniel Lemire daniel@lemire.me
PR-URL: #52381
Fixes: #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

[detached HEAD d8a53adcd5] buffer: use simdutf for atob implementation
Author: Yagiz Nizipli yagiz@nizipli.com
Date: Thu Apr 4 18:03:17 2024 -0400
3 files changed, 93 insertions(+), 73 deletions(-)
create mode 100644 benchmark/buffers/buffer-atob.js

Successfully rebased and updated refs/heads/main.

✖ ce2fd04108b1993cda354c46404010dea3e5decc
✔ 0:0 no Co-authored-by metadata co-authored-by-is-trailer
✖ 2:7 Pull request URL must reference a comment or discussion. fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 1:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length
✖ d8a53adcd5d3d6c299c9f95872c827e6c3d81363
✔ 0:0 Co-authored-by is a trailer co-authored-by-is-trailer
✖ 3:7 Pull request URL must reference a comment or discussion. fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 2:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length
ℹ Please fix the commit message and try again.
Please manually ammend the commit message, by running
git commit --amend
Once commit message is fixed, finish the landing command running
git node land --continue

https://github.com/nodejs/node/actions/runs/8593473401

Uzlopak

LGTM

nodejs-github-bot · 2024-04-08T12:43:30Z

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>

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>

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Apr 5, 2024

anonrig force-pushed the deps/simdutf branch from e031760 to e53e36dCompare April 5, 2024 21:46

anonrig added buffer Issues and PRs related to the buffer subsystem. notable-change PRs with changes that should be highlighted in changelogs. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Apr 5, 2024

lemire approved these changes Apr 5, 2024
View reviewed changes

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 5, 2024

H4ad approved these changes Apr 5, 2024
View reviewed changes

Comment threadresult.csv Outdated
Comment threadbuffer.csv Outdated

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 5, 2024

deps: update simdutf to 5.2.3
b6619c2

anonrig force-pushed the deps/simdutf branch from e53e36d to 79973d9Compare April 5, 2024 22:05

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 5, 2024

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 5, 2024

lemire reviewed Apr 6, 2024
View reviewed changes

Comment threadsrc/node_buffer.cc Outdated

mcollina approved these changes Apr 6, 2024
View reviewed changes

ronag approved these changes Apr 6, 2024
View reviewed changes

anonrig mentioned this pull request Apr 8, 2024
deps: update simdutf to 5.2.3 #52396
Closed

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 8, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 8, 2024

nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Apr 8, 2024

panva approved these changes Apr 8, 2024
View reviewed changes

Uzlopak approved these changes Apr 8, 2024
View reviewed changes

anonrig added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Apr 8, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 8, 2024

nodejs-github-bot closed this Apr 8, 2024

Uzlopak mentioned this pull request Apr 8, 2024
buffer: fix DoS vector in atob #51670
Closed

lemire mentioned this pull request Apr 8, 2024
performance of encodings (hex, base64, base64url) nodejs/performance#128
Open

richardlau mentioned this pull request Apr 9, 2024
Windows release machines are all offline nodejs/build#3674
Closed

lemire mentioned this pull request Apr 10, 2024
buffer: even faster atob #52443
Merged

targos mentioned this pull request Apr 11, 2024
Compiler failure on ARM64 Windows simdutf/simdutf#407
Closed

RafaelGSS mentioned this pull request Apr 15, 2024
2024-04-24, Version 22.0.0 (Current) #52505
Merged

maximilianschmid mentioned this pull request Apr 25, 2024
NodeJS 22 breaks vinyl-js gulpjs/vinyl-fs#350
Closed

marco-ippolito mentioned this pull request May 2, 2024
v20.13.0 proposal #52793
Merged

richardlau mentioned this pull request May 16, 2024
v18.20.3 proposal #53028
Merged

Uh oh!

Uh oh!

Conversation

anonrig commented Apr 5, 2024• edited by richardlau Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

Uh oh!

nodejs-github-bot commented Apr 5, 2024

Uh oh!

github-actionsBot commented Apr 5, 2024

Uh oh!

Jarred-Sumner commented Apr 5, 2024

Uh oh!

H4ad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 5, 2024

Uh oh!

nodejs-github-bot commented Apr 5, 2024

Uh oh!

MattIPv4 commented Apr 5, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lemire commented Apr 5, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MattIPv4 commented Apr 5, 2024

Uh oh!

anonrig commented Apr 5, 2024

Uh oh!

richardlau commented Apr 6, 2024

Uh oh!

richardlau commented Apr 6, 2024

Uh oh!

lemire commented Apr 6, 2024

Uh oh!

lemire commented Apr 6, 2024

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 8, 2024

Uh oh!

nodejs-github-bot commented Apr 8, 2024

Uh oh!

nodejs-github-bot commented Apr 8, 2024

PR-URL: #52381 Fixes: #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

Successfully rebased and updated refs/heads/main.

Uh oh!

Uzlopak left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

15 participants

anonrig commented Apr 5, 2024•
edited by richardlau
Loading

MattIPv4 commented Apr 5, 2024•
edited
Loading

lemire commented Apr 5, 2024•
edited
Loading

PR-URL: #52381
Fixes: #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