stream: improve performance of finished()

[avcribl]

The changes optimize the end-of-stream.js module to only preserve AsyncLocalStorage context when it's actually needed, improving performance by avoiding ALS overhead in the common case where no async context tracking is active.
The performance test results with this patch:

./out/Release/node ./benchmark/streams/end-of-stream.js streams/end-of-stream.js streamType="readable" n=100000: 3,290,307.950450068 streams/end-of-stream.js streamType="writable" n=100000: 1,004,954.4253168119 streams/end-of-stream.js streamType="duplex" n=100000: 893,658.706842607 

The same perf test results with the current nodejs:

node ./benchmark/streams/end-of-stream.js streams/end-of-stream.js streamType="readable" n=100000: 385,512.31905964843 streams/end-of-stream.js streamType="writable" n=100000: 348,685.1671624238 streams/end-of-stream.js streamType="duplex" n=100000: 382,477.79857896036 

You can compare and see the noticeable performance improvement with this patch.

[nodejs-github-bot]

Review requested:

• @nodejs/performance
• @nodejs/streams

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.57%. Comparing base (657428a) to head (d51ca0d).
⚠️ Report is 90 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #59873 +/- ## ========================================== + Coverage 88.56% 88.57% +0.01%  ========================================== Files 704 704 Lines 208322 208330 +8 Branches 40027 40040 +13 ========================================== + Hits 184501 184533 +32 + Misses 15863 15818 -45 - Partials 7958 7979 +21 

Files with missing lines	Coverage Δ
lib/internal/streams/end-of-stream.js	97.41% <100.00%> (+0.06%)	⬆️

... and 42 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[H4ad]

Hey, thanks for the PR, I added a few comments about some issues I found running locally.

Also, would you mind split this change in two commits, the first one being the benchmark and the second one the optimization?

You can also improve the commit message a little bit by adding what you really changed to improve performance, eg: stream: preserve AsyncLocalStorage on finished only when needed

About the benchmark name, maybe changing to finished should be more relevant than end-of-stream since we are testing the finished function.

Preview of the results:

h4ad:node-copy-4/ (improve-stream-finished-perf✗) $ node-benchmark-compare streams.csv [10:49:03] confidence improvement accuracy (*) (**) (***) streams/compose.js n=1000 *** 42.25 % ±3.90% ±5.24% ±6.91% streams/end-of-stream.js streamType='readable' n=100000 *** 76.67 % ±1.41% ±1.89% ±2.48% streams/end-of-stream.js streamType='writable' n=100000 *** 105.00 % ±2.01% ±2.69% ±3.56% streams/pipe-object-mode.js n=5000000 -0.30 % ±0.81% ±1.08% ±1.42% streams/pipe.js n=5000000 0.14 % ±1.77% ±2.36% ±3.08% Be aware that when doing many comparisons the risk of a false-positive result increases. In this case, there are 5 comparisons, you can thus expect the following amount of false-positive results: 0.25 false positives, when considering a 5% risk acceptance (*, **, ***), 0.05 false positives, when considering a 1% risk acceptance (**, ***), 0.01 false positives, when considering a 0.1% risk acceptance (***) 

[mcollina]

While I love the PR, but I fear that this breaks continuation when using async_hooks.

We could argue that with ALS not using async_hooks we could remove them (or add them via a compile time flag) but that's outside of the scope of this PR.

[avcribl]

While I love the PR, but I fear that this breaks continuation when using async_hooks.

We could argue that with ALS not using async_hooks we could remove them (or add them via a compile time flag) but that's outside of the scope of this PR.

Thanks @mcollina! I removed the async_hooks part.

[krsjenswbp]

thanks

[Flarna]

There is command line option --no-async-context-frame to disable use of AsyncContextFrame in ALS.

I think we might need special handing for this variation.

[mcollina]

@avcribl you didn't address my comment.

[avcribl]

@avcribl you didn't address my comment.

@mcollina Are you suggesting not to use AsyncLocalStorage ??= require('async_hooks').AsyncLocalStorage at all? I see you have an open PR here #59867
Would you please provide more hints as to what your suggestion is here? (This is my first contribution)

[avcribl]

@avcribl you didn't address my comment.

@mcollina I've pushed a new commit that adds a check to determine whether async_context_frame is enabled. I tested both scenarios:
when async_context_frame is enabled, and when it is disabled (falling back to async_hooks).

In both cases, the unit tests ran successfully and passed:

./out/Release/node --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /Users/amir/Documents/projects4/node/test/async-hooks/test-async-local-storage-stream-finished.js ./out/Release/node --no-async-context-frame --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /Users/amir/Documents/projects4/node/test/async-hooks/test-async-local-storage-stream-finished.js 

Also, I ran the benchmark test with and without the flag and both have similar improved performance:

./out/Release/node ./benchmark/streams/finished.js streams/finished.js streamType="readable" n=100000: 1,033,352.7517537934 streams/finished.js streamType="writable" n=100000: 1,383,377.7808003046 ./out/Release/node --no-async-context-frame ./benchmark/streams/finished.js streams/finished.js streamType="readable" n=100000: 1,044,042.0363062695 streams/finished.js streamType="writable" n=100000: 1,362,108.6226052495 

Please let me know if it addresses your comment.

[mcollina]

I don't see any new tests in this PR, just a benchmark.

@Qard or @Flarna could chime in as well, but checking if AsyncContextFrame is enabled is not enough in case plain async_hooks are used. There are still a lot of modules out there that are not using AsyncLocalStorage for context tracking. My understanding is that using async_hooks.createHook() does not enable this block, breaking continuation.

[Flarna]

It's quite hard to tell. I would recommend to write tests for all these scenarios to be improved here and also verify that non improvable scenarios still work.

FWIW I'm not a big fan of tinkering with async hooks/async local store internals at several places in code.
Maybe using enabledHooksExist instead getHookArrays()[0].length would make it easier to understand.

[Qard]

This could be simplified to:

Suggested change

	if((AsyncContextFrame.enabled&&AsyncContextFrame.current())||
	(!AsyncContextFrame.enabled&&getHookArrays()[0].length>0)){
	if(AsyncContextFrame.current()||enabledHooksExist()){

The AsyncContextFrame.current() call is a no-op when inactive, you don't really need the enabled check as that will just add more instructions. Also, as @Flarna pointed out, enabledHooksExist() is the clearer way to check if there are active async_hooks listeners.

[avcribl]

Thanks! I removed AsyncContextFrame.enabled

[Qard]

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.

[avcribl]

Thanks all for the feedback!
Following the suggestion to use enabledHooksExist(), I spent quite some time on it and gave that a try—but found that it consistently returned true, even when no async hooks were active. So, I reverted to using getHookArrays()[0].length > 0, as it provides a more accurate reflection of the current hook state.
I've also added additional unit tests to cover both code paths and validate the conditions across different scenarios.

[mcollina]

lgtm

[mcollina]

Note that this does not verify continuation is preserved

[mcollina]

Note that this does not verify continuation is preserved

[Flarna]

This doesn't verify the code path either. It is a copy of the condition and someone might change one of the locations.

[avcribl]

Thanks! I expanded on the test!

[avcribl]

@mcollina I addressed your comment, thanks!

[avcribl]

I rebased my PR. Can I please get approval for kicking off the workflows? Thanks!

[nodejs-github-bot]

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

[avcribl]

Can the pipeline be triggered again? Still, some flaky tests fail, thank you!

[nodejs-github-bot]

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

[avcribl]

Can I get another run please? Thanks!

[nodejs-github-bot]

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

[avcribl]

CI is green now!

[avcribl]

The CI build is green, do I need to do anything more here? Thanks!

[avcribl]

Thanks @H4ad ! I think the label commit-queue needs to be added as well.

[nodejs-github-bot]

Landed in 4fe325d