Skip to content

stream: bump default highWaterMark#50120

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
Closed
RafaelGSS wants to merge 5 commits into from
Closed

stream: bump default highWaterMark #50120

RafaelGSS wants to merge 5 commits into from

Conversation

@RafaelGSS
Copy link
Member

@RafaelGSSRafaelGSS commented Oct 10, 2023

Original PR: #46608

@RafaelGSSRafaelGSS added stream Issues and PRs related to the stream subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Oct 10, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 10, 2023

Review requested:

  • @nodejs/streams

@nodejs-github-botnodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 10, 2023
@RafaelGSSRafaelGSS added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Oct 10, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
rubiesonthesky
rubiesonthesky reviewed Oct 10, 2023
View reviewed changes
@github-actionsgithub-actionsbot mentioned this pull request Oct 11, 2023
Open
30 tasks
mcollina
mcollina approved these changes Oct 11, 2023
View reviewed changes
Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

lgtm

mcollina
mcollina approved these changes Oct 29, 2023
View reviewed changes
Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@benjamingrbenjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 29, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 29, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
This was referenced Oct 30, 2023
Open
Open
Open
Open
@ronag@RafaelGSS
stream: bump default highWaterMark
5d6e5c9
This should give a performance boost accross the board. Given that the old limit is a decod old and memory capacity has doubled many times since I think it is appropriate to slightly bump the default limit.
@RafaelGSSRafaelGSSforce-pushed the bump-highWaterMark-value branch from 4ad103c to 5d6e5c9CompareDecember 4, 2023 21:42
@RafaelGSSRafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 4, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 4, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@RafaelGSSRafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 19, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 25, 2023

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 25, 2023

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

@lpinca
Copy link
Member

lpinca commented Dec 26, 2023•
edited
Loading

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3369/
Benchmark CI (streams): https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1485/
Benchmark CI (net): https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1486/
Benchmark CI (http): https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1487/

@lpinca
Copy link
Member

lpinca commented Dec 26, 2023•
edited
Loading

@RafaelGSS the last commit does not make much sense. I don't think we should update the benchmark to use 64 MiB instead of 16 MiB.

@RafaelGSS
Copy link
MemberAuthor

RafaelGSS commented Dec 26, 2023

@RafaelGSS the last commit does not make much sense. I don't think we should update the benchmark to use 64 MiB instead of 16 MiB.

Yeah, it was a naive attempt. Since we're getting ENOBUFS on Windows benchmarks, I believe we should reduce it? Considering the memory consumption should have increased.

@RafaelGSSRafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 26, 2023
@lpinca
Copy link
Member

lpinca commented Dec 26, 2023

Yes, I think so.

@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 26, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 26, 2023

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

lpinca added a commit to websockets/ws that referenced this pull request Dec 26, 2023
@lpinca
[test] Use stream.getDefaultHighWaterMark() when available
391ddf3
Refs: nodejs/node#50120
@MomenNano
Copy link

MomenNano commented Mar 8, 2024

Any plans to get this to v22?

@mcollinamcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 8, 2024
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 8, 2024
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 8, 2024

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

@ronagronag mentioned this pull request Mar 10, 2024
Merged
ronag added a commit to nxtedition/node that referenced this pull request Mar 10, 2024
@ronag
stream: bump default highWaterMark
26cf776
This should give a performance boost accross the board. Given that the old limit is a decod old and memory capacity has doubled many times since I think it is appropriate to slightly bump the default limit. Refs: nodejs#46608 Refs: nodejs#50120
ronag added a commit to nxtedition/node that referenced this pull request Mar 10, 2024
@ronag
stream: bump default highWaterMark
3c400fc
This should give a performance boost accross the board. Given that the old limit is a decod old and memory capacity has doubled many times since I think it is appropriate to slightly bump the default limit. PR-URL: nodejs#52037 Refs: nodejs#46608 Refs: nodejs#50120
ronag added a commit to nxtedition/node that referenced this pull request Mar 10, 2024
@ronag
stream: bump default highWaterMark
8f4a308
This should give a performance boost accross the board. Given that the old limit is a decod old and memory capacity has doubled many times since I think it is appropriate to slightly bump the default limit. PR-URL: nodejs#52037 Refs: nodejs#46608 Refs: nodejs#50120
ronag added a commit to nxtedition/node that referenced this pull request Mar 11, 2024
@ronag
stream: bump default highWaterMark
b73ec99
This should give a performance boost accross the board. Given that the old limit is a decod old and memory capacity has doubled many times since I think it is appropriate to slightly bump the default limit. PR-URL: nodejs#52037 Refs: nodejs#46608 Refs: nodejs#50120
ronag added a commit to nxtedition/node that referenced this pull request Mar 11, 2024
@ronag
stream: bump default highWaterMark
cc2c84d
This should give a performance boost accross the board. Given that the old limit is a decod old and memory capacity has doubled many times since I think it is appropriate to slightly bump the default limit. PR-URL: nodejs#52037 Refs: nodejs#46608 Refs: nodejs#50120
@trivikr
Copy link
Member

trivikr commented Mar 11, 2024

Should this be closed in favor of #52037?

ronag added a commit to nxtedition/node that referenced this pull request Mar 13, 2024
@ronag
stream: bump default highWaterMark
207a3f5
This should give a performance boost accross the board. Given that the old limit is a decod old and memory capacity has doubled many times since I think it is appropriate to slightly bump the default limit. PR-URL: nodejs#52037 Refs: nodejs#46608 Refs: nodejs#50120
ronag added a commit to nxtedition/node that referenced this pull request Mar 13, 2024
@ronag
stream: bump default highWaterMark
92087be
This should give a performance boost accross the board. Given that the old limit is a decod old and memory capacity has doubled many times since I think it is appropriate to slightly bump the default limit. PR-URL: nodejs#52037 Refs: nodejs#46608 Refs: nodejs#50120
nodejs-github-bot pushed a commit that referenced this pull request Mar 13, 2024
@ronag
stream: bump default highWaterMark
1abff07
This should give a performance boost accross the board. Given that the old limit is a decod old and memory capacity has doubled many times since I think it is appropriate to slightly bump the default limit. PR-URL: #52037 Refs: #46608 Refs: #50120 Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
@ronag@rdw-msft
stream: bump default highWaterMark
c6c57d5
This should give a performance boost accross the board. Given that the old limit is a decod old and memory capacity has doubled many times since I think it is appropriate to slightly bump the default limit. PR-URL: nodejs#52037 Refs: nodejs#46608 Refs: nodejs#50120 Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
jcbhmr pushed a commit to jcbhmr/node that referenced this pull request May 15, 2024
@ronag@jcbhmr
stream: bump default highWaterMark
29dce7f
This should give a performance boost accross the board. Given that the old limit is a decod old and memory capacity has doubled many times since I think it is appropriate to slightly bump the default limit. PR-URL: nodejs#52037 Refs: nodejs#46608 Refs: nodejs#50120 Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes

@jasnelljasnelljasnell approved these changes

@benjamingrbenjamingrbenjamingr approved these changes

@ronagronagronag approved these changes

@MoLowMoLowMoLow approved these changes

@trivikrtrivikrtrivikr approved these changes

+2 more reviewers

@mscdexmscdexmscdex left review comments

@rubiesontheskyrubiesontheskyrubiesonthesky left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.semver-majorPRs that contain breaking changes and should be released in the next major version.streamIssues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

12 participants

@RafaelGSS@nodejs-github-bot@lpinca@MomenNano@trivikr@mcollina@mscdex@jasnell@benjamingr@rubiesonthesky@ronag@MoLow