Moving ScanObjects::do_work to the plan

[wks]

TL;DR: If ScanObjects is plan-specific, why not let Plan do what ScanObjects::do_work does, so that ScanObjects is no longer plan-specific?

Status Quo

Now we have

• ScanObjects
• ScanObjectsAndMarkLines

Only Immix uses the latter. Because it uses the latter, Immix also has

• ImmixProcessEdges
• GenImmixMatureProcessEdges

They only exist because they need the special ScanObjectsAndMarkLines

But we don't want so many plan-specific work packets.

Proposal.

Just one ScanObjects. No ScanObjectsAndMarkLines.

We move ScanObjects::do_work to Plan.

// For implemetation, ScanObjects now needs a Plan type 
struct ScanObjects<E: ProcessEdgesWork, P: Plan> {
   ...
}

impl<E: ProcessEdgesWork, P: Plan> GCWork<E::VM> for ScanObjects<E, P> { 
    fn do_work(&mut self, worker: &mut GCWorker<E::VM>, mmtk: &'static MMTK<E::VM>) {
        // It has to be statically dispatched because it has type parameters.
        P::scan_objects::<E>::(worker.tls, &self.buffer);

        // Alternatively we can give it a `&dyn ProcessEdgesWorkFactory<E>` so it is less efficient but can be dynamically dispatched.
    }
}

trait Plan {
    fn scan_objects<E: ProcessEdgesWork>(tls: VMWorkerThread, buffer: &Vec<ObjectReference>) {
        trace!("Now I do the actual ScanObject");
        {
            let mut closure = ObjectsClosure::<E>::new(worker);
            <E::VM as VMBinding>::VMScanning::scan_objects(tls, buffer, &mut closure);
        }
        trace!("The actual ScanObject End");
    }
}

If Immix is special, it can just override the plan.

impl Plan for Immix {
    fn scan_objects<E: ProcessEdgesWork>(tls: VMWorkerThread, buffer: &Vec<ObjectReference>) {
        trace!("ScanObjectsAndMarkLines");
        let mut closure = ObjectsClosure::<E>::new(worker);
        for object in buffer {
            <E::VM as VMBinding>::VMScanning::scan_object(tls, *object, &mut closure);
            if super::MARK_LINE_AT_SCAN_TIME
                && !super::BLOCK_ONLY
                && self.immix_space.in_space(*object)
            {
                self.immix_space.mark_lines(*object);
            }
        }
    }
}

And we can delete ScanObjectsAndMarkLines.

[qinsoon]

The idea looks fine. But scan_objects is policy-specific, rather than plan-specific. But I am sure we can work around that. Possibly scan_objects can be a part of the PolicyTraceObject trait introduced in #575.

[qinsoon]

I ended up with an implementation of scan_object() similar to trace_object(). For spaces in a plan with #[policy_scan], it checks if an object is in any of the space. If it is, forward the call to the scan_object() defined by the policy. Otherwise, simply call VM::VMScanning::scan_object(). See the implementation here: 74a71de

In theory the generated code should be doing the same as before (with policy-specific scan work packet).

[wks]

I suggest changing PolicyTraceObject::scan_object to PolicyTraceObject::post_scan_object, making it a "post-scan hook" rather than let it customise how to do scan_object. The concrete code can be found in my personal repository: wks@226994a

In aspect-oriented programming terms, scan_object was an around advice, and my new post_scan_object is an after advice. "Around advice" is more powerful than "after advice". However, with great power comes great responsibilities. "Around advice" is too powerful for the ImmixSpace use case, and "after advice" (or "before advice") is just enough.

I examined what ImmixSpace does in scan_object. It uses scan_object as an opportunity to call mark_line if Immix is configured to MARK_LINE_AT_SCAN_TIME. It doesn't change how to scan object, nor does ImmixSpace do anything to the edges yielded from scanning. In fact, it is the VM binding that determines how to scan objects, not mmtk-core, and certainly not concrete GC algorithms.

More over, I would like to add object enqueuing (see #581). By doing this, I will change ScanObjects so that it may not necessarily call VMScanning::scan_object. Instead, if an object does not support edge-enqueuing, we shall call a different method to scan the object and process all of its edges at the same time.

impl<E: ProcessEdgesWork> GCWork<E::VM> for ScanObjects<E> {
    fn do_work(&mut self, worker: &mut GCWorker<E::VM>, _mmtk: &'static MMTK<E::VM>) {
        trace!("ScanObjects");
        {
            let tls = worker.tls;
            let mut edges_closure = EdgesClosure::<E>::new(worker);
            let mut objects_closure = ObjectsClosure::<E>::new(worker);
            for object in &self.buffer {
                use crate::vm::Scanning;
                if <E::VM as VMBinding>::VMScanning::support_edge_enqueuing {
                    // Do edge-enqueuing as we do it now.
                    <E::VM as VMBinding>::VMScanning::scan_object(tls, *object, &mut edges_closure);
                } else {
                    // Additionally support object-enqueuing.
                    <E::VM as VMBinding>::VMScanning::scan_object_and_process_edges(tls, *object, |to_obj| {
                        trace_object(to_obj, &mut objects_closure)
                    );
                }
                // This is called no matter how an object is scanned,
                // so ImmixSpace remain oblivious of object-enqueuing or edge-enqueuing.
                self.plan.post_scan_object(*object);
            }
        }
        trace!("ScanObjects End");
    }
}

If ImmixSpace provides an "around advice" to scan_object, it will have the responsibility to actually scan the object. This is why we require it to call VMScanning::scan_object. If I change the way of object scanning and enqueuing, ImmixSpace::scan_object will need to be changed, too. This is what I meant by "'around advices' are too powerful". If we make it a "post-scan hook" (i.e. an "after advice"), it will be orthogonal to the actual object scanning, making it much clearer and making refactoring easier.

[qinsoon]

This sounds good. I probably will just cherry-pick your commit to my PR.