Uh oh!
There was an error while loading. Please reload this page.
[release/10.0] [mono][sgen] Make color visible to client permanent - #121483
Merged
Conversation
…net#121243) This option is also currently causing crashes in the gc bridge.
A color (SCC) that isn't containing any bridge objects is made visible to client if xrefs_in * xrefs_out is greater than 60. Later on in bridge processing, we need to build the callback to pass for .net android. During this stage, we reduce from the full set of SCCs to SCCs that should be visible to client (containing bridge objects or satisfying the above condition). If an SCC has an xref to a color that is not visible to client, we need to do a recursive traversal to find all neighbors that are visible to client. The problem is that this process can end up making an SCC no longer visible to client, leading to inconsistencies in the computation. Consider a color(C1) that has a neighbor that is not visible to client(C2). In this final stage, we compute the neighbors of C1 by traversing recursively through the neighbors of C2. If C2 ends up pointing to colors that were already neighbors of C1, then, following this computation, C1 would end up with fewer xrefs_out, making the color no longer visible to client. This make future checks incorrect, resulting in building incorrect graph for client. This scenario seems rare in practice, we should have gotten way more reports otherwise. We fix this by pinning the visible_to_client property for a color once it first satisfies it, so it will no longer matter how many actual xrefs the color has. Fixes assertions like: ``` * Assertion at /home/vbrezae/Xamarin/repos/runtime/src/mono/mono/metadata/sgen-tarjan-bridge.c:1151, condition `color_visible_to_client (cd)' not met ```
Contributor
Tagging subscribers to this area: @BrzVlad |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the GC bridge implementation where color visibility to client could change during bridge processing, leading to incorrect behavior. The fix introduces a flag to pin a color as visible to client once detected, preventing it from becoming invisible even if it loses cross-references later.
Key changes:
- Added a
visibleToClient/visible_to_clientflag to ColorData struct in both CoreCLR and Mono implementations - Modified color visibility checking logic to cache the visibility decision
- Added a new test case that exercises the "bridgeless heavy color changing" scenario
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/tests/GC/Features/Bridge/Bridge.cs | Adds InlineData struct and BridgelessHeavyColorChanging test to verify the fix |
| src/mono/mono/metadata/sgen-tarjan-bridge.c | Implements visible_to_client flag, fixes loop counter bug in dump_color_table, refactors visibility checks |
| src/coreclr/gc/gcbridge.cpp | Implements visibleToClient flag and refactors visibility checks (parallel to Mono changes) |
| src/mono/mono/metadata/sgen-bridge.c | Adds disable-non-bridge-scc configuration option |
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Nov 10, 2025
Open
steveisok
approved these changes
Nov 10, 2025
steveisok
enabled auto-merge (squash)
November 10, 2025 20:09
steveisok
commented
Nov 10, 2025
Member
/ba-g Known issues #117164dotnet/dnceng#6408dotnet/dnceng#5015#97020 |
Uh oh!
There was an error while loading. Please reload this page.
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 freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Backport #121247 and #121243 to release/10.0.
This fixes assertions like
Customer Impact
Some applications on maui-android can randomly crash during GC, when using the default gc bridge (the tarjan bridge). We've had a few fixes for the tarjan bridge merged a few months ago, but there is still this one issue. The workaround used by customers is to fallback to an older GC bridge which has worse performance. For some this performance impact is not acceptable. This backport also fixes the same issue in the CoreCLR gcbridge implementation. CoreCLR doesn't have a fallback GC bridge implementation, so this fix is essential for the successful use of CoreCLR/NativeAOT on android, at least for some customers.
Regression
Testing
Tested on our own gc bridge tests, with scenario causing the issue.
Risk
The GC bridge is a sensitive area and fixes here typically have some carried risk. This fix however is quite straightforward, it simply pins the value of a property inside an SCC node, rather than having it recomputed with unstable value that was leading to problems. No changes are done to the core algorithm. Low risk.