Skip to content

Conversation

@qinsoon
Copy link
Member

@qinsoon qinsoon commented Oct 20, 2025

This PR moves GC triggering related code to GCTrigger, including:

  • The old GCRequester which handles the request for GCs.
  • The check for is_collection_enabled in memory_manager and Space::acquire.
  • Code for user collection request and internal collection request in MMTK.

@qinsoon
Copy link
Member Author

qinsoon commented Oct 20, 2025

This PR was originally created to support a fix for #1398. By consolidating all GC-triggering logic into GCTrigger, it becomes easier (or possible) to apply locks or atomics to address the data race when enabling collection.

I’ve also prototyped a thread-safe “enable collection” API, but it turned out to be difficult to use from Julia due to the issues described in mmtk/mmtk-julia#278. Since Julia is the main reason we need these APIs, it doesn’t make much sense to introduce them if they don’t integrate well there. As a result, a proper fix for #1398 may be postponed.

Given that said, I think this PR on its own is still a good cleanup, even if there’s no immediate follow-up to fix #1398.

@qinsoon qinsoon added the PR-extended-testing Run extended tests for the pull request label Oct 20, 2025
@qinsoon qinsoon requested a review from wks October 21, 2025 02:39
Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

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

Just one minor problem. Other parts look good.

Comment on lines 102 to 104
fn get_plan(&self) -> &dyn Plan<VM = VM> {
unsafe { self.plan.assume_init() }
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method is identical to GCTrigger::plan. Keep one of them and move the other.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. I didn't realize we had that function.

wks
wks previously approved these changes Oct 21, 2025
Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

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

LGTM See comment below

@wks wks dismissed their stale review October 21, 2025 03:05

Still things worth updating.

@wks
Copy link
Collaborator

wks commented Oct 21, 2025

I grepped the code and found there are still comments mentioning the GCRequester type. We should update those comments, too.

And the eBPF tools capture.bt and visualize.py also contains the USDT name gcrequester_request. It can be renamed (e.g. simply change it to gc_requested)

@qinsoon
Copy link
Member Author

qinsoon commented Oct 21, 2025

Done. I further pushed a change for memory_manager::gc_poll (which no longer needs to check is_collection_enabled -- gc_trigger.poll will check is_collection_enabled)

Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

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

LGTM

@qinsoon qinsoon enabled auto-merge October 21, 2025 03:37
@qinsoon qinsoon added this pull request to the merge queue Oct 21, 2025
Merged via the queue into mmtk:master with commit c6317a3 Oct 21, 2025
32 of 33 checks passed
@qinsoon qinsoon deleted the consolidate-gc-trigger branch October 21, 2025 05:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR-extended-testing Run extended tests for the pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants