Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

stream: improve readable webstream pipeTo #49690

Merged
merged 3 commits into from Sep 19, 2023
Merged

stream: improve readable webstream pipeTo #49690

merged 3 commits into from Sep 19, 2023

Conversation

rluvaton
Copy link
Member

@rluvaton rluvaton commented Sep 17, 2023 •
edited

improvement of ~60%

benchmark ci:

                                                                       confidence improvement accuracy (*)    (**)   (***)
webstreams/creation.js kind='ReadableStream.tee' n=50000                        *     -3.14 %       ±2.61%  ±3.49%  ±4.56%
webstreams/creation.js kind='ReadableStream' n=50000                                   0.82 %       ±3.54%  ±4.71%  ±6.13%
webstreams/creation.js kind='ReadableStreamBYOBReader' n=50000                        -0.03 %       ±4.17%  ±5.55%  ±7.23%
webstreams/creation.js kind='ReadableStreamDefaultReader' n=50000                      3.01 %      ±10.03% ±13.40% ±17.56%
webstreams/creation.js kind='TransformStream' n=50000                                  1.19 %       ±2.14%  ±2.84%  ±3.70%
webstreams/creation.js kind='WritableStream' n=50000                                  -1.15 %       ±3.61%  ±4.81%  ±6.26%
webstreams/pipe-to.js highWaterMarkW=1024 highWaterMarkR=1024 n=500000        ***     62.25 %       ±3.69%  ±4.95%  ±6.51%
webstreams/pipe-to.js highWaterMarkW=1024 highWaterMarkR=2048 n=500000        ***     66.37 %       ±3.48%  ±4.67%  ±6.15%
webstreams/pipe-to.js highWaterMarkW=1024 highWaterMarkR=4096 n=500000        ***     64.58 %       ±4.21%  ±5.65%  ±7.44%
webstreams/pipe-to.js highWaterMarkW=1024 highWaterMarkR=512 n=500000         ***     61.06 %       ±3.50%  ±4.69%  ±6.16%
webstreams/pipe-to.js highWaterMarkW=2048 highWaterMarkR=1024 n=500000        ***     66.45 %       ±3.90%  ±5.22%  ±6.85%
webstreams/pipe-to.js highWaterMarkW=2048 highWaterMarkR=2048 n=500000        ***     65.32 %       ±3.50%  ±4.70%  ±6.20%
webstreams/pipe-to.js highWaterMarkW=2048 highWaterMarkR=4096 n=500000        ***     63.26 %       ±3.89%  ±5.21%  ±6.84%
webstreams/pipe-to.js highWaterMarkW=2048 highWaterMarkR=512 n=500000         ***     64.78 %       ±3.94%  ±5.27%  ±6.93%
webstreams/pipe-to.js highWaterMarkW=4096 highWaterMarkR=1024 n=500000        ***     69.88 %       ±3.30%  ±4.41%  ±5.80%
webstreams/pipe-to.js highWaterMarkW=4096 highWaterMarkR=2048 n=500000        ***     65.36 %       ±3.73%  ±5.00%  ±6.58%
webstreams/pipe-to.js highWaterMarkW=4096 highWaterMarkR=4096 n=500000        ***     65.94 %       ±3.70%  ±4.96%  ±6.52%
webstreams/pipe-to.js highWaterMarkW=4096 highWaterMarkR=512 n=500000         ***     59.84 %       ±4.31%  ±5.78%  ±7.63%
webstreams/pipe-to.js highWaterMarkW=512 highWaterMarkR=1024 n=500000         ***     65.53 %       ±3.91%  ±5.24%  ±6.90%
webstreams/pipe-to.js highWaterMarkW=512 highWaterMarkR=2048 n=500000         ***     66.57 %       ±3.59%  ±4.81%  ±6.33%
webstreams/pipe-to.js highWaterMarkW=512 highWaterMarkR=4096 n=500000         ***     69.25 %       ±3.92%  ±5.27%  ±6.95%
webstreams/pipe-to.js highWaterMarkW=512 highWaterMarkR=512 n=500000          ***     66.25 %       ±3.61%  ±4.83%  ±6.34%
webstreams/readable-async-iterator.js n=100000                                         2.85 %       ±6.90%  ±9.17% ±11.94%

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

local tests

                                                                       confidence improvement accuracy (*)   (**)  (***)
webstreams/pipe-to.js highWaterMarkW=1024 highWaterMarkR=1024 n=500000        ***     60.99 %       ±1.17% ±1.56% ±2.04%
webstreams/pipe-to.js highWaterMarkW=1024 highWaterMarkR=2048 n=500000        ***     59.79 %       ±1.08% ±1.44% ±1.88%
webstreams/pipe-to.js highWaterMarkW=1024 highWaterMarkR=4096 n=500000        ***     60.23 %       ±1.11% ±1.48% ±1.93%
webstreams/pipe-to.js highWaterMarkW=1024 highWaterMarkR=512 n=500000         ***     59.52 %       ±1.31% ±1.75% ±2.29%
webstreams/pipe-to.js highWaterMarkW=2048 highWaterMarkR=1024 n=500000        ***     59.72 %       ±1.27% ±1.69% ±2.20%
webstreams/pipe-to.js highWaterMarkW=2048 highWaterMarkR=2048 n=500000        ***     60.98 %       ±1.19% ±1.59% ±2.07%
webstreams/pipe-to.js highWaterMarkW=2048 highWaterMarkR=4096 n=500000        ***     58.00 %       ±1.10% ±1.46% ±1.91%
webstreams/pipe-to.js highWaterMarkW=2048 highWaterMarkR=512 n=500000         ***     60.16 %       ±1.25% ±1.66% ±2.18%
webstreams/pipe-to.js highWaterMarkW=4096 highWaterMarkR=1024 n=500000        ***     59.35 %       ±1.21% ±1.62% ±2.11%
webstreams/pipe-to.js highWaterMarkW=4096 highWaterMarkR=2048 n=500000        ***     59.69 %       ±1.13% ±1.51% ±1.98%
webstreams/pipe-to.js highWaterMarkW=4096 highWaterMarkR=4096 n=500000        ***     59.78 %       ±1.02% ±1.36% ±1.78%
webstreams/pipe-to.js highWaterMarkW=4096 highWaterMarkR=512 n=500000         ***     59.84 %       ±1.43% ±1.91% ±2.52%
webstreams/pipe-to.js highWaterMarkW=512 highWaterMarkR=1024 n=500000         ***     60.59 %       ±1.10% ±1.47% ±1.91%
webstreams/pipe-to.js highWaterMarkW=512 highWaterMarkR=2048 n=500000         ***     59.37 %       ±1.17% ±1.57% ±2.05%
webstreams/pipe-to.js highWaterMarkW=512 highWaterMarkR=4096 n=500000         ***     59.87 %       ±1.24% ±1.66% ±2.17%
webstreams/pipe-to.js highWaterMarkW=512 highWaterMarkR=512 n=500000          ***     58.74 %       ±1.48% ±1.98% ±2.59%

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

@rluvaton rluvaton added performance Issues and PRs related to the performance of Node.js. needs-benchmark-ci PR that need a benchmark CI run. labels Sep 17, 2023
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Sep 17, 2023
@rluvaton
Copy link
Member Author

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

@rluvaton rluvaton marked this pull request as ready for review September 17, 2023 19:35
@rluvaton rluvaton added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 17, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 17, 2023
@nodejs-github-bot
Copy link
Collaborator

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

mcollina
mcollina approved these changes Sep 17, 2023
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

anonrig
anonrig approved these changes Sep 17, 2023
View reviewed changes
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

nice job!

@rluvaton rluvaton added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 17, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 17, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@H4ad H4ad mentioned this pull request Sep 18, 2023
Open
@nodejs-github-bot
Copy link
Collaborator

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

@MoLow MoLow added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 18, 2023
@rluvaton rluvaton added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 18, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 19, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/49690
✔  Done loading data for nodejs/node/pull/49690
----------------------------------- PR info ------------------------------------
Title      stream: improve readable webstream `pipeTo` (#49690)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     rluvaton:improve-webstream-pipe-to-perf -> nodejs:main
Labels     performance, author ready, needs-ci, needs-benchmark-ci, web streams
Commits    3
 - stream: improve readable webstream `pipeTo`
 - stream: fix lint
 - stream: inline var
Committers 1
 - Raz Luvaton <16746759+rluvaton@users.noreply.github.com>
PR-URL: https://github.com/nodejs/node/pull/49690
Reviewed-By: Matteo Collina 
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Moshe Atlow 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/49690
Reviewed-By: Matteo Collina 
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Moshe Atlow 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 17 Sep 2023 19:09:30 GMT
   ✔  Approvals: 3
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/49690#pullrequestreview-1630123133
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/49690#pullrequestreview-1630128154
   ✔  - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/49690#pullrequestreview-1630314547
   ✔  Last GitHub CI successful
   ℹ  Last Benchmark CI on 2023-09-17T19:12:26Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1392/
   ℹ  Last Full PR CI on 2023-09-18T04:32:27Z: https://ci.nodejs.org/job/node-test-pull-request/54022/
- Querying data for job/node-test-pull-request/54022/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 49690
From https://github.com/nodejs/node
 * branch                  refs/pull/49690/merge -> FETCH_HEAD
✔  Fetched commits as 1149d6b49de9..fac8ca681a0d
--------------------------------------------------------------------------------
Auto-merging lib/internal/webstreams/readablestream.js
[main 6c0ce1d299] stream: improve readable webstream `pipeTo`
 Author: Raz Luvaton <16746759+rluvaton@users.noreply.github.com>
 Date: Sun Sep 17 21:59:46 2023 +0300
 1 file changed, 35 insertions(+), 16 deletions(-)
Auto-merging lib/internal/webstreams/readablestream.js
[main 678f8fc97a] stream: fix lint
 Author: Raz Luvaton <16746759+rluvaton@users.noreply.github.com>
 Date: Sun Sep 17 22:39:38 2023 +0300
 1 file changed, 2 insertions(+), 2 deletions(-)
Auto-merging lib/internal/webstreams/readablestream.js
[main aa0654f732] stream: inline var
 Author: Raz Luvaton <16746759+rluvaton@users.noreply.github.com>
 Date: Sun Sep 17 22:57:16 2023 +0300
 1 file changed, 1 insertion(+), 3 deletions(-)
   ✔  Patches applied
There are 3 commits in the PR. Attempting autorebase.
Rebasing (2/6)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

stream: improve readable webstream pipeTo


PR-URL: #49690

Reviewed-By: Matteo Collina matteo.collina@gmail.com

Reviewed-By: Yagiz Nizipli yagiz@nizipli.com

Reviewed-By: Moshe Atlow moshe@atlow.co.il


[detached HEAD 975bb54bac] stream: improve readable webstream pipeTo

Author: Raz Luvaton 16746759+rluvaton@users.noreply.github.com

Date: Sun Sep 17 21:59:46 2023 +0300

1 file changed, 35 insertions(+), 16 deletions(-)

Rebasing (3/6)

Rebasing (4/6)


Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

stream: fix lint


PR-URL: #49690

Reviewed-By: Matteo Collina matteo.collina@gmail.com

Reviewed-By: Yagiz Nizipli yagiz@nizipli.com

Reviewed-By: Moshe Atlow moshe@atlow.co.il


[detached HEAD 1e53e06250] stream: fix lint

Author: Raz Luvaton 16746759+rluvaton@users.noreply.github.com

Date: Sun Sep 17 22:39:38 2023 +0300

1 file changed, 2 insertions(+), 2 deletions(-)

Rebasing (5/6)

Rebasing (6/6)


Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

stream: inline var


PR-URL: #49690

Reviewed-By: Matteo Collina matteo.collina@gmail.com

Reviewed-By: Yagiz Nizipli yagiz@nizipli.com

Reviewed-By: Moshe Atlow moshe@atlow.co.il


[detached HEAD beca4e7ce3] stream: inline var

Author: Raz Luvaton 16746759+rluvaton@users.noreply.github.com

Date: Sun Sep 17 22:57:16 2023 +0300

1 file changed, 1 insertion(+), 3 deletions(-)


Successfully rebased and updated refs/heads/main.


ℹ  Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.
https://github.com/nodejs/node/actions/runs/6239919725

@rluvaton rluvaton added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Sep 19, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 19, 2023
@nodejs-github-bot nodejs-github-bot merged commit 346abdd into nodejs:main Sep 19, 2023
67 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 346abdd

@rluvaton rluvaton deleted the improve-webstream-pipe-to-perf branch September 19, 2023 19:23
ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
@rluvaton @ruyadorno
stream: improve readable webstream pipeTo
b29d927 
PR-URL: #49690
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
This was referenced Sep 28, 2023
Closed
Merged
@hyunbinseo hyunbinseo mentioned this pull request Oct 16, 2023
Open
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
@rluvaton @alexfernandez
stream: improve readable webstream pipeTo
cdc6f45 
PR-URL: nodejs#49690
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@anonrig anonrig anonrig approved these changes

@MoLow MoLow MoLow approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-benchmark-ci PR that need a benchmark CI run. needs-ci PRs that need a full CI run. performance Issues and PRs related to the performance of Node.js. web streams
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@rluvaton @nodejs-github-bot @mcollina @anonrig @MoLow