Skip to content

Conversation

@wks
Copy link
Collaborator

@wks wks commented Oct 25, 2023

We removed the feature for letting the coordinator call both stop_all_mutators and resume_mutators since #794 and #814. The documentation should reflect that.

@wks wks requested a review from qinsoon October 25, 2023 13:10

/// Resume all the mutator threads, the opposite of the above. When a GC is finished, MMTk calls this method.
///
/// This method may be called by any GC worker thread, and may not be the same GC worker thread that called
Copy link
Collaborator

Choose a reason for hiding this comment

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

That is not correct. At least as of now. Currently the GC controller thread calls resume_mutators in EndOfGC

Copy link
Collaborator

Choose a reason for hiding this comment

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

See here as well:

let mut end_of_gc = EndOfGC {
elapsed: gc_start.elapsed(),
};
end_of_gc.do_work_with_stat(&mut self.coordinator_worker, self.mmtk);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK. That's true, although it is not intended. I'll adjust the comment and not mention "worker".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Random aside, I feel like we should rename stop_all_mutators to suspend_mutators to be in line with resume_mutators.

We no longer have the feature for letting the coordinator call
stop_mutators or resume_mutators.
@wks wks force-pushed the fix/stop-mutators-doc branch from 7c738ff to e14f427 Compare October 25, 2023 13:19
@wks wks changed the title Update the doc comment of {stop,resume}_mutators Update doc comments Oct 25, 2023
Copy link
Collaborator

@k-sareen k-sareen left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@k-sareen k-sareen enabled auto-merge October 25, 2023 13:39
@k-sareen k-sareen added this pull request to the merge queue Oct 25, 2023
Merged via the queue into mmtk:master with commit 59ff055 Oct 25, 2023
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.

2 participants