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

Cleanup GC worker init #514

Merged
merged 37 commits into from
Jan 13, 2022
Merged

Cleanup GC worker init #514

merged 37 commits into from
Jan 13, 2022

Conversation

qinsoon
Copy link
Member

@qinsoon qinsoon commented Jan 9, 2022

This PR resolves the discussion here: #499 (comment).

@qinsoon qinsoon marked this pull request as ready for review January 9, 2022 23:48
@qinsoon qinsoon requested a review from wks January 9, 2022 23:48
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.

There is no problem with this refactoring.

But the recurring long sequence of .as_ref().unwrap() to get into Option<_> shows that MMTk may have two-step initialisation anti-pattern. It is understandable because MMTk was originally developed in Java. We may consider a more major refactoring later, if we find it problematic.

@@ -206,9 +206,9 @@ impl<VM: VMBinding> GCWorkScheduler<VM> {
buckets.iter().all(|&b| self.work_buckets[b].is_drained())
}

pub fn initialize_worker(self: &Arc<Self>, tls: VMWorkerThread) {
pub fn initialize_coordinator_worker(self: &Arc<Self>, tls: VMWorkerThread) {
let mut coordinator_worker = self.coordinator_worker.as_ref().unwrap().write().unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This as_ref().wrap() is the same, but let's look at it later.

Comment on lines +53 to +61
// Initialize the GC worker for coordinator. We are not using the run() method from
// GCWorker so we manually initialize the worker here.
self.scheduler
.read()
.unwrap()
.as_ref()
.unwrap()
.initialize_coordinator_worker(tls);

Copy link
Collaborator

Choose a reason for hiding this comment

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

The as_ref().unwrap() part is for getting into the Option<_> in RwLock<Option<Arc<GCWorkScheduler<VM>>>>.

This long sequence may be a consequence of MMTk using two-phase initialisation. It is considered by Rust as an anti-pattern, but considering that GC is very low-level, there may be reasons why it has to be done this way. Let's investigate it later.

Copy link
Member Author

Choose a reason for hiding this comment

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

We have an issue tracking this: #57. In this particular case, I don't think it is because of GC being low-level.

@qinsoon qinsoon added the PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) label Jan 13, 2022
@qinsoon qinsoon merged commit df3dee4 into master Jan 13, 2022
@qinsoon qinsoon deleted the cleanup-gc-worker-init branch January 13, 2022 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants