-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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.
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
Increase buffer pool size and streams high water marks #27121
Comments
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 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. |
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
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 |
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
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? |
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. |
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
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. |
It largely depends on the use case. Adding the flag would be top. |
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. |
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
The text was updated successfully, but these errors were encountered: