stream: use new AsyncResource instead of bind #59867

mcollina · 2025-09-12T13:42:15Z

The bind method uses ObjectDefineProperty that shows up in flamegraphs. This changes it to avoid the utility.

The bind method uses ObjectDefineProperty that shows up in flamegraphs. This changes it to avoid the utility. Signed-off-by: Matteo Collina <[email protected]>

nodejs-github-bot · 2025-09-12T13:42:20Z

Review requested:

@nodejs/streams

gurgunday

lgtm!

Signed-off-by: Matteo Collina <[email protected]>

nodejs-github-bot · 2025-09-12T14:53:23Z

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

codecov · 2025-09-12T15:35:25Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.27%. Comparing base (01bc728) to head (7ebc47b).
⚠️ Report is 33 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59867      +/-   ##
==========================================
- Coverage   88.28%   88.27%   -0.01%     
==========================================
  Files         701      702       +1     
  Lines      206749   206772      +23     
  Branches    39779    39780       +1     
==========================================
+ Hits       182520   182522       +2     
- Misses      16236    16258      +22     
+ Partials     7993     7992       -1

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

... and 56 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.

slagiewka · 2025-09-13T11:32:11Z

This is the performance improvement of this PR
Using the benchmark provided by @slagiewka in #58218 (comment). This makes things even better.

Wherever the performance increase is coming from, I don't think it's from this PR.
My benchmark just calls AsyncLocalStorage.bind() which I imagine has nothing to do with this PR making eos faster in general.

If you run this micro benchmark on main, it will show the same performance. I can confirm that main is 5x faster than v24.8.0, though.

slagiewka · 2025-09-13T12:13:16Z

This is the performance improvement of this PR
Using the benchmark provided by @slagiewka in #58218 (comment). This makes things even better.

Wherever the performance increase is coming from, I don't think it's from this PR. My benchmark just calls AsyncLocalStorage.bind() which I imagine has nothing to do with this PR making eos faster in general.

If you run this micro benchmark on main, it will show the same performance. I can confirm that main is 5x faster than v24.8.0, though.

I was able to bisect where it happened. As expected, in #58618.

The lower number is on the commit before #58618. Obviously this has the perf hit from deprecate we solved in #58218.

RafaelGSS · 2025-09-13T12:39:17Z

@slagiewka I have used 24.8.0 as a reference in comparison to this PR. Since this PR is based on main, it might be a bit unrelated, yes.

Anyway, avoiding the .bind usage here, I'm sure it's a performance improvement - I'll hide my comment as the gains aren't clear.

H4ad

Before merging this one, let's also take a look at #59873, I didn't see any benchmarks for this change so I don't think we should push yet

H4ad

Actually, we probably can combine both PRs, the other one optimizes to avoid bind when not needed, and this one can optimize when it's needed.

Using the benchmark provided in the other PR:

                                                        confidence improvement accuracy (*)   (**)  (***)
streams/compose.js n=1000                                      ***     31.28 %       ±2.16% ±2.89% ±3.78%
streams/end-of-stream.js streamType='readable' n=100000        ***     63.37 %       ±1.52% ±2.04% ±2.68%
streams/end-of-stream.js streamType='writable' n=100000        ***     84.17 %       ±1.77% ±2.37% ±3.12%
streams/pipe-object-mode.js n=5000000                                   0.56 %       ±1.13% ±1.51% ±1.97%
streams/pipe.js n=5000000                                              -0.12 %       ±1.99% ±2.65% ±3.46%

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 (***)

Dismissing the request change but I still recommends to take a look at the #59873 to see how we can combine the improvements.

We now have benchmark.

nodejs-github-bot · 2025-09-13T14:53:52Z

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

Flarna · 2025-09-15T20:03:29Z

lib/internal/streams/end-of-stream.js

@@ -54,6 +54,14 @@ function isRequest(stream) {

 const nop = () => {};

+function bindAsyncResource(fn, type) {


Should we add a bind variant (or add an option) which doesn't copy the length property to AsyncLocalStore and AsyncResource?
There are cases like express handlers where this is needed but at a lot places noone cars about lenght.

I think we might just document how to do it "fast"? It's literally 4 lines of code.

I'm ok in adding another method as well.

nodejs-github-bot · 2025-09-16T07:50:52Z

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

nodejs-github-bot · 2025-09-16T09:02:41Z

Commit Queue failed

- Loading data for nodejs/node/pull/59867
✔  Done loading data for nodejs/node/pull/59867
----------------------------------- PR info ------------------------------------
Title      stream: use new AsyncResource instead of bind (#59867)
Author     Matteo Collina <[email protected]> (@mcollina)
Branch     mcollina:remove-asyncresource-bind -> nodejs:main
Labels     stream, needs-ci, commit-queue-squash
Commits    2
 - stream: use new AsyncResource instead of bind
 - fixup
Committers 1
 - Matteo Collina <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/59867
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/59867
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 12 Sep 2025 13:42:15 GMT
   ✔  Approvals: 6
   ✔  - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/59867#pullrequestreview-3217764704
   ✔  - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/59867#pullrequestreview-3216929242
   ✔  - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/59867#pullrequestreview-3217006798
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/59867#pullrequestreview-3217839938
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/59867#pullrequestreview-3218380803
   ✔  - Stephen Belanger (@Qard): https://github.com/nodejs/node/pull/59867#pullrequestreview-3227157720
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2025-09-16T07:50:52Z: https://ci.nodejs.org/job/node-test-pull-request/69244/
- Querying data for job/node-test-pull-request/69244/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/17760525866

nodejs-github-bot · 2025-09-16T15:15:50Z

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

nodejs-github-bot · 2025-09-16T16:43:48Z

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

stream: use new AsyncResource instead of bind

ed9e709

The bind method uses ObjectDefineProperty that shows up in flamegraphs. This changes it to avoid the utility. Signed-off-by: Matteo Collina <[email protected]>

mcollina requested a review from gurgunday September 12, 2025 13:42

nodejs-github-bot added needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem. labels Sep 12, 2025

mcollina requested review from ronag and RafaelGSS September 12, 2025 13:42

gurgunday approved these changes Sep 12, 2025

View reviewed changes

ronag approved these changes Sep 12, 2025

View reviewed changes

fixup

7ebc47b

Signed-off-by: Matteo Collina <[email protected]>

BridgeAR approved these changes Sep 12, 2025

View reviewed changes

RafaelGSS approved these changes Sep 12, 2025

View reviewed changes

mcollina added request-ci Add this label to start a Jenkins CI on a PR. commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 12, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 12, 2025

ronag approved these changes Sep 12, 2025

View reviewed changes

jasnell approved these changes Sep 12, 2025

View reviewed changes

This comment was marked as resolved.

Sign in to view

lpinca approved these changes Sep 12, 2025

View reviewed changes

H4ad previously requested changes Sep 13, 2025

View reviewed changes

H4ad reviewed Sep 13, 2025

View reviewed changes

Flarna reviewed Sep 15, 2025

View reviewed changes

avcribl mentioned this pull request Sep 15, 2025

stream: improve performance of finished() #59873

Open

Qard approved these changes Sep 16, 2025

View reviewed changes

mcollina added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Sep 16, 2025

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 16, 2025

		@@ -54,6 +54,14 @@ function isRequest(stream) {

		const nop = () => {};

		function bindAsyncResource(fn, type) {

Uh oh!

stream: use new AsyncResource instead of bind #59867

Are you sure you want to change the base?

stream: use new AsyncResource instead of bind #59867

Conversation

mcollina commented Sep 12, 2025

Uh oh!

nodejs-github-bot commented Sep 12, 2025

Uh oh!

gurgunday left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Sep 12, 2025

Uh oh!

codecov bot commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment was marked as resolved.

slagiewka commented Sep 13, 2025

Uh oh!

slagiewka commented Sep 13, 2025

Uh oh!

RafaelGSS commented Sep 13, 2025

Uh oh!

H4ad left a comment

Choose a reason for hiding this comment

Uh oh!

H4ad left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Sep 13, 2025

Uh oh!

Flarna Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

mcollina Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Sep 16, 2025

Uh oh!

nodejs-github-bot commented Sep 16, 2025

Uh oh!

nodejs-github-bot commented Sep 16, 2025

Uh oh!

nodejs-github-bot commented Sep 16, 2025

Uh oh!

Uh oh!

codecov bot commented Sep 12, 2025 •

edited

Loading