-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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 computing available memory on OSX for GC #97102
Conversation
We were using the `vm_statistics_data_t::free_count` as the available memory reported to the GC. It turned out this value is only a small portion of the available memory and that the appropriate value should be based on the kern.memorystatus_level value obtained using sysctl. That value represents percentual amount of available memory, so multiplying it by the total memory bytes gets the available memory bytes. Close dotnet#94846
Tagging subscribers to this area: @dotnet/gc Issue DetailsWe were using the Close #94846
|
src/coreclr/gc/unix/gcenv.unix.cpp
Outdated
int rc = sysctlnametomib(mem_free_name, NULL, &g_kern_memorystatus_level_mib_length); | ||
if (rc == 0) | ||
{ | ||
g_kern_memorystatus_level_mib = (int*)malloc(g_kern_memorystatus_level_mib_length * sizeof(int)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
presumably if this returns null then sysctlnametomib
would return a failure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for spotting the missing check, fixed.
src/coreclr/gc/unix/gcenv.unix.cpp
Outdated
if (KERN_SUCCESS == host_page_size(mach_port, &page_size)) | ||
uint32_t mem_free = 0; | ||
size_t mem_free_length = sizeof(uint32_t); | ||
if (g_kern_memorystatus_level_mib != NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if GCToOSInterface::Initialize
succeeded, this should always be non null, correct? if so we can just assert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
src/coreclr/gc/unix/gcenv.unix.cpp
Outdated
int64_t mem_size = 0; | ||
size_t mem_size_length = sizeof(int64_t); | ||
int mib[] = { CTL_HW, HW_MEMSIZE }; | ||
rc = sysctl(mib, 2, &mem_size, &mem_size_length, NULL, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to get the total memory every time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I've moved it to the init and also unified it with other total memory computations we had above.
31809d5
to
a5bf5df
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
We were using the
vm_statistics_data_t::free_count
as the available memory reported to the GC. It turned out this value is only a small portion of the available memory and that the appropriate value should be based on the kern.memorystatus_level value obtained using sysctl. That value represents percentual amount of available memory, so multiplying it by the total memory bytes gets the available memory bytes.Close #94846