Deduplicate "all managed object wrappers" mapping - #133260
Merged
Merged
Conversation
Prevent duplicate entries in the "all managed object wrappers" table. Fixes #129191 For the case in #129191, there is always only one ComWrappers instance, so this collection will be length 1. I chose to do a simple fix like this for the following reasons: 1. This collection is read from the DAC and cDAC directly to support the `!DumpObj` command. Any significant changes to the structure (such as moving to a `HashSet<ManagedObjectWrapperHolder>`) would require very large changes to enable enumerating the structure from DAC or cDAC code, creating a higher risk change for backporting. 2. In practice, objects are passed to unmanaged code via 1 ComWrappers instance in the vast majority of cases outside of our own tests. In the unlikely case it's passed via more than 1, it would be via 2, the CsWinRT ComWrappers instance and the StrategyBasedComWrappers default instance. 3. I considered adding a parallel HashSet for deduplication. However, that felt like a very large memory cost for the common scenario (.NET object passed to native via only one ComWrappers instance) for a minor savings in a very unlikely scenario (many ComWrappers for the same .NET object).
Contributor
|
Tagging subscribers to this area: @dotnet/area-system-runtime |
|
Azure Pipelines: Successfully started running 2 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. |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The change is small, localized to diagnostics registration, and avoids unbounded list growth while keeping the existing DAC/cDAC-observable structure intact.
Review tier: Lite
Findings: None
What changed in this PR
Prevents duplicate ManagedObjectWrapperHolder entries from being recorded in the global “all managed object wrappers” ConditionalWeakTable, which is used by diagnostics tooling to enumerate managed object wrappers.
Changes:
- Adds a guarded insert (
Containscheck) when registering a managed object wrapper for diagnostics, preventing repeated additions of the sameManagedObjectWrapperHolderto the per-object list.
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs | Deduplicates per-object wrapper registration in the diagnostics tracking table to avoid list growth from repeated registrations. |
This was referenced Sep 4, 2026
Closed
Member
Author
|
/backport to release/10.0 |
Member
Author
|
/backport to release/11.0 |
Contributor
|
Started backporting to |
Contributor
|
Started backporting to |
This was referenced Sep 5, 2026
AaronRobinsonMSFT
approved these changes
Sep 5, 2026
JulieLeeMSFT
pushed a commit
that referenced
this pull request
Sep 9, 2026
…3292) Backport of #133260 to release/10.0 /cc @jkoritzinsky ## Customer Impact - [X] Customer reported - [ ] Found internally Ever increasing managed memory usage (memory leak) each time a managed object is passed to COM via ComWrappers, even when reusing the same COM instance. The only workaround is to turn off the "managed debugging" helper APIs. ## Regression - [X] Yes - [ ] No Regressed in #113907 ## Testing Local validation via SOS (the only direct exposure of the adjusted API) ## Risk Low, only affects an API exposed to SOS (no other consumers use it). As mentioned in the PR on main, the expected number of elements in the collection when deduplicated is 1 or 2 at most, so not using a hash-table is fine for the scenario (in particular the user-reported issue will have 1 item in the collection after the fix). **IMPORTANT**: If this backport is for a servicing release, please verify that: - For .NET 8 and .NET 9: The PR target branch is `release/X.0-staging`, not `release/X.0`. - For .NET 10+: The PR target branch is `release/X.0` (no `-staging` suffix). ## Package authoring no longer needed in .NET 9 **IMPORTANT**: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version. Keep in mind that we still need package authoring in .NET 8 and older versions. Co-authored-by: Jeremy Koritzinsky <jekoritz@microsoft.com>
akoeplinger
pushed a commit
that referenced
this pull request
Sep 11, 2026
…3291) Backport of #133260 to release/11.0 /cc @jkoritzinsky ## Customer Impact - [X] Customer reported - [ ] Found internally Ever increasing managed memory usage (memory leak) each time a managed object is passed to COM via ComWrappers, even when reusing the same COM instance. The only workaround is to turn off the "managed debugging" helper APIs. ## Regression - [X] Yes - [ ] No Regressed in #113907 ## Testing Local validation via SOS (the only direct exposure of the adjusted API) ## Risk Low, only affects an API exposed to SOS (no other consumers use it). As mentioned in the PR on main, the expected number of elements in the collection when deduplicated is 1 or 2 at most, so not using a hash-table is fine for the scenario (in particular the user-reported issue will have 1 item in the collection after the fix). **IMPORTANT**: If this backport is for a servicing release, please verify that: - For .NET 8 and .NET 9: The PR target branch is `release/X.0-staging`, not `release/X.0`. - For .NET 10+: The PR target branch is `release/X.0` (no `-staging` suffix). ## Package authoring no longer needed in .NET 9 **IMPORTANT**: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version. Keep in mind that we still need package authoring in .NET 8 and older versions. Co-authored-by: Jeremy Koritzinsky <jekoritz@microsoft.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prevent duplicate entries in the "all managed object wrappers" table.
Fixes #129191
For the case in #129191, there is always only one ComWrappers instance, so this collection will be length 1.
I chose to do a simple fix like this for the following reasons:
!DumpObjcommand. Any significant changes to the structure (such as moving to aHashSet<ManagedObjectWrapperHolder>) would require very large changes to enable enumerating the structure from DAC or cDAC code, creating a higher risk change for backporting.