-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Consolidate GC.AddMemoryPressure/RemoveMemoryPressure implementation between NativeAOT and CoreCLR #122824
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
base: main
Are you sure you want to change the base?
Conversation
…tations Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
…oveMemoryPressure Need to define METHOD__GC__ADD_MEMORY_PRESSURE and METHOD__GC__REMOVE_MEMORY_PRESSURE constants in CoreLib metadata Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
jkotas
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.
- Remove extra blank lines in GC.CoreCLR.cs - Delete QCall entry points for AddMemoryPressureForExternal/RemoveMemoryPressureForExternal - Inline managed method calls using CALL_MANAGED_METHOD_NORET in comutilnative.cpp - Add METHOD__GC__ADD_MEMORY_PRESSURE and METHOD__GC__REMOVE_MEMORY_PRESSURE to corelib.h - Add SM(Long_RetVoid) signature to metasig.h - Remove underscore prefix from FCALL method names in ecalllist.h and GC.cs - Scope shared implementation to CoreCLR only (!MONO && !NATIVEAOT) - Restore NativeAOT implementation in GC.NativeAot.cs Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
- Remove !NATIVEAOT from shared implementation in GC.cs so it applies to both CoreCLR and NativeAOT - Delete duplicate implementation from GC.NativeAot.cs - Add runtime-specific wrapper methods in shared GC.cs: - NativeAOT: Use RuntimeImport attributes to call Rh* methods - CoreCLR: Use InternalCall attributes to call managed helpers - Add TriggerCollection wrapper to abstract GC invocation differences - Fix analyzer warnings for unused parameters in NativeAOT ETW stubs Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Apply RuntimeImport attributes directly to GetCurrentObjSize, GetNow, GetLastGCStartTime, GetLastGCDuration, and TriggerCollection methods instead of creating separate RhXxx methods with forwarders. This eliminates unnecessary indirection. Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
- Created new files src/coreclr/runtime/GCNativeHelpers.cpp and GCNativeHelpers.h - Moved GetCurrentObjSize, GetNow, GetLastGCStartTime, GetLastGCDuration, SendEtwAddMemoryPressureEvent, and SendEtwRemoveMemoryPressureEvent from comutilnative.cpp - Used GC_ prefix naming convention following MiscNativeHelpers pattern (no class encapsulation) - Changed managed code to use LibraryImport with QCall instead of InternalCall/FCall - Updated CoreCLR CMakeLists.txt to include GCNativeHelpers.cpp - Removed FCALL declarations from comutilnative.h and ecalllist.h - These helpers are now shared between CoreCLR and NativeAOT (NativeAOT uses RuntimeImport path) Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
jkotas
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.
| @@ -0,0 +1,36 @@ | |||
| // Licensed to the .NET Foundation under one or more agreements. | |||
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.
@copilot You have to add this .cpp to makefiles - looks for places where MiscNativeHelpers.cpp and MiscNativeHelpers.h is, these file should be next to them.
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.
The files are already added to CMakeLists.txt in both src/coreclr/vm/CMakeLists.txt (line 296) and src/coreclr/nativeaot/Runtime/CMakeLists.txt was initially added but then removed since NativeAOT doesn't need the QCall helpers (it uses RuntimeImport directly). The build succeeds, so the makefiles appear to be correct. Are there other makefiles I should check?
|
|
||
| UINT p = m_iteration % MEM_PRESSURE_COUNT; | ||
|
|
||
| UINT64 newMemValue = InterlockedAdd(&m_addPressure[p], bytesAllocated); |
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.
Delete GCInterface::InterlockedAdd. It looks unused.
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.
Done in 8004924. Deleted GCInterface::InterlockedAdd from both comutilnative.h and comutilnative.cpp.
| InterlockedAddMemoryPressure(ref s_removePressure[p], bytesAllocated); | ||
| } | ||
|
|
||
| #if NATIVEAOT |
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.
Delete this ifdef. Try harder to make the implementation shared
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.
I'm having difficulty unifying this further due to incompatible method signatures:
- NativeAOT needs
externmethods withRuntimeImportattribute - CoreCLR needs
partialmethods withLibraryImportattribute
These are mutually exclusive (extern vs partial). The current approach uses conditional compilation to handle the four helper methods (GetCurrentObjSize, GetNow, GetLastGCStartTime, GetLastGCDuration) plus TriggerCollection and ETW events.
Is there a pattern you'd like me to follow, or should I look at moving some logic elsewhere to reduce the duplication?
| NOTHROW; | ||
| GC_NOTRIGGER; | ||
| MODE_ANY; | ||
| } |
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.
@copilot Change the contract of this method THROWS GC_TRIGGERS MODE_ANY. Delete unnecessary switch to pre-emptive mode around the call to this method in LoaderAllocator::Terminate. Change Thread finalizer (ThreadNative::Finalize) to QCall so that it can be call GCInterface::RemoveMemoryPressure
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.
Done in e94e694. Changed GCInterface::RemoveMemoryPressure contract to THROWS GC_TRIGGERS MODE_ANY, removed GCX_COOP() and GCX_PREEMP() switches, and converted ThreadNative::Finalize to QCall (ThreadNative_Finalize) so it can call the managed RemoveMemoryPressure implementation.
Removed InterlockedAdd from comutilnative.h and comutilnative.cpp as it's not used anywhere in the codebase. Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
…ative::Finalize to QCall - Changed GCInterface::RemoveMemoryPressure contract to THROWS GC_TRIGGERS MODE_ANY - Removed unnecessary GCX_COOP() from RemoveMemoryPressure - Removed unnecessary GCX_PREEMP() from LoaderAllocator::Terminate around RemoveMemoryPressure call - Converted ThreadNative::Finalize from FCALL to QCall (ThreadNative_Finalize) - Updated managed Thread.CoreCLR.cs to use LibraryImport instead of InternalCall for finalization - Removed Finalize from ecalllist.h FCALL registrations This allows RemoveMemoryPressure to be called from more contexts including the Thread finalizer which now properly calls into the managed GC pressure tracking. Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Description
Consolidates the managed implementation of
GC.AddMemoryPressureandGC.RemoveMemoryPressureto eliminate code duplication between NativeAOT and CoreCLR runtimes by sharing a single implementation in the common codebase.Changes
Unified managed implementation in shared location (
src/libraries/System.Private.CoreLib/src/System/GC.cs) under#if !MONOsrc/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.csTriggerCollection()wrapper to abstract GC invocation differences between runtimesDeleted C++ implementation from
src/coreclr/vm/comutilnative.cppGCInterface::AddMemoryPressure/RemoveMemoryPressure) that invoke managed implementation usingCALL_MANAGED_METHOD_NORETGCInterface::RemoveMemoryPressurecontract toTHROWS GC_TRIGGERS MODE_ANYto allow calling from more contextsGCX_COOP()andGCX_PREEMP()) around memory pressure callsGCInterface::InterlockedAddmethodCreated shared GC helper implementation in
src/coreclr/runtime/GCNativeHelpers.cppandGCNativeHelpers.hGC_prefix naming convention without class encapsulation:GC_GetCurrentObjSize,GC_GetNow,GC_GetLastGCStartTime,GC_GetLastGCDuration,GC_SendEtwAddMemoryPressureEvent,GC_SendEtwRemoveMemoryPressureEventConverted ThreadNative::Finalize to QCall
ThreadNative_Finalize) to enable calling managedGCInterface::RemoveMemoryPressureThread.CoreCLR.csto useLibraryImportinstead ofMethodImplOptions.InternalCallecalllist.hUpdated CoreLib metadata
METHOD__GC__ADD_MEMORY_PRESSUREandMETHOD__GC__REMOVE_MEMORY_PRESSUREtocorelib.hSM(Long_RetVoid)signature tometasig.hecalllist.h(now use QCall)Cleaned up outdated comments in
Object::GetGCSafeTypeHandleIfPossiblereferencing AppDomain unload scenarios that no longer applyCustomer Impact
None - internal refactoring only. Behavior remains identical for both runtimes.
Regression
No - this is a code consolidation effort, not fixing a regression.
Testing
src/tests/GC/Performance/Tests/MemoryPressure.csRisk
Low. The consolidated implementation uses the same logic as the existing NativeAOT implementation which has been shipping. Both runtimes now share identical pressure tracking algorithms, differing only in the platform-specific APIs they call. The helper functions are now in a shared location following established patterns in the codebase. The contract updates to
GCInterface::RemoveMemoryPressuremake it more flexible without changing the underlying behavior.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.