Skip to content

stream: fast path single-destination pipe#63592

Open
trivikr wants to merge 2 commits into
nodejs:mainfrom
trivikr:stream-pipe-hot-path-listener-overhead
Open

stream: fast path single-destination pipe#63592
trivikr wants to merge 2 commits into
nodejs:mainfrom
trivikr:stream-pipe-hot-path-listener-overhead

Conversation

@trivikr
Copy link
Copy Markdown
Member

@trivikr trivikr commented May 27, 2026

Avoid EventEmitter dispatch for the internal readable.pipe() data handler
when it is the only data listener on the source stream.

If any additional data listener is present, the stream falls back to normal
data event emission so listener ordering is preserved.

Benchmark:

                          confidence improvement accuracy (*)   (**)  (***)
streams/pipe.js n=5000000        ***     11.96 %       +/-2.00% +/-2.66% +/-3.47%

Assisted-by: openai:gpt-5.5

Avoid EventEmitter dispatch for the readable pipe data handler when it
is the only data listener. Fall back to normal data event emission when
additional data listeners are present to preserve listener ordering.

Signed-off-by: Kamat, Trivikram <[email protected]>
Assisted-by: openai:gpt-5.5
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem. labels May 27, 2026
Copy link
Copy Markdown
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks to me like it will break stuff. What happens if a data listener is added after pipe? Can you add a test for that.

@trivikr
Copy link
Copy Markdown
Member Author

trivikr commented May 27, 2026

What happens if a data listener is added after pipe?

If a data listener is added after pipe(), the fast path stops applying because _events.data is no longer the single internal pipe handler. Subsequent chunks go through normal stream.emit('data'), preserving EventEmitter listener order.

Can you add a test for that?

Added a test in 3184889 (this PR)

The added test starts with r.pipe(w), then adds a data listener during the first destination write(). It asserts the listener does not see the in-flight first chunk, but does see the second chunk after the pipe write, matching normal EventEmitter behavior.

@trivikr trivikr requested a review from ronag May 27, 2026 05:55
@trivikr trivikr force-pushed the stream-pipe-hot-path-listener-overhead branch from 2d523a1 to 3184889 Compare May 27, 2026 06:15
@ronag ronag requested a review from mcollina May 27, 2026 06:34
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label May 27, 2026
Copy link
Copy Markdown
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might still break stuff. Some streams override emit. @mcollina wdyt?

@codecov
Copy link
Copy Markdown

codecov Bot commented May 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.32%. Comparing base (13feb34) to head (3184889).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #63592   +/-   ##
=======================================
  Coverage   90.32%   90.32%           
=======================================
  Files         730      730           
  Lines      234653   234666   +13     
  Branches    43942    43943    +1     
=======================================
+ Hits       211959   211973   +14     
+ Misses      14415    14411    -4     
- Partials     8279     8282    +3     
Files with missing lines Coverage Δ
lib/internal/streams/readable.js 97.25% <100.00%> (+0.01%) ⬆️

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

@trivikr
Copy link
Copy Markdown
Member Author

trivikr commented May 27, 2026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. request-ci Add this label to start a Jenkins CI on a PR. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants