stream: bump default highWaterMark

[ronag]

This should give a performance boost accross the board at the cost of slightly higher memory usage.

Given that the old limit is a decode old and memory capacity has doubled many times since I think it is appropriate to slightly bump the default limit.

[nodejs-github-bot]

Review requested:

• @nodejs/streams

[ronag]

TBH I would like to bump it even further to 128k but I don't think I will get consensus on that one.

[mcollina]

I've put in the "baking for lts" label to wait a bit to backport in case of issues.

FWIW I think bumping this to 128KB is acceptable. It would help reduce overhead.

cc @mscdex @cjihrig

[github-actions]

Failed to start CI

- Validating Jenkins credentials
✖  Jenkins credentials invalid

https://github.com/nodejs/node/actions/runs/4145265543

[mscdex]

-1 I don't think we should do this. Node runs on all kinds of setups, including more resource-constrained hardware. The current defaults work well memory usage-wise in either kind of setup.

I think a better solution would be to allow end users to set something like Stream.defaultObjectWaterMark/Stream.defaultWaterMark which would default to the current 16/16KB respectively.

This way users could just set this at the top of their main script and get the same effect if they want to take on that kind of potential extra memory consumption.

[lpinca]

My memory tells me that this was already proposed in the past but I can't find the PRs/issues. I kind of agree with @mscdex. Instead of changing the default for everyone, make it customizable.

[ronag]

The problem with that is that most users will not bother/dare/known how to set it. So we are slowing it down for everyone to be compatible with a few... not saying that's wrong, but is there any middle ground? Can we e.g. check the total available system memory and base it on that?

Messing around with hwm and Buffer.poolSize is kind of advanced user usage.

[mscdex]

The problem with that is that most users will not bother/dare/known how to set it.

The properties I proposed would be in the documentation, which I think developers would already be looking at (especially if they are at the point where the stream high water mark is actually becoming a bottleneck), so I don't see this as an issue.

So we are slowing it down for everyone to be compatible with a few... not saying that's wrong

It could be argued both ways. I don't think it's fair to say that everyone is running node in environments where memory usage is not a concern.

but is there any middle ground?

I believe what I proposed is a middle ground. Besides, we already employ similar user-tweakable limits throughout node core, so this would just be making things more consistent in that regard.

Can we e.g. check the total available system memory and base it on that?

I don't think there is any realistic way of doing something like this as that'd be making the assumption that the node process is the only thing running on the OS that is using any considerable amount of resources.

[benjamingr]

I agree there is a tradeoff here and nuance but I think we're currently too conservative and so agree with this change. Constrained systems can usually override this.

[LiviaMedeiros]

@ronag unrelated to contents of this PR but why the removal of needs-ci PRs that need a full CI run. label?
Correct me if I'm wrong but AFAIR it's a safety measure to prevent accidental merging of PRs with failing CI: https://github.com/nodejs/node-core-utils/blob/267dde0692d42e2482d133d4cccfcae13586ad89/lib/pr_checker.js#L429-L440
So this should be always kept (arguably with exception of doc-only PRs, trivial linting, etc.)

[BridgeAR]

Can we e.g. check the total available system memory and base it on that?

I don't think there is any realistic way of doing something like this as that'd be making the assumption that the node process is the only thing running on the OS that is using any considerable amount of resources.

Increasing the highWaterMark is going to finish the process faster and thus, could actually lead to less memory pressure for high load scenarios.

So we are slowing it down for everyone to be compatible with a few... not saying that's wrong

It could be argued both ways. I don't think it's fair to say that everyone is running node in environments where memory usage is not a concern.

Our defaults are always concern for debate and we will likely never have the correct default for everyone. What we should IMO strive for is a default that is best for most users, so that only a few have to change the them. Many users are not aware of these things and only check for them, in case they know, they have to be cautious (what I would expect for users running in an environment where memory is a concern).

Using a heuristic where we check for the available memory sounds like a good idea to me. That way we will likely have the right setting for more users than we have currently. One difficult part might be to define the thresholds and what memory to look at (overall memory vs. free memory). For now, I would just use process.constrainedMemory() and default to the lower default in case undefined is returned.

[vweevers]

Heuristics can't tell how many parallel streams will be created and what the intended purpose of the available memory is. I like that streams slow down and leave room.

I also prefer baby steps here. The effect of increasing a hardcoded default is relatively easy to measure and doesn't mess with user expectations that much, compared to heuristics. Increasing the hwm every so often (and gradually) can improve performance for a majority of users, while still giving others a chance to test it out (even unknowingly).

[ronag]

@ronag unrelated to contents of this PR but why the removal of needs-ci PRs that need a full CI run. label?

Correct me if I'm wrong but AFAIR it's a safety measure to prevent accidental merging of PRs with failing CI: https://github.com/nodejs/node-core-utils/blob/267dde0692d42e2482d133d4cccfcae13586ad89/lib/pr_checker.js#L429-L440

So this should be always kept (arguably with exception of doc-only PRs, trivial linting, etc.)

We don't land anything without CI. Don't worry. I just missed adding the request-ci label.

[lpinca]

This increases the HWM by eight times while leaving the default for object mode unchanged. It will increase memory usage by eight times on a proxy that pipes data to a slower destination. That is almost an order of magnitude.

FWIW we are using 64 KiB for file system streams.

[debadree25]

This would require a rebase I think? removed the author ready for the time being

[marco-ippolito]

for reference: #27121

[aduh95]

This needs a rebase.

[nodejs-github-bot]

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

[aduh95]

Quite a few CI failures that would need to be addressed.

[RafaelGSS]

@ronag do you need help with that? Would be great to have this on v21 proposal.

[ronag]

Feel free to take over.

[RafaelGSS]

@ronag Apparently, I don't have push permissions to this PR (nor nxtedition fork). I'll create another PR cherry-picking this commit.

[constantind]

FYI: In kubernetes with flannel, 256k and 512k only increase throughput on red hat linux 8 with defaults. any other values make no difference a value higher than 600k starts to drop throughput, see also test scenario streaming more than 200MB, the 16k is unable to get the TCP state machine working: https://github.com/orgs/nodejs/discussions/51082