nodejs
process: preserve AsyncLocalStorage on queueMicrotask only when needed
Does the same optimization as https://github.com/nodejs/node/pull/59873 for queueMicrotask
branch:
./node benchmark/run.js --filter queue-microtask process
process/queue-microtask-breadth.js
process/queue-microtask-breadth.js n=400000: 18,938,347.488090977
process/queue-microtask-depth.js
process/queue-microtask-depth.js n=1200000: 35,862,646.06556887
main
./node benchmark/run.js --filter queue-microtask process
process/queue-microtask-breadth.js
process/queue-microtask-breadth.js n=400000: 5,198,220.087856157
process/queue-microtask-depth.js
process/queue-microtask-depth.js n=1200000: 12,707,696.847598467
Codecov Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 88.58%. Comparing base (6274eb7) to head (2f8776e).
:warning: Report is 80 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #60913 +/- ##
==========================================
+ Coverage 88.55% 88.58% +0.02%
==========================================
Files 703 703
Lines 208291 208296 +5
Branches 40170 40166 -4
==========================================
+ Hits 184443 184509 +66
+ Misses 15874 15807 -67
- Partials 7974 7980 +6
| Files with missing lines | Coverage Δ | |
|---|---|---|
| lib/internal/process/task_queues.js | 100.00% <100.00%> (ø) |
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
Before merging, some concerns raised in another PR with same changes:
One thing worth noting is that this would be a breaking change as currently the AsyncLocalStorage.bind(...) will always bind, even if there are no hooks or stores in use, but async_hooks could start listening at any time. In the current model you could start listening after the bind call there but before the callback is called and you would see the events. After this change you would not. By @Qard
Would mind giving some thoughts on this one, @mcollina and @Qard?
Edit: Benchmark: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1770/
Failed to start CI
⚠ No approving reviews found ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/19980375054
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
@H4ad Yes, the same concern applies. It is, in my opinion, problematic that it was ever possible to start seeing events for a given task partway through its lifecycle rather than deciding at emit time if any of the events should be present. But that is certainly a breaking change and so should be marked as such. A desirable change, I think. But a breaking one nonetheless.
At the same time though, async_hooks is still marked as experimental, and AsyncLocalStorage no longer relies on it, so the break is likely a lot less critical. We may be able to allow it without a major bump. I'm not strongly opinionated either way.
Benchmark result:
confidence improvement accuracy (*) (**) (***)
process/bench-env.js operation='delete' n=1000000 0.89 % ±5.64% ±7.51% ±9.77%
process/bench-env.js operation='enumerate' n=1000000 1.65 % ±3.36% ±4.47% ±5.82%
process/bench-env.js operation='get' n=1000000 -0.79 % ±1.68% ±2.24% ±2.91%
process/bench-env.js operation='query' n=1000000 1.36 % ±2.64% ±3.51% ±4.57%
process/bench-env.js operation='set' n=1000000 0.90 % ±3.45% ±4.59% ±5.98%
process/bench-hrtime.js type='bigint' n=1000000 -1.11 % ±4.98% ±6.62% ±8.62%
process/bench-hrtime.js type='diff' n=1000000 * -3.27 % ±3.21% ±4.27% ±5.57%
process/bench-hrtime.js type='raw' n=1000000 -0.25 % ±3.02% ±4.02% ±5.24%
process/coverage.js n=100000 0.03 % ±1.10% ±1.47% ±1.92%
process/getActiveResourcesInfo.js n=100000 immediatesCount=10000 timeoutsCount=10000 requestsCount=10000 handlesCount=10000 -1.21 % ±5.47% ±7.28% ±9.48%
process/memoryUsage.js n=100000 0.18 % ±0.41% ±0.55% ±0.71%
process/next-tick-breadth-args.js n=10000000 0.05 % ±0.35% ±0.46% ±0.60%
process/next-tick-breadth.js n=10000000 0.01 % ±0.75% ±1.00% ±1.30%
process/next-tick-depth-args.js n=7000000 -0.79 % ±1.24% ±1.65% ±2.14%
process/next-tick-depth.js n=7000000 0.48 % ±1.89% ±2.51% ±3.27%
process/next-tick-exec-args.js n=4000000 -0.02 % ±8.70% ±11.57% ±15.06%
process/next-tick-exec.js n=4000000 -0.03 % ±0.17% ±0.23% ±0.30%
process/next-tick-loop-args.js loop=100 n=10000 -0.84 % ±2.36% ±3.14% ±4.08%
process/next-tick-loop-args.js loop=100 n=20000 -0.43 % ±0.87% ±1.16% ±1.51%
process/next-tick-loop-args.js loop=100 n=40000 0.35 % ±0.58% ±0.78% ±1.01%
process/next-tick-loop-args.js loop=200 n=10000 0.29 % ±1.80% ±2.40% ±3.12%
process/next-tick-loop-args.js loop=200 n=20000 0.29 % ±0.80% ±1.07% ±1.39%
process/next-tick-loop-args.js loop=200 n=40000 -0.50 % ±0.64% ±0.85% ±1.10%
process/next-tick-loop.js loop=100 n=10000 -0.73 % ±1.50% ±2.00% ±2.61%
process/next-tick-loop.js loop=100 n=20000 -0.02 % ±1.85% ±2.46% ±3.21%
process/next-tick-loop.js loop=100 n=40000 -0.24 % ±1.29% ±1.72% ±2.24%
process/next-tick-loop.js loop=200 n=10000 -0.41 % ±1.30% ±1.73% ±2.26%
process/next-tick-loop.js loop=200 n=20000 -0.27 % ±1.40% ±1.87% ±2.43%
process/next-tick-loop.js loop=200 n=40000 0.19 % ±1.12% ±1.49% ±1.94%
process/queue-microtask-breadth.js n=400000 *** 221.40 % ±5.59% ±7.53% ±9.98%
process/queue-microtask-depth.js n=1200000 *** 137.38 % ±5.80% ±7.81% ±10.33%
process/resourceUsage.js n=100000 -0.67 % ±2.84% ±3.77% ±4.91%
Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 32 comparisons, you can thus
expect the following amount of false-positive results:
1.60 false positives, when considering a 5% risk acceptance (*, **, ***),
0.32 false positives, when considering a 1% risk acceptance (**, ***),
0.03 false positives, when considering a 0.1% risk acceptance (***)
Impressive 🚀
At the same time though, async_hooks is still marked as experimental, and AsyncLocalStorage no longer relies on it, so the break is likely a lot less critical. We may be able to allow it without a major bump. I'm not strongly opinionated either way.
The linked PR https://github.com/nodejs/node/pull/59873 was marked semver major.
