Review requested:

• @nodejs/streams

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

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

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.

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.

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.

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

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.

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

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.

I'd be ok in doing baby steps with this.

How about we increase the highwatermarks to 64KB? I think matching file system streams is a safe choice.

Changed to 64k

@mscdex Are you still objecting?

Needs a test :-)

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

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.

I like this idea because it becomes an easy way to change the default, and we could adjust "default default" more easily in semver major changes.

Added test. @jasnell

@mscdex ping