Skip to content
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

Fix constraint detection in the GC #51656

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

gbaraldi
Copy link
Member

This wasn't respecting the documentation that Libuv has, which lead to weird results. This might have caused some issues with julia inside containers

@oscardssmith oscardssmith added performance Must go faster GC Garbage collector kind:bugfix This change fixes an existing bug backport 1.10 Change should be backported to the 1.10 release labels Oct 10, 2023
if (constrained_mem > 0 && constrained_mem < total_mem)
uint64_t constrained_mem = uv_get_constrained_memory(); // This returns != 0 for not constrained
int constrained = constrained_mem != 0 ? 1 : 0; // But it can return -1 if there is a constraint but no value
constrained_mem = (total_mem < constrained) ? total_mem : constrained_mem; // The constrained value can also be greater so take the min
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
constrained_mem = (total_mem < constrained) ? total_mem : constrained_mem; // The constrained value can also be greater so take the min
constrained_mem = constrained_mem * 4 / 3; // n.b. the user constraint might be larger than the `uv_get_total_memory` value. Try to fill no more than 75% of that constraint with GC pages.

Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know that this is exactly a "fix", since the behavior before was not wrong, but possibly also not intended. However, I don't think either is correct. The constraint is just the cgroup, it doesn't make sense to me to arbitrarily take the minimum with a different value and create an unnecessary constraint with that, which was neither present before nor requested by the user.

@gbaraldi
Copy link
Member Author

So my goal is to detect if we have a constraint, and if so respect it. Though if we have a constraint that doesn't have a set value (like GHA does) we probably want to follow the max physical memory at least (GHA does set it). Though we might want to make a bit fancier and do something like. If it's uintmax, then follow max physical, otherwise follow the constraint set?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Oct 11, 2023

We should probably respect RLIMIT_AS, which was the posix standardized way to set constraints, before cgroup2 invented its own convoluted mechanism of query, but the MemBalance paper proved that applying max physical memory as a limit guarantees Pareto-suboptimal performance and memory utilization both for the system as a whole and the application in particular, so that seems undesirable.

@gbaraldi
Copy link
Member Author

About the RLIMIT_AS, shouldn't the logic be in libuv?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Oct 17, 2023

About the RLIMIT_AS, shouldn't the logic be in libuv?

Sure. make a PR?

@gbaraldi gbaraldi removed the backport 1.10 Change should be backported to the 1.10 release label Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GC Garbage collector kind:bugfix This change fixes an existing bug performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants