Skip to content

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

@Qard Qard force-pushed the als-rewrite-followup branch from 57238f0 to 185ec61 Compare August 7, 2024 02:40
@codecov
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 force-pushed the als-rewrite-followup branch from 185ec61 to cb6d5e6 Compare August 7, 2024 05:16
@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 force-pushed the als-rewrite-followup branch from cb6d5e6 to 604d429 Compare August 8, 2024 20:45
@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
@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
mcollina pushed a commit to mcollina/node that referenced this pull request Dec 12, 2025
PR-URL: nodejs#54239
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
mcollina pushed a commit to mcollina/node that referenced this pull request Dec 12, 2025
PR-URL: nodejs#54239
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
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