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

rluvaton · 2023-10-13T13:03:13Z

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 (***)

nodejs-github-bot · 2023-10-13T13:03:19Z

Review requested:

@nodejs/streams

rluvaton · 2023-10-13T13:05:58Z

lib/internal/streams/readable.js

 };

-function readableAddChunk(stream, chunk, encoding, addToFront) {
+function readableAddChunkOg(stream, chunk, encoding, addToFront) {


need to extract as well and cleanup

Real OG stuff ;)

lib/internal/streams/readable.js

mcollina

lgtm

lib/internal/streams/readable.js

rluvaton · 2023-10-14T16:14:46Z

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

lib/internal/streams/readable.js

ronag

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

rluvaton · 2023-10-14T20:35:55Z

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

nodejs-github-bot · 2023-10-15T17:01:15Z

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

github-actions · 2023-10-15T17:18:59Z

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.

nodejs-github-bot · 2023-10-15T18:31:10Z

Landed in 3907bd1

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

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

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

… functions nodejs#50173 brings too many code duplication. Trying to reduce it a bit.

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

Notable changes: doc: * add H4ad to collaborators (Vinícius Lourenço) nodejs#50217 esm: * (SEMVER-MINOR) detect ESM syntax in ambiguous JavaScript (Geoffrey Booth) nodejs#50096 fs: * (SEMVER-MINOR) add flush option to appendFile() functions (Colin Ihrig) nodejs#50095 lib: * (SEMVER-MINOR) add `navigator.userAgent` (Yagiz Nizipli) nodejs#50200 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 PR-URL: nodejs#50335

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

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

lib/internal/streams/readable.js Outdated Show resolved Hide resolved

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

lib/internal/streams/readable.js Outdated Show resolved Hide resolved

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

lib/internal/streams/readable.js Outdated Show resolved Hide resolved

ronag reviewed Oct 14, 2023

View reviewed changes

lib/internal/streams/readable.js Outdated Show resolved Hide resolved

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 pushed a commit that referenced this pull request Oct 23, 2023

stream: call helper function from push and unshift

f3a9ea0

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

targos mentioned this pull request Oct 23, 2023

v21.1.0 release proposal #50335

Merged

wa-Nadoo added a commit to wa-Nadoo/node that referenced this pull request Oct 26, 2023

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

337ba20

… functions nodejs#50173 brings too many code duplication. Trying to reduce it a bit.

wa-Nadoo mentioned this pull request Oct 26, 2023

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

Open

targos pushed a commit that referenced this pull request Nov 11, 2023

stream: call helper function from push and unshift

4861ad6

PR-URL: #50173 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Robert Nagy <[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

Uh oh!

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

rluvaton commented Oct 13, 2023 •

edited

Loading