Reduce the GC handle cost of every RCW in ComWrappers - #132040
Reduce the GC handle cost of every RCW in ComWrappers#132040Sergio0694 wants to merge 9 commits into
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR reduces per-RCW overhead in ComWrappers by avoiding an extra resurrection-tracking handle when the managed proxy type doesn’t require finalization, and by using strongly-typed WeakGCHandle<T> to streamline weak-handle operations. It also updates reference-tracker plumbing and tests to align with the new handle usage and (as noted in the description) includes the RCW-cache bucketing work it’s stacked on.
Changes:
- Switch
NativeObjectWrapperproxy handles toWeakGCHandle<object>and skip allocating the resurrection-tracking handle whenRuntimeHelpers.ObjectHasFinalizer(comProxy)is false. - Partition the RCW cache into per-processor buckets and use
WeakGCHandle<NativeObjectWrapper>for cached entries. - Update tracker/test call sites to use
TryGetTargetrather thanGCHandle.Target.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/Interop/COM/ComWrappers/API/Program.cs | Tightens the “no lock around QI” regression test to ensure the nested call targets the same COM instance and asserts completion. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/TrackerObjectManager.cs | Uses ProxyHandle.TryGetTarget when collecting proxies to release. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs | Core change: uses WeakGCHandle for proxy handles, conditionally allocates resurrection tracking, and partitions RCW cache into buckets. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs | Adds RuntimeHelpers.ObjectHasFinalizer (CoreCLR). |
| src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/TrackerObjectManager.NativeAot.cs | Updates NativeAOT reference-tracker callback plumbing to use WeakGCHandle<object>. |
| src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.NativeAot.cs | Adds RuntimeHelpers.ObjectHasFinalizer (NativeAOT). |
f61051e to
6ac4779
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/TrackerObjectManager.NativeAot.cs:248
RootObject.TryGetTargetcan fail for a weak handle. The current code ignores the return value and may passnullintoAddReferencePath, which expects non-nullobjectarguments and can throw during a GC callback. Bail out early when the source object is no longer available.
_ = ((FindReferenceTargetsCallback.Instance*)pThis)->RootObject.TryGetTarget(out object? sourceObject);
if (!TryGetObject(referenceTrackerTarget, out object? targetObject))
{
return HResults.S_FALSE;
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs:617
ComWrappersnow callsRuntimeHelpers.ObjectHasFinalizer, but that intrinsic is only added for CoreCLR and NativeAOT in this PR.RuntimeHelpers.Mono.csdoes not define it, so Mono builds will fail unless a Mono implementation/stub is added (even if COM interop is unsupported there).
if (RuntimeHelpers.ObjectHasFinalizer(comProxy))
{
_proxyHandleTrackingResurrection = new WeakGCHandle<object>(comProxy, trackResurrection: true);
}
// 'ObjectHasFinalizer' reads the MethodTable, which requires the object to be kept alive
GC.KeepAlive(comProxy);
src/tests/Interop/COM/ComWrappers/API/Program.cs:1123
- The worker thread is created as a foreground thread. If
Jointimes out (or the thread deadlocks), the test process can hang even after the assertion fails because the thread will keep the process alive. Mark the thread as background (or otherwise ensure it cannot outlive the test) to prevent hangs on failure paths.
Thread thread = new Thread(() =>
{
// Make sure that ComWrappers isn't locking in GetOrCreateObjectForComInstance
// around the QI call by calling it on a different thread from within a QI call to register a new managed wrapper
// for a COM object representing "this".
src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/TrackerObjectManager.NativeAot.cs:190
DetachNonPromotedObjectscurrently skips wrappers whose proxy weak handle has already been cleared (i.e.,TryGetTargetreturns false). Those objects are precisely the ones that are not promoted and should be detached/notified. Preserve the previous behavior by treating a missing target as "not promoted".
ReferenceTrackerNativeObjectWrapper? nativeObjectWrapper = Unsafe.As<ReferenceTrackerNativeObjectWrapper>(weakNativeObjectWrapperHandle.Target);
if (nativeObjectWrapper != null &&
nativeObjectWrapper.TrackerObject != IntPtr.Zero &&
nativeObjectWrapper.ProxyHandle.TryGetTarget(out object? proxyTarget) &&
!RuntimeImports.RhIsPromoted(proxyTarget))
|
Tagging subscribers to this area: @dotnet/interop-contrib |
|
/azp list |
|
/azp run runtime-coreclr gcstress0x3-gcstress0xc |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
I don't think this assumption is true. Some other object's finalizer could resurrect the proxy. I have a test in #132058 that passes on main but fails with this exception on this branch due to the |
Every RCW allocates two weak GC handles for its proxy: a plain weak one, and a second 'WeakTrackResurrection' one that exists only so the NativeObjectWrapper is cleaned up after the proxy's finalizer has run (the proxy's finalizer may access the native object). If the proxy's type declares no finalizer, it can never observe the native object once it becomes unreachable, and it can never be resurrected, so the second handle would always be cleared at the same time as the first. Detect that case from the MethodTable and skip allocating it. This is worth doing because allocating, clearing and freeing GC handles is a substantial part of the cost of every RCW, and resurrection tracking handles are more expensive for the GC to process than plain weak ones. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The handle always tracks the proxy object, so a strongly typed weak handle expresses that directly: it allocates through GCHandle.InternalAlloc without revalidating the handle type, and skips the cast when reading the target. Only this handle is converted. The '_proxyHandle' field and the handles in 'GCHandleSet' are mirrored in native code (see 'NativeObjectWrapperObject' in interoplibinterface_comwrappers.h, read via 'GetProxyHandle') and flow through the on-stack COM struct used for reference tracker callbacks, so converting those is a wider change that needs to be validated against the native side. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Both handles tracking the proxy object are now strongly typed weak handles. They allocate through GCHandle.InternalAlloc without revalidating the handle type, and skip the cast when reading the target. The layout is unchanged, so the native mirror of 'NativeObjectWrapper' still matches: 'GCHandle' only alters the stored value for pinned handles, so a weak handle holds the raw handle in both representations. This also applies to the on-stack COM struct used for reference tracker callbacks, which passes the handle straight through to native code. The redundant 'IsAllocated' checks around the disposal calls are also dropped, as 'WeakGCHandle<T>.Dispose' already handles a default handle. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Skipping the resurrection tracking handle for an RCW whose type declares no finalizer was wrong. Such an RCW never becomes eligible for finalization on its own account, but something else's finalizer can hold on to it and resurrect it, and then this wrapper would already have torn down state the RCW still needs. The two handles only disagree while the RCW is unreachable but not yet collected. An RCW that declares no finalizer only reaches that state by way of someone else holding it, and in that case reporting it as alive is both what keeps this wrapper from releasing too early and the honest answer, as it may be about to become reachable again. So rather than dropping the second handle, let the proxy handle track resurrection and drop the separate one, which costs the same single handle per RCW as before while behaving correctly. An RCW that does declare a finalizer does reach that state by itself, and there the two meanings genuinely differ, so it keeps both handles. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
6ac4779 to
869fe82
Compare
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
@AustinWise I've ported your new test here and updated the PR to keep the test passing, good find! 😄 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs:624
RuntimeHelpers.ObjectHasFinalizeris used here, but this helper is only added in the CoreCLR and NativeAOTRuntimeHelperspartials in this PR.System.Private.CoreLibfor Mono imports the sharedComWrappers.cs(viaSystem.Private.CoreLib.Shared.projitems), so Mono builds will fail unlessRuntimeHelpers.Monoalso providesObjectHasFinalizer(or this call is guarded appropriately).
bool proxyHasFinalizer = RuntimeHelpers.ObjectHasFinalizer(comProxy);
_proxyHandle = new WeakGCHandle<object>(comProxy, trackResurrection: !proxyHasFinalizer);
if (proxyHasFinalizer)
{
_proxyHandleTrackingResurrection = new WeakGCHandle<object>(comProxy, trackResurrection: true);
}
// 'ObjectHasFinalizer' reads the MethodTable, which requires the object to be kept alive
GC.KeepAlive(comProxy);
src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/TrackerObjectManager.NativeAot.cs:244
RootObject.TryGetTarget's return value is ignored, sosourceObjectcan be null (orTryGetTargetcan throw if the handle is uninitialized). That null then flows intoAddReferencePath(sourceObject, ...), which expects a non-nullobjectand is reached from an[UnmanagedCallersOnly]entrypoint. This should early-returnS_FALSEif the root object isn't available.
_ = ((FindReferenceTargetsCallback.Instance*)pThis)->RootObject.TryGetTarget(out object? sourceObject);
|
/azp run runtime-coreclr gcstress0x3-gcstress0xc |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
- Say that ObjectHasFinalizer covers an inherited finalizer too, rather than only one the type declares, which is what the underlying flag means on both runtimes. - Drop AggressiveInlining from it. The JIT inlines it into the wrapper constructor either way, so the attribute wasn't buying anything. - Assert that the reference tracker callback's root object is still set, and make the nullability explicit at the point it is passed on. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs:455
RuntimeHelpers.ObjectHasFinalizeris a tiny helper that’s called on a hot path (RCW construction).ObjectHasComponentSizeis markedAggressiveInlining, but this new helper isn’t, which makes it more likely to stay as a call and add overhead at scale. Consider applying the same inlining hint for consistency and to keep the per-RCW cost minimal.
internal static unsafe bool ObjectHasFinalizer(object obj)
src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.NativeAot.cs:219
RuntimeHelpers.ObjectHasFinalizeris analogous in shape/usage toObjectHasComponentSize, but unlike that helper it isn’t markedAggressiveInlining. Since this is used during RCW construction, keeping it inlined helps minimize overhead on a hot path and keeps the two helpers consistent.
internal static unsafe bool ObjectHasFinalizer(object obj)
src/tests/Interop/COM/ComWrappers/API/Program.cs:695
resurrecter.UnmanagedWrapperis treated as non-null (it’s passed toTryGetComInstanceand thenFinalRelease()is called). If it ever becomes null unexpectedly, this will fail with a less-informative NRE/arg-null. Adding an explicit assertion (and using null-forgiving where needed) makes failures clearer and avoids nullable-flow warnings.
Assert.True(handle.TryGetTarget(out Resurrecter resurrecter));
ManualReleaseITestObjectWrapper? unmanagedWrapper = resurrecter.UnmanagedWrapper;
Assert.NotNull(resurrecter);
Assert.True(ComWrappers.TryGetComInstance(unmanagedWrapper, out IntPtr unmanagedObjOther));
Assert.Equal(unmanagedObj, unmanagedObjOther);
Reading the MethodTable requires the object to stay alive, and having every caller remember that is easy to get wrong. Do it in the helper instead, the way GetMultiDimensionalArrayRank already does, and drop the call at the one use site. The generated code for the wrapper constructor is unchanged: same 353 bytes, same 102 instructions, and an identical instruction stream. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/tests/Interop/COM/ComWrappers/API/Program.cs:691
AssertNativeObjectWrapperAlivetreatsresurrecter.UnmanagedWrapperas nullable but then uses it without asserting non-null, while also assertingresurrecternon-null redundantly (it’s anoutvalue fromTryGetTarget). Capture the wrapper viaAssert.NotNull(...)so the intent is explicit and the subsequent calls are safe.
Assert.True(handle.TryGetTarget(out Resurrecter resurrecter));
ManualReleaseITestObjectWrapper? unmanagedWrapper = resurrecter.UnmanagedWrapper;
Assert.NotNull(resurrecter);
Assert.True(ComWrappers.TryGetComInstance(unmanagedWrapper, out IntPtr unmanagedObjOther));
src/tests/Interop/COM/ComWrappers/API/Program.cs:633
Resurrecteris declared using a parameterless primary constructor (class Resurrecter()), which is unusual in this test suite and adds an unnecessary dependency on that syntax. Use a normal class declaration to keep the test style consistent and avoid confusion about constructor semantics.
This issue also appears on line 688 of the same file.
class Resurrecter()
NativeAOT has no collectible types, so the MethodTable can't go away while the call is in progress and there is nothing to keep alive. This matches ObjectHasComponentSize right above it, which doesn't have one either. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/tests/Interop/COM/ComWrappers/API/Program.cs:692
Assert.NotNull(resurrecter);is redundant (the local is non-null) and the code doesn’t assert thatresurrecter.UnmanagedWrapperis non-null before using it. That weakens the test intent and can turn failures into a NullReferenceException atTryGetComInstance/FinalReleaseinstead of a clear assertion failure.
Assert.True(handle.TryGetTarget(out Resurrecter resurrecter));
ManualReleaseITestObjectWrapper? unmanagedWrapper = resurrecter.UnmanagedWrapper;
Assert.NotNull(resurrecter);
Assert.True(ComWrappers.TryGetComInstance(unmanagedWrapper, out IntPtr unmanagedObjOther));
Assert.Equal(unmanagedObj, unmanagedObjOther);
Motivation
Every RCW that
ComWrapperscreates allocates two weak GC handles for its proxy object:The second one exists so that the
NativeObjectWrapperreleases only after the proxy has actually been collected, because the proxy may still call into the native object while finalizing. Until then other callers, notably the reference tracker runtime, must see the proxy as dead as soon as it is eligible for finalization, which is what the first, plain weak handle provides.Allocating, clearing and freeing GC handles is a substantial part of the per-RCW cost. In a NativeAOT profile of CsWinRT,
HndCreateHandle/TableAllocSingleHandleFromCacheare prominent on the allocating thread, andQuickSort/CompareHandlesByFreeOrder— the GC handle table sorting free handles — account for roughly 35% of the finalizer thread.WeakTrackResurrectionhandles are also more expensive for the GC than plain weak handles, since they are processed in a separate, later phase.What this changes
1. One handle per RCW instead of two, when the proxy declares no finalizer
The two handles only ever disagree in one window: while the RCW is unreachable but not yet collected. An RCW whose type declares no finalizer never enters that window on its own account, so a single handle can serve both purposes.
The subtlety is that it can still be put in that window by something else: another object's finalizer may hold a reference to the RCW, and resurrect it. So the handle that is kept has to be the one that tracks resurrection — the proxy handle is allocated as
WeakTrackResurrectionand the separate handle is dropped, rather than the other way around. That keeps the wrapper from tearing down state a resurrected RCW still needs, and reporting such an RCW as alive is also the honest answer, since it may be about to become reachable again.An RCW that does declare a finalizer reaches that window by itself, and there the two meanings genuinely differ, so it keeps both handles.
Whether the proxy declares a finalizer is read straight off the
MethodTable, via a new internalRuntimeHelpers.ObjectHasFinalizeron each runtime — modelled on the existingRuntimeHelpers.ObjectHasComponentSize, which has the same shape and the same "callers are required to keep obj alive" contract.No public API change, and no opt-in required from callers: this applies automatically to any
ComWrappersuser whose proxies have no finalizer. That includes CsWinRT, whoseWindowsRuntimeObject(the object it passes toGetOrRegisterObjectForComInstance) has no finalizer — the finalizer it needs lives on a separateWindowsRuntimeObjectReferenceobject.Note that this does not remove a GC cycle for such wrappers:
GC.ReRegisterForFinalizewould not have fired anyway, sinces_nativeObjectWrapperTablekeeps the wrapper alive while the proxy is alive. The saving is the handle itself.2. Use
WeakGCHandle<T>for both proxy handlesBoth handles always track the proxy object, so a strongly typed weak handle expresses that directly: it allocates through
GCHandle.InternalAllocwithout revalidating the handle type, and skips the cast when reading the target.This is safe even though
_proxyHandleis mirrored in native code (NativeObjectWrapperObjectininteroplibinterface_comwrappers.h, read viaGetProxyHandle) and flows through the on-stack COM struct used for reference tracker callbacks (FindReferenceTargetsCallback.Instance.RootObject).GCHandleonly alters the stored value for pinned handles:so a weak
GCHandleand aWeakGCHandle<T>hold bit-identical raw handles, and the layout is unchanged. Native code only ever treats it as an opaqueOBJECTHANDLE, so the switch between the two weak handle kinds is invisible there.The redundant
IsAllocatedchecks around disposal are also dropped, sinceWeakGCHandle<T>.Disposealready handles a default handle.Testing
This includes @AustinWise's test from #132058, which covers exactly the case above: an RCW with no finalizer of its own, held and resurrected by another object's finalizer. An earlier revision of this PR dropped the resurrection tracking handle outright, and that test is what showed it to be wrong.
ComWrappersTests,ComWrappersTestsBuiltInComDisabled,GcRestrictedCalloutReversePInvokeandWeakReferenceTestall pass against the modified CoreLib, built Checked so theCoreLibBinderlayout checks and theDebug.Asserts are live.mainfor both proxy shapes, since the cache asks the proxy handle for its target and that handle's kind has changed.Benchmark results
Measured on a 32-core Windows x64 machine, Release runtime, using the CsWinRT benchmark suite (
ProjectedConstructionPerf) against both CsWinRT 2.3.0-prerelease and 3.0.0-preview on the same runtime build. Mean of 2 runs, in µs.mainhere isc99188c2f97, so it already includes #132033.CsWinRT 2.x
CsWinRT 3.0
So roughly 9–13% off object construction on top of what #132033 already landed.
ConstructProjectedClassWithStringbarely moves because it is dominated by HSTRING marshalling.Collection counts are unchanged within noise, which is expected: this removes a handle rather than an allocation, and the Gen2 reduction previously reported for this work came from #132033, which is now in
main.Note
Parts of this pull request description were generated with GitHub Copilot. All benchmark numbers in it were measured locally.