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: add global Stream.defaultHighwaterMark #40511

Closed
wants to merge 11 commits into from
Closed

stream: add global Stream.defaultHighwaterMark #40511

wants to merge 11 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Oct 19, 2021

No description provided.

@ronag ronag requested a review from mcollina October 19, 2021 05:57
@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. needs-ci PRs that need a full CI run. labels Oct 19, 2021
@ronag ronag added the stream Issues and PRs related to the stream subsystem. label Oct 19, 2021
@ronag
Copy link
Member Author

ronag commented Oct 19, 2021

@nodejs/streams @nodejs/buffer

mcollina
mcollina reviewed Oct 19, 2021
View reviewed changes
lib/buffer.js Outdated Show resolved Hide resolved
@ronag
Copy link
Member Author

ronag commented Oct 19, 2021 •
edited

Some background for this. In high memory environment we usually override Buffer.poolSize to e.g. 128 * 1024 to reduce some overheads. It would be very useful if these kind of adjustments (both in terms of increasing and decreasing) would propagate to other parts of Node, e.g. streams.

benjamingr
benjamingr approved these changes Oct 19, 2021
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Ideally this would also have a test that sets Buffer.poolSize and checks hwm

@ronag ronag 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 19, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 19, 2021
@nodejs-github-bot
Copy link
Collaborator

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

mcollina
mcollina reviewed Oct 19, 2021
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.

I think this should be its own commit or possibly its own PR.

I agree with the change.

@mcollina mcollina added the notable-change PRs with changes that should be highlighted in changelogs. label Oct 19, 2021
@ronag
Copy link
Member Author

ronag commented Oct 19, 2021

I think this should be its own commit or possibly its own PR.

What should be?

@ronag
Copy link
Member Author

ronag commented Oct 19, 2021

Split into 2 commits

@mcollina
Copy link
Member

cc @nodejs/tsc - wdyt the severity of this should be? Should we backport this change to LTS lines?

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

@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 19, 2021
@ronag ronag mentioned this pull request Oct 19, 2021
Merged
@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 19, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 19, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@targos
Copy link
Member

targos commented Oct 19, 2021

Doesn't it need a test?

@addaleax
Copy link
Member

wdyt the severity of this should be?

… semver-major? It’s a pretty big potential increase in memory usage for people who increase the buffer pool size but not the stream HWMs.

@mcollina mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 19, 2021
@mcollina
Copy link
Member

wdyt the severity of this should be?

… semver-major? It’s a pretty big potential increase in memory usage for people who increase the buffer pool size but not the stream HWMs.

Either major or minor with do-not-backport labels, i.e. I don't think this should land in a LTS line. I'm slightly for the minor with do-not-backports but I'm good either way.

I don't expect a significant increase in memory usage but I expect an improvement in throughput.

@ronag
Copy link
Member Author

ronag commented Oct 19, 2021

IMO semver-minor

@github-actions github-actions bot mentioned this pull request Nov 8, 2021
Open
46 tasks
@mcollina mcollina added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 8, 2021
This was referenced Nov 9, 2021
Open
Open
@ronag ronag requested a review from ChALkeR November 10, 2021 09:50
@ronag ronag removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 10, 2021
@ronag
Copy link
Member Author

ronag commented Nov 10, 2021

needs tests

@ronag ronag added good first issue Issues that are suitable for first-time contributors. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. labels Nov 19, 2021
@ronag ronag removed the good first issue Issues that are suitable for first-time contributors. label Nov 19, 2021
mscdex
mscdex reviewed Nov 19, 2021
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
@ronag @mscdex
Update doc/api/stream.md
eb0c8c3 
Co-authored-by: mscdex <mscdex@users.noreply.github.com>
@mscdex
Copy link
Contributor

mscdex commented Nov 19, 2021

Missing documentation for Stream.defaultObjectModeHighWaterMark ?

@ronag ronag removed the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Nov 24, 2021
@ronag ronag marked this pull request as draft November 24, 2021 12:56
@ronag ronag closed this Jan 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@wa-Nadoo wa-Nadoo wa-Nadoo left review comments

@vweevers vweevers vweevers left review comments

@Mesteery Mesteery Mesteery left review comments

@BridgeAR BridgeAR Awaiting requested review from BridgeAR

@JungMinu JungMinu Awaiting requested review from JungMinu

@benjamingr benjamingr Awaiting requested review from benjamingr

@Trott Trott Awaiting requested review from Trott

@mcollina mcollina Awaiting requested review from mcollina

@ChALkeR ChALkeR Awaiting requested review from ChALkeR

Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer subsystem. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor 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.

None yet