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

Introduce a context object for GC controller thread #533

Merged
merged 5 commits into from
Feb 3, 2022

Conversation

wks
Copy link
Collaborator

@wks wks commented Jan 29, 2022

This is one of a series of planned changes to refactor the scheduler. The problems are described in issue #532

This PR introduces the GCController struct. Like GCWorker, a GCController shall be owned by the GC controller thread. It owns its GCWorker (the coordinator_worker) and the receiving end of the worker-to-controller channel. It also has references to the data structures it needs to work with, including the scheduler and the ControllerCollectorContext which is used to trigger GC.

Because of this new struct, some related fields and operations are removed from ControllerCollectorContext and GCWorkScheduler.

There is also an API change. spawn_worker_thread is renamed to spawn_gc_thread. Now both GC worker threads and the GC controller thread are spawn with a context object.

This PR does not change the two-phase initialisation of GCWorkScheduler. It will be done in another PR.

wks added 2 commits January 29, 2022 14:42
This commit introduces the `GCController` struct.  It is owned by the GC
controller thread.  It owns the `GCWorker` for the controller, and has
references to objects that it needs to communicate with other threads.

Previously, those dependencies were scattered in
`ControllerCollectorContext` and `GCWorkScheduler`, which complicated
the initialization of the MMTk instance and the GC controller thread.
Move the GCWorkScheduler::wait_for_completion method to the GCController
type.  This allows us to move controller-specific fields away from
GCWorkScheduler, too.
src/memory_manager.rs Outdated Show resolved Hide resolved
Copy link
Member

@qinsoon qinsoon left a comment

Choose a reason for hiding this comment

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

Very nice refactoring. It makes the code much cleaner. As this PR is still a draft, I did not approve it. But I do not see any major issue with the PR.

src/memory_manager.rs Outdated Show resolved Hide resolved
src/plan/mod.rs Outdated Show resolved Hide resolved
src/scheduler/controller.rs Show resolved Hide resolved
src/scheduler/controller.rs Outdated Show resolved Hide resolved
src/plan/controller_collector_context.rs Outdated Show resolved Hide resolved
src/scheduler/controller.rs Outdated Show resolved Hide resolved
src/scheduler/controller.rs Outdated Show resolved Hide resolved
src/scheduler/scheduler.rs Outdated Show resolved Hide resolved
src/scheduler/controller.rs Outdated Show resolved Hide resolved
-   Renamed ControllerCollectorContext to GCRequester
-   Removed initializer and finalizer functions from GCWorkScheduler
-   The embedded GCController is no longer boxed.
-   Added comments
-   Minor adjustment to API functions.
-   Other minor fixes.
@wks wks marked this pull request as ready for review January 31, 2022 16:30
@wks
Copy link
Collaborator Author

wks commented Jan 31, 2022

@qinsoon I made changes according to your suggestions. I set this PR to be ready for review.

When I was reading the code of GCWorker, I think some methods of GCWorkScheduler can be moved to GCWorker, too, just like what I have done to GCController. I'll do it in the next PR.

Copy link
Member

@qinsoon qinsoon 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 merged commit 29afb0c into mmtk:master Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants