stream: drop noop .then in pipe orchestration#63578
Closed
mcollina wants to merge 1 commit into
Closed
Conversation
Apply the same markPromiseAsHandled-instead-of-setPromiseHandled substitution as the per-chunk path to the four pipe orchestration call sites that fire once per pipe setup rather than per chunk: - readablestream.js: pipeThrough's inner readableStreamPipeTo promise - readablestream.js: pipeTo's run() loop promise - transfer.js: cross-realm readable pipe promise - transfer.js: cross-realm writable pipe promise In each case the chain Promise + noop closure that setPromiseHandled allocates has no observer beyond V8's unhandled-rejection tracker: errors flow back via the relevant stream's closed/errored state path, which the surrounding code already monitors. markPromiseAsHandled flips the same V8 flag without the allocation. No measurable benchmark impact (these sites fire once per pipe, not per chunk), but the change is consistent with the per-chunk fix and removes four unnecessary Promise + closure allocations from each pipe setup. WPT streams parity preserved: 1403 subtests passing, 0 unexpected failures.
Member
Author
|
Closing — no measurable perf benefit. These four sites fire once per pipe setup, not per chunk, so the consistency cleanup doesn't justify reviewer burden. The per-chunk equivalent change ships in #63572. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #63578 +/- ##
==========================================
+ Coverage 90.32% 90.34% +0.01%
==========================================
Files 730 730
Lines 234209 234483 +274
Branches 43934 43908 -26
==========================================
+ Hits 211558 211837 +279
+ Misses 14372 14371 -1
+ Partials 8279 8275 -4
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the per-chunk
markPromiseAsHandledsubstitution. Apply the same pattern to the four remaining pipe-orchestrationsetPromiseHandledcall sites that fire once per pipe setup (rather than per chunk):readablestream.js:pipeThrough's innerreadableStreamPipeTopromisereadablestream.js: pipeTo'srun()loop promisetransfer.js: cross-realm readable + writable pipe promisesIn each case the chain Promise + noop closure that
setPromiseHandledallocates has no observer beyond V8's unhandled-rejection tracker — errors flow back via the relevant stream's closed/errored state path, which surrounding code already monitors.markPromiseAsHandledflips the same V8 flag with no allocation.These sites are not on a per-chunk hot path, so no benchmark impact is expected. The change is purely consistency cleanup matching the per-chunk fix.
WPT streams parity preserved: 1403 subtests passing, 0 unexpected failures.
Other
setPromiseHandledcall sites inwritablestream.js(Ensure{Ready,Closed}PromiseRejected,setupWritableStreamDefaultWriter,RejectCloseAndClosedPromiseIfNeeded) andreadablestream.js(readableStreamError,readableStreamReaderGenericRelease) were considered but not converted: probing one of them (Ensure{Ready,Closed}PromiseRejectedon the writer-release path) reproduces an unhandled-rejection regression onwritable-streams/close.any.jsand the piping error-propagation tests —MarkAsHandledand.then(undefined, () => {})are not equivalent in that specific V8 path. Those sites stay onsetPromiseHandled.