-
Notifications
You must be signed in to change notification settings - Fork 4.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
[mono] Fix sgen_gc_info.memory_load_bytes #53364
Conversation
Set it to used memory size instead of available memory size
Tagging subscribers to this area: @BrzVlad Issue DetailsSet it to used memory size instead of available memory size
|
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.
Looks good, thanks for fixing this.
Let see how the CI build will look. I think we might need to update also the |
Let me try this tomorrow together with EventPipe and RuntimeEventSource since it presents a bunch of GC data to tools like dotnet-counters. |
For systems without information about available physical memory size Also handle -1 return values from sysconf calls
Looks like RuntimeEventSource won't be affected since this metric is currently not published as an EventSource counter. |
Did we check this property against coreclr GC? Just looking at the documentation won't tell what is actually returned, looking at coreclr GC implementation of the property it is calculated like this: *lastRecordedMemLoadBytes = (uint64_t) (((double)(last_gc_info->memory_load)) / 100 * gc_heap::total_physical_mem); where last_gc_info->memory_load seems to either be memory load on last GC enter/exit representing memory load on process/system, coming from GCToOSInterface::GetMemoryStatus that has the following comment, // Get memory status That calculation take into account if there are restrictions on amount of memory that can be used by GC. and gc_heap::total_physical_mem is set to: gc_heap::total_physical_mem = (size_t)GCConfig::GetGCTotalPhysicalMemory(); and that seems to return either total amount of physical memory or any restricted limitation setup. |
On Windows it looks like at least one coreclr GC code path (ignoring memory restrictions and limitations) will just fall through using dwMemoryLoad here, https://docs.microsoft.com/en-us/windows/win32/api/sysinfoapi/ns-sysinfoapi-memorystatusex, and total physical is ullTotalPhys from that same struct, so then it will just calculate percent used out of total physical memory and that should be total physical bytes used, similar to our mono_determine_physical_ram_size() - mono_determine_physical_ram_available_size (), but we don't go over using the memory load (in percent) reducing some complexity in the calculations. Not sure how well this works with heap size restrictions on Mono though. |
Setting a explicit max heap size will probably cause issues the way the calculation is currently done, since sgen_gc_info.total_available_memory_bytes is then set by user, but mono_determine_physical_ram_available_size will still reflect the amount of physical memory available on the system. |
Makes sense. I will change it to use |
That might not work very well either. Let me think more about it. |
Looks like CoreCLR uses different calculations when having heap size limitations, using process working set size in calculation, see |
I made it to scale the memory load by The |
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.
At some point in time we should probably do calculation against process working set when having a heap max limit instead of using system global memory state, but if current fix solves memory monitor issue I believe it will give us a good enough representation of this metric and way better than what we had (that was doing the opposite to what it should have done).
…asm_debugger_and_use_debugger_agent * upstream/main: (597 commits) Fix42292 (dotnet#52463) [mono] Remove some obsolete emscripten flags. (dotnet#53486) Fixed path to projects (dotnet#53435) support ServerCertificateContext in quic (dotnet#53175) Socket: delete unix local endpoint filename on Close (dotnet#52103) [mono] Fix sgen_gc_info.memory_load_bytes (dotnet#53364) Refactor MsQuic's native IP address types. (dotnet#53461) Re-enabled optimizations for gtFoldExprConst (dotnet#53347) Add diagnostic support into sample app and AppBuilders on Mono. (dotnet#53361) Fix issues with virtuals and mdarrays (dotnet#53400) Split Variant marshalling tests (dotnet#53035) Update clrjit.natvis to cover GT_SIMD and GT_HWINTRINSIC (dotnet#53470) remove WSL checks in tests (dotnet#53475) Always spawn message loop thread for SystemEvents (dotnet#53467) add AcceptAsync cancellation overloads (dotnet#53340) Remove unnecessary reference to iOS workload pack in the Mono workload (dotnet#53425) Add CookieContainer.GetAllCookies (dotnet#53441) Remove DynamicallyAccessedMembers on JsonSerializer (dotnet#53235) [wasm] Re-enable Wasm.Build.Tests (dotnet#53433) [libraries] Move library tests Feature Switches defaults to Functional tests (dotnet#53253) ...
Set it to used memory size instead of available memory size