Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mono] Add a few bridge tests #113703

Merged
merged 4 commits into from
Mar 24, 2025
Merged

[mono] Add a few bridge tests #113703

merged 4 commits into from
Mar 24, 2025

Conversation

BrzVlad
Copy link
Member

@BrzVlad BrzVlad commented Mar 19, 2025

This PR ports over some bridge tests that we had in mono/mono. In order to enforce finalization of some objects that are logically dead, we reuse the FinalizerHelpers.PerformNoPinAction, which runs an Action while ensuring objects allocated by it are seen dead by the GC. Because mono GC is conservative, we go through extra lengths to ensure no pinning occurs.

Mono currently has two algorithms for building the SCC/xref set over the dead bridge objects: new and tarjan. The new bridge is the legacy one which should be more stable and this PR attempts to reuse the bridge output compare functionality to catch potential bugs with the tarjan bridge (which is the default used bridge). When using the bridge compare functionality, the GC will randomly keep alive half of the bridge objects (since we don't have a java counterpart that responds with the liveness). We also pass the additional debug flag bridge=BridgeBase which means that all objects that are instances of BridgeBase or a derived type are treated as a bridge object and they take part in the SCC construction.

This testing approach is very cheap but it has some potential pitfalls: maybe the new bridge itself has some bugs, the output between the bridges differs for benign reasons and the tarjan bridge needs to have some functionality disabled in order for this comparison to work (scc_precise_merge and disable_non_bridge_scc, which are optimizations meant to speed up the bridge processing, even if they don't produce the theoretical correct set of SCCs/xrefs).

Currently there are quite a bit of tests crashing that need investigation. In the future we could also add more complex random graph generators for stress testing.

This PR also fixes a few logging issues in the tarjan bridge due to bitrot.

Copy link
Contributor

Tagging subscribers to this area: @BrzVlad
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds and ports several bridge tests used to ensure proper finalization of logically dead objects and to compare two GC bridge algorithms (legacy "new" and default "tarjan") in the mono runtime. The changes include a new test runner in BridgeTester.cs and a comprehensive set of bridge and graph tests in Bridge.cs to exercise various object linking, fragmentation, and cycle handling scenarios.

Reviewed Changes

Copilot reviewed 2 out of 5 changed files in this pull request and generated 2 comments.

File Description
src/tests/GC/Features/Bridge/BridgeTester.cs Adds a test runner that launches the Bridge test application with specific GC environment variables
src/tests/GC/Features/Bridge/Bridge.cs Introduces multiple bridge/graph test scenarios and helper methods to exercise the GC bridge behavior
Files not reviewed (3)
  • src/mono/mono/metadata/sgen-tarjan-bridge.c: Language not supported
  • src/tests/GC/Features/Bridge/Bridge.csproj: Language not supported
  • src/tests/GC/Features/Bridge/BridgeTester.csproj: Language not supported

@BrzVlad
Copy link
Member Author

BrzVlad commented Mar 19, 2025

cc @Maoni0

@Maoni0
Copy link
Member

Maoni0 commented Mar 19, 2025

this is great! thanks so much @BrzVlad. I think Spider does test the case where we need to include a non bridge object in the graph. but I wonder if we could add a simple test that demonstrates that (which would make debugging much easier to just see how the algo works in general)? basically the idea is that you have multiple bridge objects (or bridge objects that would form multiple SCCs) that point to one non bridge object which points to multiple bridge objects, right?

@BrzVlad
Copy link
Member Author

BrzVlad commented Mar 20, 2025

Given Spider test only has 3 bridge objects allocated, it doesn't make sense to include non bridge objects in the set of SCCs to be passed, since there is a very small number of xrefs that you can have anyway. I think the test was meant to test the ability of the tarjan bridge to ignore large numbers of SCCs (that are irrelevant because they contain only non-bridge objects).

I haven't looked into the inclusion of non bridge objects in the final set of SCCs, but what you are describing sounds like what I would expect to trigger this optimization. Note that when we have bridge output comparison enabled, this optimization is disabled.

These tests were lying around from mono days and they create quite complex graphs. But the NestedCycles test that @filipnavara wrote in order to reproduce a specific crash is a pretty good starting point to see how the algorithm works, if this is what you are asking.

@BrzVlad BrzVlad force-pushed the fix-tarjan-bridge branch from 5a662af to e5daa05 Compare March 24, 2025 07:46
@Maoni0
Copy link
Member

Maoni0 commented Mar 24, 2025

  ScanData *sd = find_data (dyn_array_ptr_get (&registered_bridges, i));

can you also get rid of int here too please?


Refers to: src/mono/mono/metadata/sgen-tarjan-bridge.c:995 in e5daa05. [](commit_id = e5daa05, deletion_comment = False)

Copy link
Member

@Maoni0 Maoni0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other than the couple of nits this LGTM! I might add a couple of test cases later when I've looked more at how the bridge works but that doesn't need to be in this PR.

@BrzVlad BrzVlad force-pushed the fix-tarjan-bridge branch from e5daa05 to a526d3c Compare March 24, 2025 10:15
BrzVlad added 4 commits March 24, 2025 12:16
It serves no purpose and it would later crash the runtime since we didn't patch the lockword back in place.
These are ported from some of the bridge tests we had on mono/mono. In order to test them we compare between the output of the new and the tarjan bridge.
@BrzVlad BrzVlad force-pushed the fix-tarjan-bridge branch from a526d3c to 71fc2cc Compare March 24, 2025 10:17
@BrzVlad BrzVlad merged commit e0f1b21 into dotnet:main Mar 24, 2025
112 of 115 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants