stream: avoid unnecessary drain for sync stream#50014

ronag · 2023-10-02T12:26:49Z

We have tried this before but reverted due to:

2868f52
#35926

However, it has quite a significant performance improvement and doesn't actually break the "streams spec". Let's have another look at it.

streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='no' n=100000 *** 7.14 % ±2.24% ±2.98% ±3.90% streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='yes' n=100000 ** 4.12 % ±2.47% ±3.29% ±4.28% streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='no' n=100000 *** 6.89 % ±2.21% ±2.94% ±3.84% streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='yes' n=100000 *** 10.73 % ±2.25% ±3.00% ±3.90% streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='no' n=100000 *** 8.46 % ±2.40% ±3.21% ±4.23% streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='yes' n=100000 ** 3.00 % ±1.86% ±2.49% ±3.27% streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='no' n=100000 *** 7.99 % ±4.00% ±5.34% ±6.99% streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='yes' n=100000 *** 9.51 % ±5.19% ±6.97% ±9.20% streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='no' n=100000 *** 7.11 % ±2.05% ±2.73% ±3.55% streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='yes' n=100000 *** 233.76 % ±6.96% ±9.35% ±12.37% streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='no' n=100000 *** 6.40 % ±1.06% ±1.42% ±1.85% streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='yes' n=100000 *** 215.94 % ±4.96% ±6.66% ±8.79% streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='no' n=100000 *** 6.51 % ±1.51% ±2.01% ±2.61% streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='yes' n=100000 *** 213.41 % ±3.75% ±5.01% ±6.57% streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='no' n=100000 *** 5.80 % ±1.54% ±2.06% ±2.69% streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='yes' n=100000 *** 194.75 % ±4.29% ±5.77% ±7.62%

nodejs-github-bot · 2023-10-02T12:26:54Z

Review requested:

@nodejs/streams

PR-URL: nodejs#50014

nodejs-github-bot · 2023-10-03T07:45:35Z

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

PR-URL: nodejs#50014

nodejs-github-bot · 2023-10-03T10:34:12Z

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

nodejs-github-bot · 2023-10-03T13:15:45Z

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

ronag · 2023-10-03T13:15:45Z

@mcollina PTAL

mcollina

lgtm, clever

benjamingr

I'm surprised Matteo is unconcerned with this :D

lpinca · 2023-10-03T17:02:49Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3302/ (queued).

nodejs-github-bot · 2023-10-04T12:45:42Z

Landed in 557044a

Refs: nodejs#50014

Refs: #50014 PR-URL: #50446 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

PR-URL: nodejs#50014 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Refs: nodejs#50014 PR-URL: nodejs#50446 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

PR-URL: #50014 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Refs: #50014 PR-URL: #50446 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

PR-URL: nodejs#50014 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

ronag added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 2, 2023

ronag requested review from benjamingr and mcollina October 2, 2023 12:26

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 2, 2023

ronag requested a review from lpinca October 2, 2023 12:27

ronag added the needs-citgm PRs that need a CITGM CI run. label Oct 2, 2023

This comment was marked as outdated.
Sign in to view

ronag force-pushed the drain-sync branch 6 times, most recently from 926e815 to e384e57Compare October 2, 2023 12:44

ronag removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 2, 2023

ronag added a commit to nxtedition/node that referenced this pull request Oct 2, 2023
stream: avoid unnecessary drain for sync stream
a4aa161
PR-URL: nodejs#50014

ronag force-pushed the drain-sync branch from e384e57 to a4aa161Compare October 2, 2023 12:45

ronag added a commit to nxtedition/node that referenced this pull request Oct 2, 2023
stream: avoid unnecessary drain for sync stream
35087cb
PR-URL: nodejs#50014

ronag force-pushed the drain-sync branch from a4aa161 to 35087cbCompare October 2, 2023 12:45

ronag added a commit to nxtedition/node that referenced this pull request Oct 2, 2023
stream: avoid unnecessary drain for sync stream
2b8fe13
PR-URL: nodejs#50014

ronag force-pushed the drain-sync branch from 35087cb to 2b8fe13Compare October 2, 2023 12:45

ronag added a commit to nxtedition/node that referenced this pull request Oct 2, 2023
stream: avoid unnecessary drain for sync stream
f7e2c54
PR-URL: nodejs#50014

ronag force-pushed the drain-sync branch from 2b8fe13 to f7e2c54Compare October 2, 2023 12:48

ronag added a commit to nxtedition/node that referenced this pull request Oct 2, 2023
stream: avoid unnecessary drain for sync stream
e39b0c0
PR-URL: nodejs#50014

ronag force-pushed the drain-sync branch from f7e2c54 to e39b0c0Compare October 2, 2023 12:49

ronag added a commit to nxtedition/node that referenced this pull request Oct 2, 2023
stream: avoid unnecessary drain for sync stream
bdfe338
PR-URL: nodejs#50014

ronag force-pushed the drain-sync branch from e39b0c0 to bdfe338Compare October 2, 2023 12:52

ronag added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-citgm PRs that need a CITGM CI run. labels Oct 2, 2023

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 2, 2023

stream: avoid unnecessary drain for sync stream
4844d30
PR-URL: nodejs#50014

ronag force-pushed the drain-sync branch from 980032d to 4844d30Compare October 3, 2023 07:51

mcollina approved these changes Oct 3, 2023
View reviewed changes

ronag added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 3, 2023

benjamingr approved these changes Oct 3, 2023
View reviewed changes

github-actionsbot mentioned this pull request Oct 4, 2023
CI Reliability 2023-10-04 nodejs/reliability#683
Open
14 tasks

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 4, 2023

nodejs-github-bot merged commit 557044a into nodejs:mainOct 4, 2023

github-actionsbot mentioned this pull request Oct 5, 2023
CI Reliability 2023-10-05 nodejs/reliability#684
Open
13 tasks

RafaelGSS mentioned this pull request Oct 9, 2023
2023-10-17, Version 21.0.0 (Current) #49870
Merged

ronag added a commit to nxtedition/node that referenced this pull request Oct 28, 2023
stream: remove no longer relevant comment
49d6dc9
Refs: nodejs#50014

alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
stream: avoid unnecessary drain for sync stream
82ec33c
PR-URL: nodejs#50014 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

targos pushed a commit that referenced this pull request Nov 11, 2023
stream: avoid unnecessary drain for sync stream
88c739b
PR-URL: #50014 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

targos mentioned this pull request Nov 12, 2023
v20.10.0 release proposal #50682
Merged

juanarbol mentioned this pull request Dec 6, 2023
2023-12-06, N|Solid v5.0.1 Iron Release nodesource/nsolid#41
Merged

dy-dx mentioned this pull request Feb 29, 2024
piping to a stream with highWaterMark: 0 and objectMode: false is broken since node v20.10.0 #51930
Closed

poolik mentioned this pull request Mar 27, 2024
Significant memory usage increase starting from 20.10.0 for a simple HTTP server (potentially stream related) #52228
Closed

debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
stream: avoid unnecessary drain for sync stream
9edaaf7
PR-URL: nodejs#50014 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

orgads mentioned this pull request May 28, 2024
[REG 20.9->20.10] stream: Pipe is stopped after error on another pipe from the same source #53185
Open

anonrig mentioned this pull request Oct 17, 2025
update node.js streams implementation cloudflare/workerd#5298
Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

stream: avoid unnecessary drain for sync stream#50014

stream: avoid unnecessary drain for sync stream #50014

ronag commented Oct 2, 2023•
edited
Loading

Uh oh!

nodejs-github-bot commented Oct 2, 2023

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Oct 3, 2023

Uh oh!

nodejs-github-bot commented Oct 3, 2023

Uh oh!

nodejs-github-bot commented Oct 3, 2023

Uh oh!

ronag commented Oct 3, 2023

Uh oh!

mcollina left a comment

Uh oh!

benjamingr left a comment

Uh oh!

lpinca commented Oct 3, 2023•
edited
Loading

Uh oh!

nodejs-github-bot commented Oct 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

stream: avoid unnecessary drain for sync stream#50014

stream: avoid unnecessary drain for sync stream #50014

Conversation

ronag commented Oct 2, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 2, 2023

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Oct 3, 2023

Uh oh!

nodejs-github-bot commented Oct 3, 2023

Uh oh!

nodejs-github-bot commented Oct 3, 2023

Uh oh!

ronag commented Oct 3, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

lpinca commented Oct 3, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ronag commented Oct 2, 2023•
edited
Loading

lpinca commented Oct 3, 2023•
edited
Loading