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.

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

lib: improve async_context_frame structure #54239

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

Qard
Copy link
Member

@Qard Qard commented Aug 7, 2024

This is a followup to #48528 to deal with some unresolved feedback and to try a prototype-swap improvement to speed up AsyncResource performance a bit. Here's the benchmark numbers:

                                                                                          confidence improvement accuracy (*)   (**)  (***)
async_hooks/async-local-storage-getstore-nested-resources.js n=500000 resourceCount=10          ***      0.65 %       ±0.20% ±0.27% ±0.34%
async_hooks/async-local-storage-getstore-nested-resources.js n=500000 resourceCount=100                  0.05 %       ±0.24% ±0.32% ±0.40%
async_hooks/async-local-storage-getstore-nested-resources.js n=500000 resourceCount=1000        ***      1.07 %       ±0.27% ±0.35% ±0.45%
async_hooks/async-local-storage-getstore-nested-run.js n=10000 storageCount=1                     *     -0.81 %       ±0.66% ±0.87% ±1.11%
async_hooks/async-local-storage-getstore-nested-run.js n=10000 storageCount=10                          -0.45 %       ±0.65% ±0.85% ±1.09%
async_hooks/async-local-storage-getstore-nested-run.js n=10000 storageCount=100                          0.46 %       ±0.67% ±0.88% ±1.13%
async_hooks/async-local-storage-propagate-asyncresource.js n=1000 storageCount=0                ***      4.35 %       ±0.68% ±0.90% ±1.14%
async_hooks/async-local-storage-propagate-asyncresource.js n=1000 storageCount=1                ***      5.27 %       ±0.58% ±0.77% ±0.98%
async_hooks/async-local-storage-propagate-asyncresource.js n=1000 storageCount=10               ***      1.28 %       ±0.63% ±0.83% ±1.06%
async_hooks/async-local-storage-propagate-asyncresource.js n=1000 storageCount=100               **      0.37 %       ±0.23% ±0.30% ±0.38%
async_hooks/async-local-storage-propagate-promise.js n=100000 storageCount=0                    ***      0.59 %       ±0.34% ±0.45% ±0.57%
async_hooks/async-local-storage-propagate-promise.js n=100000 storageCount=1                    ***      0.27 %       ±0.15% ±0.20% ±0.25%
async_hooks/async-local-storage-propagate-promise.js n=100000 storageCount=10                            0.11 %       ±0.12% ±0.16% ±0.20%
async_hooks/async-local-storage-propagate-promise.js n=100000 storageCount=100                          -0.17 %       ±0.19% ±0.26% ±0.33%
async_hooks/async-local-storage-run.js n=10000000                                                        0.29 %       ±0.39% ±0.51% ±0.66%

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

@Qard Qard added doc Issues and PRs related to the documentations. diag-agenda Issues and PRs to discuss during the meetings of the diagnostics working group. async_local_storage AsyncLocalStorage request-ci Add this label to start a Jenkins CI on a PR. labels Aug 7, 2024
@nodejs-github-bot 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 Aug 7, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2024
@nodejs-github-bot
Copy link
Collaborator

Copy link

codecov bot commented Aug 7, 2024

Codecov Report

Attention: Patch coverage is 93.02326% with 3 lines in your changes missing coverage. Please review.

Project coverage is 87.09%. Comparing base (d1229ee) to head (604d429).
Report is 514 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/async_context_frame.js 92.30% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #54239      +/-   ##
==========================================
- Coverage   87.09%   87.09%   -0.01%     
==========================================
  Files         647      647              
  Lines      181836   181778      -58     
  Branches    34917    34882      -35     
==========================================
- Hits       158376   158320      -56     
- Misses      16743    16766      +23     
+ Partials     6717     6692      -25     
Files with missing lines Coverage Δ
lib/async_hooks.js 99.65% <100.00%> (ø)
...nternal/async_local_storage/async_context_frame.js 78.72% <ø> (ø)
lib/internal/process/task_queues.js 100.00% <100.00%> (ø)
lib/internal/timers.js 99.45% <100.00%> (ø)
src/api/async_resource.cc 95.83% <100.00%> (ø)
lib/internal/async_context_frame.js 79.72% <92.30%> (-20.28%) ⬇️

... and 50 files with indirect coverage changes

@Qard Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Qard Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 8, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 8, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Qard Qard added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 9, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 9, 2024
@nodejs-github-bot nodejs-github-bot merged commit 7366808 into nodejs:main Aug 9, 2024
55 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 7366808

@Qard Qard deleted the als-rewrite-followup branch August 9, 2024 20:17
targos pushed a commit that referenced this pull request Aug 14, 2024
PR-URL: #54239
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
targos pushed a commit that referenced this pull request Aug 14, 2024
PR-URL: #54239
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Aug 19, 2024
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_local_storage AsyncLocalStorage diag-agenda Issues and PRs to discuss during the meetings of the diagnostics working group. doc Issues and PRs related to the documentations. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants