buffer: even faster atob by lemire · Pull Request #52443 · nodejs/node · GitHub
Skip to content

buffer: even faster atob#52443

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
lemire:better_atob
May 12, 2024
Merged

buffer: even faster atob#52443
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
lemire:better_atob

Conversation

@lemire

@lemire lemire commented Apr 10, 2024

Copy link
Copy Markdown
Member

Improves atob slightly more by adding a special case (one-byte non-external): this avoids the overhead of copying the string to 16-bit units. The gains are over 25% on a server with an Intel Ice Lake processor, GCC 12. I expect that gains of the same order will be observed generally.

This is a follow-up to #52381 by @anonrig

And yes, I am aware that the use of atob is discouraged but these performance gains are expected to be basically free.

                                          confidence improvement accuracy (*)   (**)  (***)
buffers/buffer-atob.js n=1000000 size=128        ***     26.48 %       ±1.52% ±2.10% ±2.91%
buffers/buffer-atob.js n=1000000 size=16         ***     35.41 %       ±0.53% ±0.75% ±1.06%
buffers/buffer-atob.js n=1000000 size=32         ***     32.09 %       ±1.04% ±1.45% ±2.03%
buffers/buffer-atob.js n=1000000 size=64         ***     31.39 %       ±0.54% ±0.75% ±1.03%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 4 comparisons, you can thus expect the following amount of false-positive results:
  0.20 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.04 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

@lemire lemire requested a review from anonrig April 10, 2024 01:34
@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Apr 10, 2024
@anonrig anonrig added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Apr 10, 2024
benjamingr
benjamingr approved these changes Apr 10, 2024
View reviewed changes

@benjamingr benjamingr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly don't see the point in these tbh (since while we're improving our performance we're steering users towards methods that are only there for compat) and believe your time would be better spent optimizing paths that are hot or users.

But this is open source, this is still an improvement and actual changes LGTM.

@lemire

lemire commented Apr 10, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

@anonrig anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 10, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 10, 2024
@nodejs-github-bot

nodejs-github-bot commented Apr 10, 2024

Copy link
Copy Markdown
Collaborator

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

@benjamingr

benjamingr commented Apr 11, 2024

Copy link
Copy Markdown
Member

I do understand people who criticize the optimization of legacy methods, and if we only improved the legacy methods, I would take to heart the criticism, but everything is getting faster.

Yes sorry for the confusion after reviewing the ones related to buffer this makes more sense. Optimizing in general is good the only issue was with optimizing only the APIs we don't want users to use - so these changes LGTM (and sorry for not editing my CR here after reviewing the Buffer changes!)

@nodejs-github-bot

nodejs-github-bot commented Apr 11, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Apr 12, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Apr 12, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Apr 14, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Apr 14, 2024

Copy link
Copy Markdown
Collaborator

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

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label May 10, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 10, 2024
@nodejs-github-bot

nodejs-github-bot commented May 10, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented May 12, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented May 12, 2024

Copy link
Copy Markdown
Collaborator

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

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label May 12, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 12, 2024
@nodejs-github-bot nodejs-github-bot merged commit 11c90c1 into nodejs:main May 12, 2024
@nodejs-github-bot

nodejs-github-bot commented May 12, 2024

Copy link
Copy Markdown
Collaborator

targos pushed a commit that referenced this pull request May 13, 2024
@lemire @targos
buffer: even faster atob
67fcb6b 
PR-URL: #52443
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@targos targos mentioned this pull request May 13, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@lemire @marco-ippolito
buffer: even faster atob
4979981 
PR-URL: #52443
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@marco-ippolito marco-ippolito mentioned this pull request Jun 17, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@lemire @marco-ippolito
buffer: even faster atob
5fbb6c0 
PR-URL: #52443
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@lemire @marco-ippolito
buffer: even faster atob
8b3e83b 
PR-URL: #52443
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
@lemire @soophoo
buffer: even faster atob
0a3cec9 
PR-URL: nodejs#52443
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
@lemire @bmeck
buffer: even faster atob
8958d92 
PR-URL: nodejs#52443
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@chee chee mentioned this pull request Feb 20, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingr benjamingr benjamingr approved these changes
@anonrig anonrig anonrig approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@lemire @nodejs-github-bot @benjamingr @anonrig @aduh95