Skip to content

Revert "[release/9.0] Fix edge cases in Tarjan GC bridge (Android)" - #114641

Merged
steveisok merged 1 commit into
release/9.0-stagingfrom
revert-114391-9.0-gcbridge
Apr 14, 2025
Merged

Revert "[release/9.0] Fix edge cases in Tarjan GC bridge (Android)"#114641
steveisok merged 1 commit into
release/9.0-stagingfrom
revert-114391-9.0-gcbridge

Conversation

@steveisok

Copy link
Copy Markdown
Member

Reverts #114391

I think due to the assertion #114637 is fixing, we revert for now and let main run with the bridge change. I would be ok bringing this back up for servicing next month.

CopilotAI review requested due to automatic review settings April 14, 2025 17:47
@ghostghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 14, 2025
@steveisok
steveisok requested review from vitek-karas and removed request for lambdageekApril 14, 2025 17:47

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 6 changed files in this pull request and generated 1 comment.

Files not reviewed (2)
  • src/tests/GC/Features/Bridge/Bridge.csproj: Language not supported
  • src/tests/GC/Features/Bridge/BridgeTester.csproj: Language not supported
Comments suppressed due to low confidence (2)

src/tests/GC/Features/Bridge/BridgeTester.cs:1

  • The removal of BridgeTester.cs may reduce test coverage for the GC bridge functionality. Please verify that adequate tests exist elsewhere to validate this behavior.
File removed (BridgeTester.cs)

src/mono/mono/metadata/sgen-tarjan-bridge.c:792

  • Replacing 'new_color(FALSE)' with 'reduce_color()' in the non-bridge branch changes the logic for creating a new color. Verify that this change is intentional and that it does not adversely affect the SCC formation behavior.
color_data = reduce_color ();

Comment threadsrc/mono/mono/metadata/sgen-bridge.c
@steveisok

Copy link
Copy Markdown
MemberAuthor

@carlossanlop ideally this should get in before you branch for servicing.

@carlossanlopcarlossanlop added os-android area-GC-mono and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 14, 2025
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@carlossanlop

Copy link
Copy Markdown
Contributor

Since the original PR was merged on Apr 8 (in other words, it hasn't been released yet), this can be marked as servicing-approved and get merged as soon as the CI looks good.

There's a GitHub Copilot review comment. Is that something you want to take or was it an incorrect suggestion?

cc @filipnavara

@steveisoksteveisok added the Servicing-approved Approved for servicing release label Apr 14, 2025
@filipnavara

Copy link
Copy Markdown
Member

There's a GitHub Copilot review comment. Is that something you want to take or was it an incorrect suggestion?

The suggestion is reverting the revert... in other words, NO. :) It will be fixed once this is resubmitted for next servicing release.

@matouskozak

Copy link
Copy Markdown
Member

fyi: this could possily fix the CI crashes we're seeing #114631

@steveisok

Copy link
Copy Markdown
MemberAuthor

/ba-g Wasm restarts

@steveisok
steveisok merged commit 39bae29 into release/9.0-stagingApr 14, 2025
@steveisok
steveisok deleted the revert-114391-9.0-gcbridge branch April 14, 2025 22:39
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-GC-monoos-androidServicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@steveisok@carlossanlop@filipnavara@matouskozak@vitek-karas@AaronRobinsonMSFT