stream: improve readable stream read perf by rluvaton · Pull Request #50173 · nodejs/node · GitHub

Performance improvement for readable.read by around ~10%

Benchmarks

Benchmark URL

                                                                                         confidence improvement accuracy (*)    (**)   (***)
streams/creation.js kind='duplex' n=50000000                                                             0.05 %       ±1.22%  ±1.62%  ±2.12%
streams/creation.js kind='readable' n=50000000                                                          -1.00 %       ±1.12%  ±1.49%  ±1.95%
streams/creation.js kind='transform' n=50000000                                                          2.30 %       ±4.02%  ±5.35%  ±6.96%
streams/creation.js kind='writable' n=50000000                                                          -0.14 %       ±0.90%  ±1.20%  ±1.57%
streams/destroy.js kind='duplex' n=1000000                                                              -1.51 %       ±1.88%  ±2.50%  ±3.25%
streams/destroy.js kind='readable' n=1000000                                                            -1.95 %       ±2.02%  ±2.69%  ±3.51%
streams/destroy.js kind='transform' n=1000000                                                           -0.28 %       ±1.45%  ±1.93%  ±2.51%
streams/destroy.js kind='writable' n=1000000                                                            -0.03 %       ±2.02%  ±2.69%  ±3.50%
streams/pipe-object-mode.js n=5000000                                                                    1.40 %       ±3.61%  ±4.81%  ±6.26%
streams/pipe.js n=5000000                                                                               -0.57 %       ±2.60%  ±3.47%  ±4.53%
streams/readable-async-iterator.js sync='no' n=100000                                                    1.39 %       ±3.39%  ±4.55%  ±6.01%
streams/readable-async-iterator.js sync='yes' n=100000                                                  -1.66 %       ±2.95%  ±3.97%  ±5.25%
streams/readable-bigread.js n=1000                                                                      -0.36 %       ±7.13%  ±9.49% ±12.35%
streams/readable-bigunevenread.js n=1000                                                                 0.72 %       ±1.64%  ±2.18%  ±2.84%
streams/readable-boundaryread.js type='buffer' n=2000                                           ***      6.36 %       ±1.25%  ±1.67%  ±2.17%
streams/readable-boundaryread.js type='string' n=2000                                           ***      2.66 %       ±1.42%  ±1.90%  ±2.49%
streams/readable-from.js n=10000000                                                                     -4.26 %       ±8.28% ±11.05% ±14.45%
streams/readable-readall.js n=5000                                                              ***     10.08 %       ±5.50%  ±7.33%  ±9.57%
streams/readable-unevenread.js n=1000                                                                   -0.15 %       ±2.90%  ±3.86%  ±5.02%
streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='no' n=100000                    -0.37 %       ±3.01%  ±4.01%  ±5.22%
streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='yes' n=100000                    0.54 %       ±2.45%  ±3.26%  ±4.25%
streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='no' n=100000                   -0.02 %       ±2.75%  ±3.68%  ±4.82%
streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='yes' n=100000                   0.35 %       ±2.39%  ±3.18%  ±4.14%
streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='no' n=100000                    0.78 %       ±2.31%  ±3.11%  ±4.11%
streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='yes' n=100000            *      2.81 %       ±2.52%  ±3.36%  ±4.37%
streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='no' n=100000            *     -2.73 %       ±2.48%  ±3.29%  ±4.29%
streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='yes' n=100000                 -1.59 %       ±2.34%  ±3.12%  ±4.06%
streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='no' n=100000                   -1.57 %       ±2.72%  ±3.66%  ±4.83%
streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='yes' n=100000                  -0.27 %       ±2.67%  ±3.55%  ±4.62%
streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='no' n=100000                  -2.95 %       ±3.00%  ±4.03%  ±5.32%
streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='yes' n=100000                  0.94 %       ±2.69%  ±3.57%  ±4.65%
streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='no' n=100000                  -0.42 %       ±2.25%  ±3.01%  ±3.96%
streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='yes' n=100000                  0.03 %       ±2.43%  ±3.23%  ±4.20%
streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='no' n=100000                  0.18 %       ±2.33%  ±3.12%  ±4.13%
streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='yes' n=100000                -0.37 %       ±2.42%  ±3.22%  ±4.19%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 35 comparisons, you can thus
expect the following amount of false-positive results:
  1.75 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.35 false positives, when considering a   1% risk acceptance (**, ***),
  0.04 false positives, when considering a 0.1% risk acceptance (***)

Review requested:

@nodejs/streams

need to extract as well and cleanup

Real OG stuff ;)

lgtm

Benchmark URL: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1443

Why do you overwrite push/unshift instead of just dispatching to the correct helper method?

Benchmark URL: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1444

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

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @rluvaton.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment.

Landed in 3907bd1

PR-URL: #50173 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Notable changes: doc: * add H4ad to collaborators (Vinícius Lourenço) #50217 esm: * (SEMVER-MINOR) detect ESM syntax in ambiguous JavaScript (Geoffrey Booth) #50096 fs: * (SEMVER-MINOR) add flush option to appendFile() functions (Colin Ihrig) #50095 lib: * (SEMVER-MINOR) add `navigator.userAgent` (Yagiz Nizipli) #50200 stream: * (SEMVER-MINOR) allow pass stream class to `stream.compose` (Alex Yang) #50187 * call helper function from push and unshift (Raz Luvaton) #50173 PR-URL: #50335

PR-URL: #50173 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Notable changes: deps: * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) #49908 doc: * add H4ad to collaborators (Vinícius Lourenço) #50217 esm: * (SEMVER-MINOR) detect ESM syntax in ambiguous JavaScript (Geoffrey Booth) #50096 * use import attributes instead of import assertions (Antoine du Hamel) #50140 * --experimental-default-type flag to flip module defaults (Geoffrey Booth) #49869 fs: * (SEMVER-MINOR) add flush option to appendFile() functions (Colin Ihrig) #50095 * add flush option to writeFile() functions (Colin Ihrig) #50009 lib: * (SEMVER-MINOR) add WebSocket client (Matthew Aitken) #49830 stream: * (SEMVER-MINOR) allow pass stream class to `stream.compose` (Alex Yang) #50187 * call helper function from push and unshift (Raz Luvaton) #50173 * optimize Writable (Robert Nagy) #50012 test_runner, cli: * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) #49996 vm: * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) #50141 * use default HDO when importModuleDynamically is not set (Joyee Cheung) #49950 wasi: PR-URL: #50682

Notable changes: deps: * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) nodejs#49908 doc: * add H4ad to collaborators (Vinícius Lourenço) nodejs#50217 esm: * (SEMVER-MINOR) detect ESM syntax in ambiguous JavaScript (Geoffrey Booth) nodejs#50096 * use import attributes instead of import assertions (Antoine du Hamel) nodejs#50140 * --experimental-default-type flag to flip module defaults (Geoffrey Booth) nodejs#49869 fs: * (SEMVER-MINOR) add flush option to appendFile() functions (Colin Ihrig) nodejs#50095 * add flush option to writeFile() functions (Colin Ihrig) nodejs#50009 lib: * (SEMVER-MINOR) add WebSocket client (Matthew Aitken) nodejs#49830 stream: * (SEMVER-MINOR) allow pass stream class to `stream.compose` (Alex Yang) nodejs#50187 * call helper function from push and unshift (Raz Luvaton) nodejs#50173 * optimize Writable (Robert Nagy) nodejs#50012 test_runner, cli: * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) nodejs#49996 vm: * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) nodejs#50141 * use default HDO when importModuleDynamically is not set (Joyee Cheung) nodejs#49950 wasi: PR-URL: nodejs#50682

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

rluvaton commented Oct 13, 2023

View reviewed changes

rluvaton added the needs-ci PRs that need a full CI run. label Oct 13, 2023

rluvaton marked this pull request as ready for review October 13, 2023 14:31

rluvaton commented Oct 13, 2023

View reviewed changes

Comment thread lib/internal/streams/readable.js Outdated

mcollina approved these changes Oct 13, 2023

View reviewed changes

ronag approved these changes Oct 13, 2023

View reviewed changes

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

This comment was marked as outdated.

Sign in to view

shrujalshah28 reviewed Oct 14, 2023

View reviewed changes

Comment thread lib/internal/streams/readable.js Outdated

rluvaton added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 14, 2023

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

This comment was marked as outdated.

Sign in to view

benjamingr approved these changes Oct 14, 2023

View reviewed changes

rluvaton added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 14, 2023

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

This comment was marked as outdated.

Sign in to view

ronag reviewed Oct 14, 2023

View reviewed changes

Comment thread lib/internal/streams/readable.js Outdated

ronag reviewed Oct 14, 2023

View reviewed changes

Comment thread lib/internal/streams/readable.js Outdated

This comment was marked as resolved.

Sign in to view

ronag reviewed Oct 14, 2023

View reviewed changes

rluvaton force-pushed the improve-readable-push-perf branch from dafe7d9 to c65f92d Compare October 14, 2023 20:30

ronag approved these changes Oct 15, 2023

View reviewed changes

rluvaton added the notable-change PRs with changes that should be highlighted in changelogs. label Oct 15, 2023

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

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

nodejs-github-bot merged commit 3907bd1 into nodejs:main Oct 15, 2023

rluvaton deleted the improve-readable-push-perf branch October 15, 2023 18:43

targos mentioned this pull request Oct 23, 2023

v21.1.0 release proposal #50335

Merged

wa-Nadoo mentioned this pull request Oct 26, 2023

stream: consolidate common code from readable push and unshift helper… #50402

Closed

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

stream: improve readable stream read perf by rluvaton · Pull Request #50173 · nodejs/node · GitHub

Uh oh!

Uh oh!

stream: improve readable stream read perf - #50173

stream: improve readable stream read perf#50173nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom rluvaton:improve-readable-push-perfrluvaton/node:improve-readable-push-perfCopy head branch name to clipboard

Conversation

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

Uh oh!

Benchmarks

Uh oh!

nodejs-github-bot commented Oct 13, 2023

Uh oh!

rluvaton Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

Uzlopak Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

rluvaton commented Oct 14, 2023

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

Uh oh!

This comment was marked as resolved.

ronag left a comment

Choose a reason for hiding this comment

Uh oh!

rluvaton commented Oct 14, 2023

Uh oh!

nodejs-github-bot commented Oct 15, 2023

Uh oh!

github-actions Bot commented Oct 15, 2023

Uh oh!

nodejs-github-bot commented Oct 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

stream: improve readable stream read perf#50173
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
rluvaton:improve-readable-push-perf

rluvaton commented Oct 13, 2023 •
edited
Loading