stream: bump default highWaterMark by RafaelGSS · Pull Request #50120 · nodejs/node · GitHub

Original PR: #46608

Review requested:

@nodejs/streams

lgtm

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.

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

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

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/

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

This reverts commit 4411343.

Yes, I think so.

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

Refs: nodejs/node#50120

Any plans to get this to v22?

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

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

Should this be closed in favor of #52037?

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

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 <rafael.nunu@hotmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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

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 Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2023

This comment was marked as outdated.

Sign in to view

rubiesonthesky reviewed Oct 10, 2023

View reviewed changes

Comment thread doc/api/stream.md Outdated

github-actions Bot mentioned this pull request Oct 11, 2023

CI Reliability 2023-10-11 nodejs/reliability#690

Open

30 tasks

mcollina approved these changes Oct 11, 2023

View reviewed changes

ronag approved these changes Oct 11, 2023

View reviewed changes

MoLow approved these changes Oct 11, 2023

View reviewed changes

jasnell approved these changes Oct 28, 2023

View reviewed changes

mcollina approved these changes Oct 29, 2023

View reviewed changes

benjamingr approved these changes Oct 29, 2023

View reviewed changes

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 29, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 29, 2023

This comment was marked as outdated.

Sign in to view

This was referenced Oct 30, 2023

CI Reliability 2023-10-30 nodejs/reliability#702

Open

CI Reliability 2023-10-31 nodejs/reliability#703

Open

CI Reliability 2023-11-01 nodejs/reliability#704

Open

CI Reliability 2023-11-02 nodejs/reliability#705

Open

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 force-pushed the bump-highWaterMark-value branch from 4ad103c to 5d6e5c9 Compare December 4, 2023 21:42

RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 4, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 4, 2023

This comment was marked as outdated.

Sign in to view

fixup! stream: bump default highWaterMark

8d7a929

RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 19, 2023

Revert "fixup! adjust net benchmark to 64"

64dd3fd

This reverts commit 4411343.

RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 26, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 26, 2023

fixup! attempt to fix ENOBUFS windows

95fa1d3

lpinca added a commit to websockets/ws that referenced this pull request Dec 26, 2023

[test] Use stream.getDefaultHighWaterMark() when available

391ddf3

Refs: nodejs/node#50120

trivikr approved these changes Dec 27, 2023

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 8, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 8, 2024

ronag mentioned this pull request Mar 10, 2024

stream: bump default highWaterMark #52037

Merged

mcollina closed this Mar 14, 2024

stream: bump default highWaterMark by RafaelGSS · Pull Request #50120 · nodejs/node · GitHub

Uh oh!

Uh oh!

stream: bump default highWaterMark - #50120

stream: bump default highWaterMark#50120RafaelGSS wants to merge 5 commits intonodejs:mainnodejs/node:mainfrom RafaelGSS:bump-highWaterMark-valueRafaelGSS/node:bump-highWaterMark-valueCopy head branch name to clipboard

Conversation

RafaelGSS commented Oct 10, 2023

Uh oh!

nodejs-github-bot commented Oct 10, 2023

Uh oh!

This comment was marked as outdated.

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Dec 25, 2023

Uh oh!

nodejs-github-bot commented Dec 25, 2023

Uh oh!

lpinca commented Dec 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpinca commented Dec 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RafaelGSS commented Dec 26, 2023

Uh oh!

lpinca commented Dec 26, 2023

Uh oh!

nodejs-github-bot commented Dec 26, 2023

Uh oh!

MomenNano commented Mar 8, 2024

Uh oh!

nodejs-github-bot commented Mar 8, 2024

Uh oh!

trivikr commented Mar 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

stream: bump default highWaterMark#50120
RafaelGSS wants to merge 5 commits into
nodejs:mainfrom
RafaelGSS:bump-highWaterMark-value

lpinca commented Dec 26, 2023 •
edited
Loading

lpinca commented Dec 26, 2023 •
edited
Loading