Increase buffer pool size and streams high water marks

[BridgeAR]

Our Buffer pool size is currently set to 8kb. This seems pretty low for most hardware used these days.

Similar with our default high water marks in streams (16kb for all streams besides fs readable streams that are set to 64kb).

Increasing the buffer size allows less allocations to be used in general and increasing the default high water mark reduces the number of chunks which improves the throughout a lot.

I would like to increase the buffer pool size to 128kb. The regular high water mark could increase to 64kb and the high water mark for fs to e.g., 256kb.

Those numbers are all "magical" numbers but they seem better defaults than the current ones. Please let me know if there are reasons to either not change the defaults or if you have an alternative suggestion.

It is also possible to change the Buffer's pool size default during runtime and to set each high water mark per stream.

@nodejs/fs @nodejs/streams @nodejs/buffer PTAL

[addaleax]

Our Buffer pool size is currently set to 8kb. This seems pretty low for most hardware used these days.

The issue with that is that keeping any slice of a buffer pool alive keeps the entire pool alive. I wouldn’t increase this, but rather work towards ultimately eliminating the pool altogether.

Please let me know if there are reasons to either not change the defaults or if you have an alternative suggestion.

I mean, I guess in a way it’s obvious, but it has the potential to increase memory usage of Node.js processes a lot. I would be careful about this – our benchmarks are exclusively focused on measuring CPU time, not RSS or similar metrics, so it’s much easier to see the upsides than to see the downsides.

[BridgeAR]

@addaleax

The issue with that is that keeping any slice of a buffer pool alive keeps the entire pool alive. I wouldn’t increase this, but rather work towards ultimately eliminating the pool altogether.

I am aware about this downside. But allocating more memory than needed and then using that in case smaller chunks are required improves the performance in these cases quite a bit. I doubt that reserving 128kb does any harm.

It would of course just block memory that could otherwise be used in case some one does not use Buffer at all but that should be almost impossible, since we also internally use Buffer quite a bit and it's heavily used throughout the ecosystem (instead of directly using Uint8Array).

[...] it has the potential to increase memory usage of Node.js processes a lot. I would be careful about this – our benchmarks are exclusively focused on measuring CPU time, not RSS or similar metrics, so it’s much easier to see the upsides than to see the downsides.

Increasing the high water marks could actually also result in less average memory usage for users that heavily rely upon streams. The reason for that is that the throughput for the individual stream would be much higher and it would therefore end earlier and the application would end up with less streams running concurrently.

For fs it would also significantly reduce the amount of system calls which would be necessary to read the file. See #27063 and #25741 for the impact.

[addaleax]

But allocating more memory than needed and then using that in case smaller chunks are required improves the performance in these cases quite a bit. I doubt that reserving 128kb does any harm.

I don’t quite see why it doesn’t do any harm? It certainly has the potential to do so – Using 128kb as the Buffer pool size, means that a single Buffer, which may be much smaller than the pool size, can retain 128kb which are not going to be re-used. I know that Buffer.allocUnsafeSlow() is there to address this issue in general, but practically speaking, people don’t use it when Buffer.alloc() or Buffer.allocUnsafe() currently does the job just fine.

[...] it has the potential to increase memory usage of Node.js processes a lot. I would be careful about this – our benchmarks are exclusively focused on measuring CPU time, not RSS or similar metrics, so it’s much easier to see the upsides than to see the downsides.

Increasing the high water marks could actually also result in less average memory usage for users that heavily rely upon streams. The reason for that is that the throughput for the individual stream would be much higher and it would therefore end earlier and the application would end up with less streams running concurrently.

That’s true, but we have to be aware that when we’re doing this, the trade-off we’re making is lower average memory usage for higher maximum memory usage.

Either way, I’m not opposed to these changes, but I personally feel that it would be get to get some metrics around these. Maybe we could first put the increased values behind a runtime flag (either an experimental-style flag or a flag that configures some of the limits), so that people could report their experiences?

[mcollina]

I think adding a flag to configure these would be a very good first step. We can then let people experiment, and some good pattern will arise.

I’m also ok to have some benchmark. Overall I’m happy to trade higher memory usage for less work on the CPU.

[lpinca]

I guess it depends on the actual hardware and application. Think of a Raspberry Pi for example or a very high number of persistent network connections like net.Socket. In these cases increasing the buffer size does not seem a good idea.

our benchmarks are exclusively focused on measuring CPU time, not RSS or similar metrics, so it’s much easier to see the upsides than to see the downsides

I totally agree with this and Node.js is already kinda bad on memory usage.

I'm -1 on changing defaults without tests that show that memory does not grow significantly.

[mcollina]

It largely depends on the use case. Adding the flag would be top.

[puzpuzpuz]

Hi guys.

I've just posted an issue that probably could help with the buffer part of the puzzle: #30611

With this API libraries will be able to use dedicated "pool" of a size they find optimal.