Skip to content

stream: reduce overhead of transfer#50107

Merged
nodejs-github-bot merged 1 commit intonodejs:mainfrom
H4ad:perf/webstreams_writable
Oct 12, 2023
Merged

stream: reduce overhead of transfer#50107
nodejs-github-bot merged 1 commit intonodejs:mainfrom
H4ad:perf/webstreams_writable

Conversation

@H4ad
Copy link
Member

@H4ad H4ad commented Oct 9, 2023 •
edited
Loading

Continuing the work started on nodejs/performance#109

                                                            confidence improvement accuracy (*)   (**)  (***)
webstreams/js_transfer.js n=10000 payload='ReadableStream'         ***     19.68 %       ±1.77% ±2.35% ±3.06%
webstreams/js_transfer.js n=10000 payload='TransformStream'        ***     18.80 %       ±1.76% ±2.35% ±3.06%
webstreams/js_transfer.js n=10000 payload='WritableStream'         ***     13.36 %       ±1.73% ±2.30% ±3.00%

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

/cc @nodejs/performance

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 9, 2023
@H4ad H4ad force-pushed the perf/webstreams_writable branch from bc826eb to b0b5614 Compare October 10, 2023 12:15
@H4ad H4ad marked this pull request as ready for review October 10, 2023 12:16
@H4ad H4ad force-pushed the perf/webstreams_writable branch from 578d490 to d2e73bc Compare October 10, 2023 12:20
@H4ad H4ad force-pushed the perf/webstreams_writable branch from d2e73bc to 68b0d91 Compare October 10, 2023 12:22
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2023
mcollina
mcollina approved these changes Oct 10, 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

H4ad
H4ad commented Oct 10, 2023
View reviewed changes
lib/internal/webstreams/readablestream.js
Comment on lines +654 to +657
writable: undefined,
port1: undefined,
port2: undefined,
promise: undefined,
Copy link
Member Author

@H4ad H4ad Oct 10, 2023

Choose a reason for hiding this comment

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

For these properties, I followed what I saw in the constructor, the old version was wrong, right?

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

nodejs-github-bot commented Oct 10, 2023

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

@anonrig anonrig changed the title perf: webstreams transfer stream: reduce overhead of transfer Oct 10, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 10, 2023

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

@github-actions github-actions bot mentioned this pull request Oct 11, 2023
Open
30 tasks
mcollina
mcollina approved these changes Oct 11, 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

@H4ad
Copy link
Member Author

H4ad commented Oct 12, 2023

Someone can add author ready?

@anonrig anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 12, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 12, 2023
@nodejs-github-bot nodejs-github-bot merged commit a85e418 into nodejs:main Oct 12, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 12, 2023

Landed in a85e418

@H4ad H4ad deleted the perf/webstreams_writable branch October 12, 2023 14:42
@RafaelGSS RafaelGSS mentioned this pull request Oct 15, 2023
Merged
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
@H4ad @alexfernandez
stream: reduce overhead of transfer
940ff7c 
PR-URL: nodejs#50107
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Nov 11, 2023
@MattiasBuelens MattiasBuelens mentioned this pull request May 1, 2024
Merged
@MattiasBuelens MattiasBuelens mentioned this pull request May 1, 2024
Closed
@targos targos mentioned this pull request Sep 19, 2024
Closed
@avivkeller avivkeller mentioned this pull request Sep 21, 2024
Closed
@jazelly jazelly mentioned this pull request Sep 22, 2024
Merged
nodejs-github-bot pushed a commit that referenced this pull request Sep 24, 2024
@jazelly
stream: set stream prototype to closest transferable superclass
0e52836 
PR-URL: #55067
Fixes: #54603
Refs: #50107
Reviewed-By: Mattias Buelens <mattias@buelens.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this pull request Oct 4, 2024
@jazelly @targos
stream: set stream prototype to closest transferable superclass
bf69ae1 
PR-URL: #55067
Fixes: #54603
Refs: #50107
Reviewed-By: Mattias Buelens <mattias@buelens.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
@jazelly @louwers
stream: set stream prototype to closest transferable superclass
340ebda 
PR-URL: nodejs#55067
Fixes: nodejs#54603
Refs: nodejs#50107
Reviewed-By: Mattias Buelens <mattias@buelens.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
@jazelly @tpoisseau
stream: set stream prototype to closest transferable superclass
abe7c33 
PR-URL: nodejs#55067
Fixes: nodejs#54603
Refs: nodejs#50107
Reviewed-By: Mattias Buelens <mattias@buelens.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
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

@Qard Qard Qard approved these changes

@anonrig anonrig anonrig 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. c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@H4ad @nodejs-github-bot @mcollina @Qard @anonrig @targos