Skip to content
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.

Sign up for GitHub

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

Jump to bottom

buffer: even faster atob #52443

Merged
merged 2 commits into from
May 12, 2024
Merged

buffer: even faster atob #52443

merged 2 commits into from
May 12, 2024

Conversation

lemire
Copy link
Member

@lemire lemire commented Apr 10, 2024

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
Copy link
Member

@benjamingr benjamingr left a comment

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
Copy link
Member Author

lemire commented Apr 10, 2024 •
edited
Loading

@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 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
Copy link
Collaborator

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

@benjamingr
Copy link
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
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
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
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
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
66 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 11c90c1

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>
eliphazbouye pushed a commit to eliphazbouye/node that referenced this pull request Jun 20, 2024
@lemire @eliphazbouye
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anonrig anonrig anonrig approved these changes

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