stream: cut promise churn in webstreams hot paths - #65138
Conversation
|
Review requested:
|
jasnell
left a comment
jasnell
left a comment
There was a problem hiding this comment.
Couple of nits, otherwise LGTM
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #65138 +/- ##
==========================================
+ Coverage 90.30% 90.34% +0.03%
==========================================
Files 751 751
Lines 250235 250330 +95
Branches 47305 47321 +16
==========================================
+ Hits 225987 226171 +184
+ Misses 15619 15557 -62
+ Partials 8629 8602 -27
🚀 New features to boost your workflow:
|
|
As long as WPT passes, I'm good if we can avoid a promise allocation. That specifically seems non-observable behavior. |
github-actions
Bot
removed
the
request-ci
|
Benchmark GHA (webstreams / pipe-to): https://github.com/nodejs/node/actions/runs/31826854651 Results
Benchmark results:
|
github-actions
Bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
aduh95
added
author ready
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/65138 ✔ Done loading data for nodejs/node/pull/65138 ----------------------------------- PR info ------------------------------------ Title stream: cut promise churn in webstreams hot paths (#65138) Author Matteo Collina <matteo.collina@gmail.com> (@mcollina) Branch mcollina:webstream-perf-round12 -> nodejs:main Labels author ready, needs-ci, commit-queue, web streams Commits 4 - benchmark: apply highWaterMark in webstreams pipe-to - stream: cut promise churn in webstreams hot paths - stream: consolidate non-op algorithm callbacks - Update lib/internal/webstreams/readablestream.js Committers 2 - Matteo Collina <hello@matteocollina.com> - GitHub <noreply@github.com> PR-URL: https://github.com/nodejs/node/pull/65138 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/65138 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 08 Aug 2026 15:01:49 GMT ✔ Approvals: 4 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/65138#pullrequestreview-4889126958 ✔ - Gürgün Dayıoğlu (@gurgunday): https://github.com/nodejs/node/pull/65138#pullrequestreview-4941618229 ✔ - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/65138#pullrequestreview-4937946571 ✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/65138#pullrequestreview-4943235120 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2026-08-15T04:53:41Z: https://ci.nodejs.org/job/node-test-pull-request/75855/ - Querying data for job/node-test-pull-request/75855/ ✔ Build data downloaded ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 65138 From https://github.com/nodejs/node * branch refs/pull/65138/merge -> FETCH_HEAD ✔ Fetched commits as 9e23066b8af4..0c0d5beaa88d -------------------------------------------------------------------------------- [main eaaf996401] benchmark: apply highWaterMark in webstreams pipe-to Author: Matteo Collina <hello@matteocollina.com> Date: Sat Aug 8 17:00:33 2026 +0200 1 file changed, 4 insertions(+), 6 deletions(-) Auto-merging lib/internal/webstreams/readablestream.js [main 1d82311a65] stream: cut promise churn in webstreams hot paths Author: Matteo Collina <hello@matteocollina.com> Date: Sat Aug 8 17:00:48 2026 +0200 3 files changed, 126 insertions(+), 25 deletions(-) Auto-merging lib/internal/webstreams/readablestream.js [main 6bb61acbad] stream: consolidate non-op algorithm callbacks Author: Matteo Collina <hello@matteocollina.com> Date: Sat Aug 8 19:32:35 2026 +0200 3 files changed, 19 insertions(+), 25 deletions(-) Auto-merging lib/internal/webstreams/readablestream.js [main e67bb74dca] Update lib/internal/webstreams/readablestream.js Author: Matteo Collina <matteo.collina@gmail.com> Date: Fri Aug 14 10:03:20 2026 -0400 1 file changed, 1 insertion(+), 1 deletion(-) ✔ Patches applied There are 4 commits in the PR. Attempting autorebase. (node:400) [DEP0190] DeprecationWarning: Passing args to a child process with shell option true can lead to security vulnerabilities, as the arguments are not escaped, only concatenated. (Use `node --trace-deprecation ...` to show where the warning was created) Rebasing (2/8) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- benchmark: apply highWaterMark in webstreams pipe-tohttps://github.com/nodejs/node/actions/runs/31872533736 |
aduh95
added
commit-queue
The highWaterMark values were passed as properties of the underlying source and sink dictionaries, where they are ignored: a queuing strategy's highWaterMark is read from the constructors' second argument. Every configuration therefore measured the identical workload at the default highWaterMark of 1, which also explains the historically high run-to-run variance of this benchmark family. Pass the strategies as the constructors' second argument and cover the default (1) alongside buffered (1024, 4096) configurations. Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: nodejs#65138 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Three related reductions on the per-chunk paths: Wrap user sink.write and source.pull callbacks without coercing their result into a promise. When the callback returns a non-thenable (the common synchronous case), fulfillment is guaranteed and no then() lookup is observable, so the fulfilled reaction is enqueued through a single shared resolved promise at the exact microtask position the coerced promise's reaction would have had, skipping the implicit async-wrapper promise per chunk. Thenable results go through PromiseResolve(), which matches the spec's "a promise resolved with" conversion (identity for native promises). Park pipeTo's pump on backpressure by installing a record that duck-types the writer's lazily-materialized [[readyPromise]] record and whose resolve function is the pump continuation itself. Backpressure clearing then resumes the pump directly instead of materializing a fresh promise record plus reaction per flip, and the pump no longer schedules a microtask per batch. writableStreamUpdateBackpressure publishes the new backpressure state before resolving the ready record so the pump observes the updated value. Replace queueMicrotask() on the pipeTo and tee chunk-forwarding paths with a reaction on the shared resolved promise, which enqueues the continuation at the same position without the per-call scheduling overhead. pipe-to improves by 8-14% across all benchmark configurations, with readable-read and tee also improving in spot runs. Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: nodejs#65138 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
The start, pull, and write non-op algorithms are all raw callbacks with an identical empty body now, so a single shared nonOpCallback replaces nonOpStart, nonOpPull, and nonOpWrite. Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: nodejs#65138 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
0c0d5be to
2929417
Compare
|
Landed in 4551732...2929417 |
7 participants
Twelfth round of pure-JS webstreams optimizations, following #64890.
Profiling
pipeToat the defaulthighWaterMarkshowed roughly 10% of the profile inqueueMicrotaskplus its native binding (one call per pump batch), a freshwriter.readypromise record plus reaction per backpressure flip on thepipeThroughshape, and an implicit async-wrapper promise persink.write/source.pullinvocation. Three changes, one commit:sink.writeandsource.pullcallbacks are wrapped without coercing their result into a promise. A non-thenable result (the common synchronous case) means fulfillment is guaranteed and nothen()lookup is observable, so the fulfilled reaction is enqueued through a single shared resolved promise at the exact microtask position the coerced promise's reaction would have had. Thenable results go throughPromiseResolve(), matching the reference implementation'spromiseCall(identity for native promises).[[readyPromise]]record;writableStreamUpdateBackpressureresolving it re-enters the pump directly. This removes the per-flip promise record + reaction and the per-batchqueueMicrotask. The backpressure state field is now published before the ready record is resolved so the hook observes the new value (the resolve of a real ready record only settles a promise, so the reorder is unobservable otherwise).forwardChunkhops and the pump's between-batch yield use a reaction on the shared resolved promise instead ofqueueMicrotask, which enqueues at the same position with less overhead.A separate first commit fixes the
pipe-to.jsbenchmark: thehighWaterMarkvalues were passed inside the underlying source/sink dictionaries where they are ignored, so all 16 configurations measured the identical workload at the defaulthighWaterMarkof 1. The strategies are now passed as the constructors' second argument, with the matrix covering the default (1) and buffered (1024, 4096) configurations.Benchmark results with the fixed benchmark (30 runs):
The full-suite run showed no regressions in any other family;
readable-read normal,tee normal, andpipeThroughpassthrough spot runs also improve (~+6-18%). Verified with the WPT streams/compression/encoding suites, the full parallel webstream/whatwg test set, and a shutdown-ordering stress (abort mid-write, close with pending writes, sync-throwing and rejecting sinks, error propagation) whose event log is byte-identical tomain.