-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Fix computing available memory on OSX for GC #97102
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
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
Maoni0
left a comment
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_countas 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