Halve the GC handles a cached RCW costs in ComWrappers - #133081
Sergio0694 wants to merge 7 commits into
Conversation
The cache stored a weak handle to the NativeObjectWrapper, which then held a second weak handle to the RCW itself. Every cached RCW therefore cost two GC handles, and finding one meant dereferencing both of them. The wrapper is reachable from the RCW it tracks, through the wrapper table, so a handle to the RCW keeps exactly the same entries alive as a handle to the wrapper did. The cache now stores a copy of the handle the wrapper already had, which halves the handles a cached RCW costs, takes the handle allocation out of the write lock, and removes an indirection from the lookup that every native to managed transition performs. Every handle in the cache belongs to the wrapper that created it, so the cache only ever drops entries and never frees them, and NativeObjectWrapper.Release removes its entry before freeing its handle, so an entry can never name a handle that has been freed. Entries are identified by the handle rather than by what it points at, because once an RCW is collected several dead entries are indistinguishable by their targets. An entry is resolved back to its wrapper through the wrapper table, so an RCW is registered there before its entry is published, which also closes a window where another thread could find an entry whose wrapper was about to be released. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Three gaps, all on paths the cache change touches: A rejected registration has to leave nothing behind for the COM instance it was rejected for, so that instance can still get a wrapper afterwards. A dead entry replaced by a later RCW belongs to a different wrapper than the one about to finalize, and that wrapper must remove only the entry it published. Publishing, finding and removing entries all have to hold up when several threads do them at once while collections and finalizers run underneath, which is what would surface an entry being read after its owner freed it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Three tests, covering paths that the existing suite left uncovered and that any change to how an RCW is registered has to keep working. Two of them pin down what happens when several threads miss the cache for one COM instance at the same time and all call CreateObject. They are held on a barrier so they are guaranteed to be racing rather than finding each other's entries. One covers an implementation that hands every caller the same object, which is what one with its own cache would do, and the other covers distinct objects, where the ones that lost have to be left exactly as they were: not registered, not throwing from TryGetComInstance or from a weak reference, and still usable as the wrapper for some other COM instance. The third covers an RCW with no finalizer. Every other test in this file uses a type that has one, and a wrapper allocates a second GC handle for those. Without a finalizer there is a single handle that tracks resurrection, and that is the handle the cache holds, so its entries go dead at a different point in a collection than the ones already covered. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolving a cache entry back to the wrapper tracking its RCW goes through the wrapper table, so the previous commit registered the RCW there while holding the bucket write lock. That table is a ConditionalWeakTable, and every registration is a new key, so every one of them takes the single lock covering the whole table. Holding a bucket lock across that put every bucket behind one process wide lock, which is exactly what partitioning the cache into buckets was meant to avoid. Creating RCWs on 32 threads went from 1003ns to 1276ns because of it. An entry is now reserved under the bucket lock and only carries the wrapper that reserved it until that wrapper has registered its RCW, which happens with no lock held. Threads that come across the entry during that window take the wrapper from it, and everything after the window resolves through the table as before. Reserving the slot up front keeps the property that the only part of publishing that can fail on its own, growing the dictionary, has already happened by the time it matters. Completing a reservation only overwrites one reference field of one entry and never changes the dictionary's layout, so it takes the read lock rather than the write lock and concurrent creations don't serialize on it. Lookups now read entries in place instead of copying them out, which more than pays for the entry having grown. Creating RCWs is 6 to 22% faster than before this series on 8 to 32 threads, and unchanged on one thread. Looking one up is 7 to 9% faster. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A reservation carries a strong reference to the wrapper that made it, so leaving one behind keeps that wrapper alive, and a wrapper that outlives its RCW can never be finalized, which is what removes the entry. Anything that threw between reserving an entry and completing it therefore stranded the reservation: the COM instance kept its native reference forever and every later lookup for it reported a miss. Registering the RCW can throw, so can registering it with the reference tracker, and both run in that window. The reservation is now dropped whether publishing went through or not, which is right either way. If another thread picked the wrapper up out of the entry and registered it, the entry is usable and should be usable. If nobody did, nothing refers to the wrapper any more, so it is finalized and takes the entry with it, which is how this recovered before the cache started sharing handles. An entry naming an RCW that never made it into the wrapper table can be observed for as long as it takes that wrapper to be finalized, so resolving one no longer asserts that it must be there and replaces the entry instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
An RCW is only resolvable back to its COM instance once its wrapper is in the wrapper table, and that registration cannot happen while a cache lock is held, so an entry is reserved rather than published outright and a reserved entry is not handed out. Nothing covered that. The nearest test reuses the same few COM instances, so after its first iteration there is no creation left to race and it only catches this by luck. The new one gives every round a fresh COM instance for all its threads to race over, which is the shape that hits it: it fails on main, where 'TryGetComInstance' comes up empty around 50 times in 8000, and passes here. The concurrent test that was already there uses CreateObjectFlags.None, so the wrapper it builds is not a reference tracker one and putting it in the tracker handle cache does nothing, leaving that registration untested under any concurrency at all. The second test runs the same race with tracker objects and then checks what the registration is for, that the wrapper is walked, by handing the native object a thousand managed objects and collecting. It hands every caller the one wrapper rather than one each, because ITrackerObjectWrapper's finalizer fails the run if its tracker object is still connected and the winner keeps it connected for as long as the test needs it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ntry Neither is a bug on main, and both pass there. They cover two shapes of the cache paths this series reworks that nothing exercised, so that a future change to how an entry is claimed cannot quietly break them. The first races callers that bring their own object to register against callers that ask for one to be created, over the same COM instance. Only one can win, every caller has to come back with it, and the objects that lost have to be left exactly as they were: not registered, and still usable as the wrapper for some other COM instance. The second rejects a registration for a COM instance whose entry is still there but whose RCW has been collected. The existing coverage rejects one for a COM instance the cache has never seen, which takes a different path, because an entry that is present has to be taken over rather than added. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
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. |
|
Tagging subscribers to this area: @dotnet/interop-contrib |
There was a problem hiding this comment.
🟡 Changes recommended
The reservation commit currently runs even on exceptional paths and can clear the “pending” marker without a confirmed wrapper-table registration, which risks returning an RCW that can’t round-trip via TryGetComInstance.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR reworks the ComWrappers RCW cache to store (and reuse) the wrapper-owned weak handle to the RCW, reducing per-entry GC handle usage and adjusting publication to use a “reservation then commit” flow to avoid global-table contention during cache operations.
Changes:
- Switch the RCW cache buckets from
WeakGCHandle<NativeObjectWrapper>entries to anEntryholdingWeakGCHandle<object>(proxy) plus aPendingWrapperreservation marker. - Update RCW publication to reserve under the bucket write lock, register the RCW in
s_nativeObjectWrapperTableoutside the bucket lock, then commit the reservation. - Add extensive COM interop concurrency/regression tests to cover races around reservation, registration, cleanup, and rejected registrations.
File summaries
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs | Refactors RCW cache storage/publication to reuse wrapper-owned proxy handles and introduce reservation/commit semantics. |
| src/tests/Interop/COM/ComWrappers/API/Program.cs | Adds new multi-threaded regression tests validating RCW cache correctness under races/GC/finalization. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| if (reserved) | ||
| { | ||
| // The reservation is dropped whether the registration went through or not, because leaving | ||
| // one behind would keep its wrapper alive, and a wrapper that outlives its RCW can never be | ||
| // finalized, which is what removes the entry. Dropping it is right either way: if some other | ||
| // thread picked the wrapper up and registered it, the entry is now usable, and if nobody did, | ||
| // nothing is left referring to the wrapper, so it is finalized and takes the entry with it. | ||
| _rcwCache.CommitProxyForComInstance(identity, nativeObjectWrapper); | ||
| } |
| // Read the entry in place rather than copying it out. It carries a reference as well as | ||
| // the handle now, and this runs on essentially every transition from native code, so the | ||
| // copy is worth avoiding. Holding the read lock is what makes the reference safe to use: | ||
| // it keeps out the writers that could move the entry. |
There was a problem hiding this comment.
There is a lot of addtional AI nonsense left in here. Please remove superfluous commentary such as "this runs on essentially every transition from native code, so the copy is worth avoiding". That statement only makes sense with accompanying numbers, which we also don't want in comments.
| // Only ever clear the reservation this wrapper made. Its RCW may have been collected while | ||
| // the registration ran, letting another wrapper take the entry over, and that one is still | ||
| // pending on its own registration. |
There was a problem hiding this comment.
This comment feels wrong. If the RCW may be the pending wrapper, then it's definitely alive as it's being kept alive in the PendingWrapper field.
| // create the RCW for, which is the shape that hits it: without the reservation this fails in the | ||
| // low tens out of these several thousand attempts, and on a build with it, never. | ||
| [ActiveIssue("Not supported on Mono", TestRuntimes.Mono)] | ||
| [Fact] |
There was a problem hiding this comment.
All of these multithreading tests need to be marked as ConditionalFact for when multithreading is supported.
| Assert.Equal(identities[slot], unknown); | ||
| Marshal.Release(unknown); | ||
|
|
||
| if ((i % 16) == index % 16) |
There was a problem hiding this comment.
Extract 16 to a constant alongside the other constants for this test?
| // At this point, actualProxy is the RCW object for the identity | ||
| // and actualWrapper is the NativeObjectWrapper that is in the RCW cache (if not unique) that associates the identity with actualProxy. | ||
| // Register the NativeObjectWrapper to handle lifetime tracking of the references to the COM object. | ||
| RegisterWrapperForObject(actualWrapper, actualProxy); |
There was a problem hiding this comment.
Can we keep the rest of the logic still in a separate method (adding new parameters as needed)? Makes it easier to follow the overall flow of the logic here.
| // covering the whole table, and holding a bucket lock across that would put every bucket behind it. | ||
| try | ||
| { | ||
| NativeObjectWrapper registeredWrapper = s_nativeObjectWrapperTable.GetOrAdd(actualProxy, actualWrapper); |
There was a problem hiding this comment.
If all of the optimization here is around not serializing on s_nativeObjectWrapperTable, could we instead do the following once #132490 is in:
- For objects that don't derive from
ComWrappersObject, they can hit the global lock ons_nativeObjectWrapperTable - For objects that do derive, we don't need to hit the table at all.
Then we could remove the "pending" concept (which I really don't like).
What this changes
The RCW cache stored a weak GC handle to the
NativeObjectWrapper, and the wrapper separately kept its own weak handle to the RCW. So every cached RCW cost two GC handles, and every lookup dereferenced both of them to get from a COM pointer to the RCW.The wrapper is already reachable from the RCW it tracks, through
s_nativeObjectWrapperTable, so a weak handle to the RCW keeps exactly the same entries alive as a weak handle to the wrapper did. The cache now stores a copy of the handle the wrapper already owns:That gets three things: one GC handle per cached RCW instead of two, no handle allocation inside the bucket write lock, and one less dereference on
FindProxyForComInstance, which runs on essentially every transition from native to managed code.Ownership rule
Every handle in the cache belongs to the wrapper that created it. The cache only ever drops entries, it never frees them, and
NativeObjectWrapper.Releaseremoves its entry before freeing its handle, so an entry can never name a handle that has been freed. Every read of a cached handle happens inside the bucket lock, andRemovetakes the write lock, so a reader can never be part way through using an entry when its owner frees it.Removeidentifies its entry by comparing the handle itself rather than what it points at, because once an RCW has been collected several dead entries are indistinguishable by their targets. Comparing handles is exact: at the moment a wrapper removes its entry its handle is still allocated, so no other live handle can share that value.Reserving an entry
An entry now only names the RCW, so whoever finds one resolves the wrapper through
s_nativeObjectWrapperTable, which means an RCW has to be in that table before its entry can be used. Registering it there cannot happen under the bucket lock.ConditionalWeakTablehas a single lock covering the whole table, and every registration is a new key, so holding a bucket lock across one would put every bucket behind one process wide lock. That is exactly what splitting the cache into buckets was meant to avoid, and measuring it showed creating RCWs on 32 threads going from 1003ns to 1276ns.So an entry is reserved under the bucket lock and carries the wrapper that reserved it until that wrapper has registered its RCW, which happens with no lock held:
Threads that come across the entry in that window take the wrapper straight from it, and everything after the window resolves through the table. Reserving the slot up front keeps the useful property that growing the dictionary, the only part of publishing that can fail on its own, has already happened by the time it matters.
Completing a reservation only overwrites one reference field of one entry and never changes the dictionary's layout, so it takes the read lock rather than the write lock and concurrent creations do not serialize on it. The lookup that can overlap it either still sees the reservation and reports a miss, sending its caller down a path that takes the write lock and resolves the entry properly, or sees it gone and hands out an RCW that is by then registered.
A reservation is dropped whether publishing went through or not. Leaving one behind would keep its wrapper alive, and a wrapper that outlives its RCW can never be finalized, which is what removes the entry, so anything that threw in that window would strand a native reference forever. Dropping it is right either way: if another thread picked the wrapper up out of the entry and registered it, the entry is usable, and if nobody did, nothing refers to the wrapper any more, so it is finalized and takes the entry with it.
This also closes a pre-existing race. Before, the entry was published before the registration, so if the registration failed, because the user's
CreateObjectreturned an object already registered for a different COM instance, the wrapper was released andNotSupportedExceptionthrown, while a concurrentFindProxyForComInstancecould already have handed out an RCW backed by that wrapper. A reserved entry is not handed out.It closes a second one that is easier to hit. An RCW is only resolvable once its wrapper is in the table, so on main an RCW can be handed back before it can be resolved. Racing 16 threads on one COM instance over 64000 attempts,
ComWrappers.TryGetComInstancecame up empty on an RCW thatGetOrCreateObjectForComInstancehad just returned 115 times on main, and never with the entry reserved.Results
Measured on Windows x64, 16 cores and 32 logical, against
mainat818ad35699c. Both sides are the same binaries run from two shared framework layouts that differ only inSystem.Private.CoreLib, alternating between them.Handles and memory, the handle count verified by reflecting on the live cache and the bytes measured over 200k live cached RCWs:
WeakGCHandle<NativeObjectWrapper>, allocated by the cacheThe entry carries a reference alongside the handle, which is 8 more bytes in each dictionary slot and works out at 11 bytes per cached RCW once the dictionary's spare capacity is counted. Against that, one fewer GC handle per cached RCW, which does not show up in those numbers because handles live in the handle table rather than the GC heap, and one fewer handle for every GC to scan.
Plain COM interop, no WinRT involved. 10th percentile over 80 samples per side, because these are latency samples where the low end is the signal and the long tail is scheduling noise:
Looking one up on 32 threads is left out because it cannot be measured on this machine. Saturating all 32 logical processors with lookups puts the samples between 20ns and 90ns, and the baseline's own 10th percentile moved from 21ns to 30ns between runs, which is more than the effect being measured. Separate runs put it anywhere from 15% faster to 26% slower, so there is no honest number to report for it.
Single threaded creation is the one scenario that gives something up, and it is the cost of completing a reservation. It does not show up in the CsWinRT numbers below, where creating an RCW also activates and queries interfaces, so a fixed cost of about 50ns is a much smaller share of the work.
Real world, through CsWinRT 3.0 from
microsoft/CsWinRTatstaging/3.0, driving a real C++/WinRT component. These benchmarks are all single threaded, so they show the creation and lookup improvements and none of the concurrency ones. Median of six alternating rounds:ObjectReturnPerf.NewSealedObjectProjectedConstructionPerf.ConstructProjectedClassWithIntObjectReturnPerf.NewUnsealedObjectObjectReturnPerf.NewDerivedAsBaseProjectedConstructionPerf.ConstructProjectedClassWithInterfaceProjectedConstructionPerf.ConstructDerivedFastAbiProjectedClassWithIntProjectedConstructionPerf.ConstructFastAbiProjectedClassWithIntObjectReturnPerf.ExistingUnsealedObject, a cache hitObjectReturnPerf.ExistingSealedObject, a cache hitProjectedConstructionPerf.ConstructProjectedClassWithStringThe last row spends most of its time in string marshalling rather than in the RCW, which is why it does not move.
The 31
QueryInterfacePerfbenchmarks tell the same story over three rounds: the twelve that construct something improve by 1 to 4%, and the ones that never touch the cache are flat, withQueryDefaultInterfaceat 7.6ns to 7.6ns,QuerySDKNonDefaultInterfaceat 27.0ns to 27.3ns andStaticPropertyCallat 21.9ns to 22.0ns. Nothing regressed outside noise there, the largest being 3% on a 7.8ns benchmark.Allocated bytes per operation are unchanged throughout, as GC handles are not managed allocations. The win is one less handle for the GC to scan per cached RCW, plus the handle allocation no longer happening while the bucket write lock is held.
Testing
All 31 test suites under
src/tests/Interop/COMpass against a Checked runtime, the ComWrappers ones run repeatedly.Ten tests added, for gaps on paths this change touches:
Registering a caller supplied object raced against creating one, over the same COM instance. Only one can win, every caller has to come back with it, and the objects that lost have to be left exactly as they were. This one passes on main too; it is here so that a future change to how an entry is claimed cannot quietly break it.
A registration rejected for a COM instance whose entry is still present but whose RCW has been collected, which takes a different path from rejecting one for a COM instance the cache has never seen, because a present entry has to be taken over rather than added. Also passes on main.
An RCW has to be resolvable the moment it is handed back. Every round gives all its threads a fresh COM instance to race over, which is the shape that hits this, and it is the one test here that fails on main:
TryGetComInstancecomes up empty on an RCWGetOrCreateObjectForComInstancehas just returned around 50 times in 8000 there, and never here.The same race with tracker objects. The concurrent test below uses
CreateObjectFlags.None, so the wrapper it builds is not a reference tracker one and putting it in the tracker handle cache does nothing, which left that registration with no concurrent coverage at all. This one then checks what the registration is for, by handing the native object a thousand managed objects and collecting.A rejected registration has to leave nothing behind for the COM instance it was rejected for. Verified this one fails without the fix: removing the rollback leaves an unset handle in the cache and the next lookup for that instance throws
NullReferenceException.A dead entry replaced by a later RCW belongs to a different wrapper than the one about to finalize, and that wrapper must remove only the entry it published.
Publishing, finding and removing entries from several threads at once while collections and finalizers run underneath.
Several threads held inside
CreateObjecton a barrier, so they are all guaranteed to have missed the cache, then handed one shared object. Every thread has to come back with that object and it still has to round trip to the COM instance it was created for.The same race with a distinct object per thread. The ones that lost have to be left exactly as they were: not registered,
TryGetComInstancereturning false rather than throwing, usable as the target of a weak reference, and still usable as the wrapper for some other COM instance.An RCW with no finalizer. Every other test in that file uses a type that has one, and a wrapper allocates a second GC handle for those. Without a finalizer there is a single handle that tracks resurrection, and that is the handle the cache holds, so its entries go dead at a different point in a collection.
Note
Parts of this pull request description were generated with GitHub Copilot. All benchmark numbers in it were measured locally.