Clarify stack root scanning in API

[qinsoon]

If VMScanning::SCAN_MUTATORS_IN_SAFEPOINT is false, mmtk-core does not schedule any stack root scanning work (ScanStackRoots or ScanStackRoot). MMTk expects each thread to scan its own stack once the thread is stopped, thus MMTk does not schedule any stack root scanning packet. However, this is not documented anywhere.

[k-sareen]

See some further discussion here: https://mmtk.zulipchat.com/#narrow/stream/262673-mmtk-core/topic/SCAN_MUTATORS_IN_SAFEPOINT/near/364428734

@wks Suggested making the boolean into an enum to clarify intent and add more documentation.

[wks]

For example, we can change it to

/// When do we scan stacks
enum StackScanStrategy {
    /// Create stack-scanning work packets after all mutators stopped.
    AfterStopMutators,
    /// Create a stack-scanning work packet for a mutator as soon as the
    /// `mutator_visitor` callback of `Collection::stop_all_mutators` is called.
    /// The VM binding may choose not to call back, in which case mmtk-core
    /// will not create any stack-scanning work packets.  It is useful if the VM
    /// prefers scanning stacks in its own way during `stop_all_mutators`
    /// (i.e. not using the `ScanStackRoot{,s}` work packets), or
    /// scanning stacks during `Scanning::scan_vm_specific_roots`.
    OnCallback,
}

[k-sareen]

Maybe rename the OnCallback to HandledByVm or some other better name given as you point out in the comments that the callback may never actually be used if the VM scans stacks in the scan_vm_specific_roots function.

[wks]

... each thread to scan its own stack once the thread is stopped...

Currently each time the callback mutator_visitor: impl FnMut(&'static mut Mutator<VM>) is called, mmtk-core crates a ScanStackRoot work packet for the mutator given. But the ScanStackRoot work packet is added to the Prepare bucket which is not open until stop_all_mutators returns and mmtk.scheduler.notify_mutators_paused is called. This makes it impossible to achieve the desired effect.

Maybe rename the OnCallback to HandledByVm or some other better name given as you point out in the comments that the callback may never actually be used if the VM scans stacks in the scan_vm_specific_roots function.

Actually OnCallback doesn't (or not intended to) mean "handled by VM". The LXR branch is different. Here is an excerpt from https://github.com/wenyuzhao/mmtk-core/blob/5ff0dd39bda95196284426678ba63505825e36d9/src/scheduler/gc_work.rs#L219

        <E::VM as VMBinding>::VMCollection::stop_all_mutators(
            worker.tls,
            |mutator| {
                mutator.flush();
                mutator.prepare(worker.tls);
                if BULK_THREAD_SCAN {
                    mutators.push(mutator.get_tls());
                } else {
                    mmtk.scheduler.work_buckets[WorkBucketStage::RCProcessIncs]
                        .add(ScanStackRoot::<E>(mutator));
                }
                n += 1;
            },
            mmtk.get_plan()
                .current_gc_should_prepare_for_class_unloading(),
        );

So it doesn't mean stack scanning is handled by the VM binding. It is still handled by MMTk core, but in a different way.

But one possibility is to give the enum a third value, indicating that MMTk core should never create ScanStackRoot work packets as the VM really intend to do all the stack scanning by itself.

enum StackScanStrategy {
    AfterStopMutators,
    OnCallback,
    /// MMTk core will not create any `ScanStackRoot` work packet and therefore never
    /// call `Scanning::scan_roots_in_mutator_thread`.  Use this if the VM wants to handle
    /// stack scanning by itself or in `Scanning::scan_vm_specific_roots`.
    HandledByVm,
}

[k-sareen]

Yes agreed. That makes more sense.

[k-sareen]

We should also clarify that ScanStackRoot and ScanStackRoots are mutually exclusive. As in, the binding should not generate ScanStackRoots and ScanStackRoot work packets together when a GC occurs. This can lead to very hard to debug bugs like in #872. It is also very simple to schedule both work packets since the default configuration for scanning will schedule a ScanStackRoots work packet and then a binding implementer may use the callback in stop_all_mutators and end up accidentally scheduling ScanStackRoot work packets as well, as was happening in this case.

[qinsoon]

We discussed this today. These are the notes:

• We do not need multiple strategies. We just provide a callback in stop_all_mutators. The binding should call the callback once a thread is ready for stack scanning. MMTk core can only scan stacks for a thread if the binding tells us that it is ready. The exact timing when MMTk scan stack is up to MMTk.
• We would like to have only one call for ScanStackRoot (for one thread), and remove the call for ScanStackRoots to avoid confusion. If a binding cannot provide an implementation for scanning a specific thread, they can use scan_vm_specific_roots() instead. In this case, we don't know those are stack roots or just normal VM roots, and we cannot optimize for stack roots.