Procedural macros make it hard to understand the core tracing

[qinsoon]

#575 and #934 introduces macros that generates boilerplate tracing code, including some key tracing functions, for each plan.

For people who are not familiar with the code base, the macros and the generated functions may result in difficulties in understanding how tracing works in MMTk.

We may just remove the use of procedural macros, and replace with equivalent hand-written tracing functions.

[wks]

Writing hand-written tracing functions is tedious and error-prone. I think what we can do is reducing the scopes of the procedural macros so that we write more parts of them in plain Rust code.

For example, what #[derive(PlanTraceObject)] does for the Immix plan is, as shown in cargo expand (https://github.com/dtolnay/cargo-expand)

impl<VM: VMBinding> crate::plan::PlanTraceObject<VM> for SemiSpace<VM> {
    fn trace_object<Q: crate::plan::ObjectQueue, const KIND: crate::policy::gc_work::TraceKind>(
        &self,
        __mmtk_queue: &mut Q,
        __mmtk_objref: crate::util::ObjectReference,
        __mmtk_worker: &mut crate::scheduler::GCWorker<VM>,
    ) -> crate::util::ObjectReference {
        use crate::plan::PlanTraceObject;
        use crate::policy::gc_work::PolicyTraceObject;
        use crate::policy::space::Space;
        if self.copyspace0.in_space(__mmtk_objref) {
            return <CopySpace<VM> as PolicyTraceObject<VM>>::trace_object::<Q, KIND>(
                &self.copyspace0,
                __mmtk_queue,
                __mmtk_objref,
                Some(CopySemantics::DefaultCopy),
                __mmtk_worker,
            );
        }
        if self.copyspace1.in_space(__mmtk_objref) {
            return <CopySpace<VM> as PolicyTraceObject<VM>>::trace_object::<Q, KIND>(
                &self.copyspace1,
                __mmtk_queue,
                __mmtk_objref,
                Some(CopySemantics::DefaultCopy),
                __mmtk_worker,
            );
        }
        <CommonPlan<VM> as PlanTraceObject<VM>>::trace_object::<Q, KIND>(
            &self.common,
            __mmtk_queue,
            __mmtk_objref,
            __mmtk_worker,
        )
    }
    fn post_scan_object(&self, __mmtk_objref: crate::util::ObjectReference) {
        use crate::plan::PlanTraceObject;
        <CommonPlan<VM> as PlanTraceObject<VM>>::post_scan_object(&self.common, __mmtk_objref)
    }
    fn may_move_objects<const KIND: crate::policy::gc_work::TraceKind>() -> bool {
        use crate::plan::PlanTraceObject;
        use crate::policy::gc_work::PolicyTraceObject;
        false
            || <CopySpace<VM> as PolicyTraceObject<VM>>::may_move_objects::<KIND>()
            || <CopySpace<VM> as PolicyTraceObject<VM>>::may_move_objects::<KIND>()
            || <CommonPlan<VM> as PlanTraceObject<VM>>::may_move_objects::<KIND>()
    }
}

The trace_object is basically a chain of if {} if {} if {} which is expanded for each space. But if we have a macro that can enumerate spaces in a plan, we can implement the trace_object method simply using a for loop:

for space in plan.spaces() {
    if (space.in_space(objref)) {
        space.trace_object(queue, objref, worker);
    }
}

And we actually has a method HasSpace::for_each_space, where HasSpace is a supertrait of Plan, and for_each_space is also generated from procedural macros. We can (in theory) implement Plan::trace_object using for_each_space.

But why we are not using HasSpace::for_each_space?

But the difficulty is that each space has a different type, and that's why HasSpace::for_each_space takes a call-back paramerter of FnMut(&dyn Space<Self::VM>). If it is &dyn, we can't call generic methods. But the trace_object methods on many plans are generic. For example,

• fn trace_object<Q: ObjectQueue, const KIND: TraceKind>
• pub fn trace_object<Q: ObjectQueue>(
• pub fn trace_object<Q: ObjectQueue>(

The recurring theme of <Q: ObjectQueue> is there to handle GC phases (notably concurrent marking) where enqueued objects should be handled differently than enqueuing into a real queue (to be made into a ScanObject object). An alternative solution using a return value (ObjectReference, bool) exists (master...wks:trace-object-return2 which was mentioned in #607), but benchmark showed inferior results back then.

The const KIND: TraceKind is needed by the ImmixSpace. We can't call it using &dyn.

Potential solution

I have been thinking about a more general static-dispatching field-visiting macro, something like this:

struct Parent {
    #[interesting]
    field1: Foo,
    #[interesting]
    field2: Bar,
    not_interesting1: Abcde,
    #[interesting]
    field3: Baz,
    not_interesting2: Fghij,
}

for_each_interesting_fields!(parent, field, {
    field.do_something();
});

It will be expanded to something like

{ let field = parent.field1; { field.do_something(); }}
{ let field = parent.field2; { field.do_something(); }}
{ let field = parent.field3; { field.do_something(); }}

The point is, we isolate the field visiting from what we do for each field. This way we keep for_each_space implemented with macro, but the actual thing we do for each field can be done in plain Rust.

[qinsoon]

Writing hand-written tracing functions is tedious and error-prone.

Yeah, this was why we introduced the macros. But I also agree that macros may get new users confused. I was suggesting a straightforward solution, that is, removing the macros and use hand-written code. Then we think about how to make it easier for developers without using procedural macros.

[k-sareen]

In the end, we want to move away from cascading-ifs -- it has a lot of overheads. Like we've discussed, the ideal solution is probably specialization for the work packets.

[wks]

After a second thought, I think we may just revert to hand-written if-elseif-else. But instead of implementing it as one single Plan::trace_object, the implementation should be per-trace. For Immix, there will be two implementations: one for non-moving (fast) trace, and the other for defrag trace. Maybe there should be another one for transitive pinning. And the TraceKind will go away. This should partially address the issue #1314