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: bump default highWaterMark #50120

Closed
wants to merge 5 commits into from
Closed

stream: bump default highWaterMark #50120

wants to merge 5 commits into from

Conversation

RafaelGSS
Copy link
Member

Original PR: #46608

@RafaelGSS RafaelGSS 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

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 10, 2023
@RafaelGSS RafaelGSS 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-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

This comment was marked as outdated.

Sign in to view
@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

mcollina
mcollina approved these changes Oct 29, 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

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 29, 2023
@github-actions github-actions bot 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.
@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 4, 2023
@github-actions github-actions bot 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
@RafaelGSS RafaelGSS 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

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

@nodejs-github-bot
Copy link
Collaborator

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
Member Author

@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.

@RafaelGSS RafaelGSS 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-actions github-actions bot 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

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

Any plans to get this to v22?

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 8, 2024
@github-actions github-actions bot 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

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

@ronag ronag 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]>
@mcollina mcollina closed this Mar 14, 2024
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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rubiesonthesky rubiesonthesky rubiesonthesky left review comments

@mscdex mscdex mscdex left review comments

@trivikr trivikr trivikr approved these changes

@MoLow MoLow MoLow approved these changes

@ronag ronag ronag approved these changes

@benjamingr benjamingr benjamingr approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina 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. semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.