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 mcollina and benjamingr 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 e384e57 Compare 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 a4aa161 Compare 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 35087cb Compare 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 2b8fe13 Compare 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 f7e2c54 Compare 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 e39b0c0 Compare 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 bdfe338 Compare 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-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 2, 2023

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

stream: avoid unnecessary drain for sync stream

4844d30

PR-URL: nodejs#50014

ronag force-pushed the drain-sync branch from 980032d to 4844d30 Compare 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-actions bot 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:main Oct 4, 2023
24 checks passed

github-actions bot 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: avoid unnecessary drain for sync stream #50014

stream: avoid unnecessary drain for sync stream #50014

ronag commented Oct 2, 2023 •

edited

Loading

nodejs-github-bot commented Oct 2, 2023

This comment was marked as outdated.

nodejs-github-bot commented Oct 3, 2023

nodejs-github-bot commented Oct 3, 2023

nodejs-github-bot commented Oct 3, 2023

ronag commented Oct 3, 2023

mcollina left a comment

benjamingr left a comment

lpinca commented Oct 3, 2023 •

edited

Loading

nodejs-github-bot commented Oct 4, 2023

stream: avoid unnecessary drain for sync stream #50014

stream: avoid unnecessary drain for sync stream #50014

Conversation

ronag commented Oct 2, 2023 • edited Loading

nodejs-github-bot commented Oct 2, 2023

This comment was marked as outdated.

nodejs-github-bot commented Oct 3, 2023

nodejs-github-bot commented Oct 3, 2023

nodejs-github-bot commented Oct 3, 2023

ronag commented Oct 3, 2023

mcollina left a comment

Choose a reason for hiding this comment

benjamingr left a comment

Choose a reason for hiding this comment

lpinca commented Oct 3, 2023 • edited Loading

nodejs-github-bot commented Oct 4, 2023

ronag commented Oct 2, 2023 •

edited

Loading

lpinca commented Oct 3, 2023 •

edited

Loading