FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

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

/ node Public

buffer: even faster atob - #52443

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
lemire:better_atob
May 12, 2024
ConversationCommits (2)ChecksFiles changed
Merged

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

Conversation

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 requested a review from anonrig April 10, 2024 01:34
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 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 approved these changes Apr 10, 2024
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Apr 10, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

@benjamingr

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.

I think that you have now realized that this is part of a series of pull requests which do include the recommended methods which do get optimized...

#52428 (comment)

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.

If you look at the code, it is really no harder to optimize everything. It is mostly the same code. The dependency does the heavy lifting...

I submit to you that there is a good justification for optimizing the legacy functions at the same time as we optimize the recommended functions: testing. The way we proceeded, we get thorough testing because the same underlying functions are tested multiple times in different ways.

As an aside, the legacy functions have functionality that the recommended functions do not have. They are validating, among other things. The recommended functions will accept almost any garbage. That's a design choice and I respect it but some users are likely to be using the legacy methods because they offer more safety due to the validation.

anonrig added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 10, 2024
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 commented Apr 10, 2024

Copy link
Copy Markdown
Collaborator

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

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 commented Apr 11, 2024

Copy link
Copy Markdown
Collaborator

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

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 commented Apr 12, 2024

Copy link
Copy Markdown
Collaborator

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

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 commented Apr 14, 2024

Copy link
Copy Markdown
Collaborator

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

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label May 10, 2024
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 commented May 10, 2024

Copy link
Copy Markdown
Collaborator

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

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 commented May 12, 2024

Copy link
Copy Markdown
Collaborator

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

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label May 12, 2024
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 merged commit 11c90c1 into nodejs:main May 12, 2024

nodejs-github-bot commented May 12, 2024

Copy link
Copy Markdown
Collaborator

Landed in 11c90c1

targos pushed a commit that referenced this pull request May 13, 2024
buffer: even faster atob
67fcb6b 
PR-URL: #52443
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos mentioned this pull request May 13, 2024
Merged
marco-ippolito mentioned this pull request Jun 17, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
buffer: even faster atob
8b3e83b 
PR-URL: #52443
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
eliphazbouye pushed a commit to eliphazbouye/node that referenced this pull request Jun 20, 2024
buffer: even faster atob
0a3cec9 
PR-URL: nodejs#52443
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
chee mentioned this pull request Feb 20, 2025
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

benjamingr benjamingr approved these changes
anonrig anonrig approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Back | FazBrowse Home | New Git URL