Move ComWrappers AddRef to C/C++ #110762
Merged
Move ComWrappers AddRef to C/C++ #110762
Conversation
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
Xaml invokes AddRef while holding a lock that it *also* holds while a GC is in progress. Managed AddRef had to synchronize with the GC that caused intermittent deadlocks with the other thread holding Xaml's lock. This change reverts the managed AddRef implementation to match .NET Native and CoreCLR. Fixes dotnet#110747
Co-authored-by: Aaron Robinson <arobins@microsoft.com>
jkotas
commented
Dec 17, 2024
jkotas
commented
Dec 17, 2024
Sergio0694
approved these changes
Dec 17, 2024
Contributor
Sergio0694
left a comment
There was a problem hiding this comment.
Thank you!! 😄
Tested with the Microsoft Store and could not repro any hangs:
- Checked out
release/9.0-staging(51fd1e2) - Cherry-pick commits from #110558
- Cherry-pick commits from this PR
- Build with
.\build.cmd clr.aot -c Release - Set
IlcSdkPathin the Store .csproj toruntime\artifacts\bin\coreclr\windows.x64.Release\aotsdk\ - Set
GenerateAppxPackageOnBuild - Switch to Release
- Deploy
I then tried to use the Store for a while, and also did the same thing that triggered the hang last time (opening and closing it in quick succession a couple dozen times). I could not repro the issue and everything seemed to be working great 🎉
jkotas
commented
Dec 17, 2024
Comment on lines
1305
to
1312
| #if false // Implemented in C/C++ to avoid GC transitions | ||
| [UnmanagedCallersOnly] | ||
| internal static unsafe uint IUnknown_AddRef(IntPtr pThis) | ||
| { | ||
| ManagedObjectWrapper* wrapper = ComInterfaceDispatch.ToManagedObjectWrapper((ComInterfaceDispatch*)pThis); | ||
| return wrapper->AddRef(); | ||
| } | ||
| #endif |
Member
There was a problem hiding this comment.
I don't really like the #if false pattern. I'd rather we just include a comment on line 1160 explaining that we have it implemented in C/C++ to avoid GC transitions.
AaronRobinsonMSFT
approved these changes
Dec 17, 2024
Member
AaronRobinsonMSFT
left a comment
There was a problem hiding this comment.
I agree with @jkoritzinsky's comment about the #if false style. Other than that, LGTM.
jkotas
commented
Dec 17, 2024
...nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs
Outdated
Show resolved
Hide resolved
jkotas
commented
Dec 17, 2024
...nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs
Outdated
Show resolved
Hide resolved
jkotas
commented
Dec 17, 2024
...nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs
Outdated
Show resolved
Hide resolved
…e/InteropServices/ComWrappers.NativeAot.cs
jkoritzinsky
approved these changes
Dec 17, 2024
Member
Author
|
/backport to release/9.0-staging |
Contributor
|
Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/12396026201 |
4 tasks
Sergio0694
pushed a commit
to Sergio0694/runtime
that referenced
this pull request
Jan 7, 2025
* Move ComWrappers AddRef to C/C++ Xaml invokes AddRef while holding a lock that it *also* holds while a GC is in progress. Managed AddRef had to synchronize with the GC that caused intermittent deadlocks with the other thread holding Xaml's lock. This change reverts the managed AddRef implementation to match .NET Native and CoreCLR. Fixes dotnet#110747 Co-authored-by: Aaron Robinson <arobins@microsoft.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.
Xaml invokes AddRef while holding a lock that it also holds while a GC is in progress. Managed AddRef had to synchronize with the GC that caused intermittent deadlocks with the other thread holding Xaml's lock.
This change reverts the managed AddRef implementation to match .NET Native and CoreCLR.
Fixes #110747