src: fix available memory best guess logic #44730
Conversation
GuessMemoryAvailableToTheProcess() assumed that the return value of uv_get_constrained_memory() is a reasonable upper bound. It doesn't have to be, it can be much larger than actual available memory.
nodejs-github-bot
added
c++
| // There may still be room for swap, but we will just leave it here. | ||
| return allowed - rss; | ||
| uint64_t free_memory = uv_get_free_memory(); | ||
| uint64_t constrained_memory = uv_get_constrained_memory(); |
There was a problem hiding this comment.
This method is supposed to guess the amount of memory that can still be allocated by the process, so we should subtract the rss from it here (I can't remember when if (allowed < rss) can be true but that branch is probably worth keeping, too).
There was a problem hiding this comment.
Oh, right. uv_get_constrained_memory() isn't really the right tool because that's the ceiling for the whole cgroup, not just the current process, but then again, I don't have a better suggestion either.
There was a problem hiding this comment.
I think uv_get_constrained_memory() - rss here is good enough as a guess, at least slightly better than uv_get_free_memory(), it doesn't have to be accurate anyway, as we are only using it to prevent performing risky operations. If we end up underestimating the risk, then it is what it is - most systems should just kill the process that attempts to allocate more memory than it has access to. Even if we guess the risk right and skip generating the heap snapshot, the process is probably heading to a OOM soon , which isn't that much better anyway.
There was a problem hiding this comment.
There's an open pull request (libuv/libuv#3754) that adds an API that's probably closer to what is needed here but I haven't had time to look at it in detail yet.
(And the libuv upgrade in #42340 has been stuck since March so there's that...)
|
Blocked on the arrival of uv_get_available_memory() in the next libuv upgrade. |
|
This needs a rebase. |
GuessMemoryAvailableToTheProcess() assumed that the return value of uv_get_constrained_memory() is a reasonable upper bound. It doesn't have to be, it can be much larger than actual available memory.
There's another uv_get_constrained_memory() call in src/api/environment.cc that looks mostly alright but it compares against total memory, not free memory.