Tidy up mutator scan API

[qinsoon]

This PR closes #496.

• Remove Scanning::SCAN_MUTATORS_IN_SAFEPOINT, and Scanning::SINGLE_THREAD_MUTATOR_SCANNING.
• Remove Scanning::scan_roots_in_all_mutator_threads.
• Remove Collection::prepare_mutator
• Update comments for a few methods

[k-sareen]

I also just realized that the GC state is never updated to GcProper in case the binding does not use the callback and does not implement scan_roots_in_mutator_thread , i.e. the case where the binding prefers not to or cannot differentiate stack roots from other kinds of roots. It's not a big deal, but thought I'd mention it

[qinsoon]

I also just realized that the GC state is never updated to GcProper in case the binding does not use the callback and does not implement scan_roots_in_mutator_thread , i.e. the case where the binding prefers not to or cannot differentiate stack roots from other kinds of roots. It's not a big deal, but thought I'd mention it

Right. We should require the binding to use the callback. Bindings should use the callback, but do not have to implement scan_roots_in_mutator_thread.

[k-sareen]

That feels like a bit of a waste though? We'll end up scheduling empty ScanStackRoot work packets

[k-sareen]

But I also realized that if the binding doesn't execute the ScanStackRoot work packets, then the remembered sets for mutators won't be flushed

[k-sareen]

Perhaps we should just flush the mutators in prepare_mutator instead of doing it in the ScanStackRoot work packet

[k-sareen]

My understanding of the discussion was that if the binding/VM can't tell us when its threads are ready for scanning, then we should still provide a way to flush each mutator. Or is that the binding's job? If it is, then it should be documented.

[qinsoon]

My understanding of the discussion was that if the binding/VM can't tell us when its threads are ready for scanning, then we should still provide a way to flush each mutator. Or is that the binding's job? If it is, then it should be documented.

I think we conclude that a binding will tell us when the thread is ready to be scanned. It can tell us as soon as a thread is stopped, or after all the threads are stopped. But the binding has to tell us that.

Flushing mutator is not a binding's job. MMTk will do that. We require a binding to tell us if a thread can be scanned, and we schedule ScanStackRoot for the thread. In ScanStackRoot, we ask the binding for stack roots, and also flush mutators.

[k-sareen]

Right. I'm getting hung up on the case where the binding/VM doesn't differentiate between thread roots and other kinds of roots. In that case the binding will just dump all roots into the scan_vm_specific_roots function. But that won't flush mutators unless the binding also creates empty ScanStackRoot work packets by using the callback in stop_all_mutators.

Also can we rename the ScanStackRoot work packet while we're at it? It's ambiguous. Renaming to ScanStackRoots (given that we're removing the ScanStackRoots work packet) or ScanStackRootsForMutator would be better imo.

[wks]

Someone may want some work to be done before scanning any stacks, but currently there is no such things. For Ruby, "potential pinning parents" (PPP) needs to be handled before transitive closure, but can be done at the same time as stack scanning.

We also had some issue letting OpenJDK report the yielding of individual threads. See mmtk/openjdk#9 There used to be something called report_java_thread_yield, but we removed that. So the current status is that OpenJDK can only report that all threads have come to a stop.

I think we can leave the code as is for now. After we figure out how to report the yielding of each thread (for OpenJDK 11, 17 and 22), we can make another attempt and move the ScanStackRoot work packet to Unconstrained or another bucket.

[k-sareen]

There is the prepare_mutator function for VM-specific behaviour. But this PR would remove that.

[qinsoon]

Someone may want some work to be done before scanning any stacks, but currently there is no such things. For Ruby, "potential pinning parents" (PPP) needs to be handled before transitive closure, but can be done at the same time as stack scanning.

The binding can do whatever work is needed before it tells MMTk that the thread is ready. For the Ruby case, the binding can just schedule the PPP work to a bucket prior to the Closure bucket.

[qinsoon]

There is the prepare_mutator function for VM-specific behaviour. But this PR would remove that.

Yeah. I think it is confusing about when we should invoke it. For the old timing where we call prepare_mutator, the binding can do it before returning stop_all_mutators. JikesRVM is the only binding we know that uses the method. I changed it to https://github.com/mmtk/mmtk-jikesrvm/pull/147/files#diff-54f4f04a18788baf9bf66cff8c4256151b680087e3490fb8e8f62e2d60ad9fabR30.

[wks]

Also can we rename the ScanStackRoot work packet while we're at it? It's ambiguous. Renaming to ScanStackRoots (given that we're removing the ScanStackRoots work packet) or ScanStackRootsForMutator would be better imo.

I would suggest simply calling it ScanMutatorRoots. It's not only stack roots, but may include other thread-local states as well. For example, for OpenJDK, it calls JavaThread::oops_do which scans not only Java frames, but also thread-local JNI handles, pending exceptions, monitors, etc.

[qinsoon]

Also can we rename the ScanStackRoot work packet while we're at it? It's ambiguous. Renaming to ScanStackRoots (given that we're removing the ScanStackRoots work packet) or ScanStackRootsForMutator would be better imo.

I would suggest simply calling it ScanMutatorRoots. It's not only stack roots, but may include other thread-local states as well. For example, for OpenJDK, it calls JavaThread::oops_do which scans not only Java frames, but also thread-local JNI handles, pending exceptions, monitors, etc.

I have renamed the packet to ScanMutatorRoots.

[wks]

LGTM. But bindings need to adapt to this change, too.