Arguments of prepare, release and end_of_gc of each space

[wks]

Each space has prepare, release and optionally end_of_gc methods. But their arguments differ.

Prepare:

• CopySpace: pub fn prepare(&self, from_space: bool)
• MallocSpace: pub fn prepare(&mut self, full_heap: bool)
• MarkCompactSpace: pub fn prepare(&self) {}
• ImmixSpace: pub fn prepare(&mut self, major_gc: bool, plan_stats: StatsForDefrag)
• LargeObjectSpace: pub fn prepare(&mut self, full_heap: bool)
• ImmortalSpace: pub fn prepare(&mut self)
• VMSpace: pub fn prepare(&mut self)

The CopySpace sets its self.from_space flag inside prepare(from_space).

Other spaces usually reset their metadata or set their internal states in prepare. Different spaces need different information to do so. For example, some spaces are sensitive to whether the current GC is full-heap. ImmixSpace, specifically, will need to determine the available_clean_pages_for_defrag using statistics from the Plan (which can be Immix, GenImmix or StickyImmix). Interestingly, the decision of whether to do defrag GC is made in schedule_immix_full_heap_collection which is called in the ScheduleCollection work packet.

It is annoying that different spaces need different information from the Plan. A PR #1315 attempted to "unify" the prepare/release/end_of_gc methods of different spaces. But it only made the arguments dynamically typed.

Similar is true for release. LargeObjectSpace and ImmixSpace take an extra full_heap: bool parameter, while other spaces only needs &self.

And end_of_gc is only needed for ImmixSpace, MarkSweepSpace.

What should we do?

I am not advocating for making changes soon. We will probably do nothing for now, but it's interesting to look at alternatives.

Do nothing

By doing nothing, we admit that each space needs different information in prepare and release, and may or may not need end_of_gc. Then each Plan has the responsibility to know exactly what their spaces need and provide precisely what information needed to do prepare/release/end_of_gc.

The good part of this is that the information is precise. Plans and policies have maximum and the most precise control of what information to pass between the plan and the policy.

The bad part is of course being a bit complicated.

Unify those functions

An argument for unifying those functions is that plan is a configuration/composition of spaces. If a plan has multiple spaces, it should give each space a chance to prepare. The good part of this is that it makes the system uniform.

But there are two questions remain to be answered.

1. For SemiSpace and GenCopy, the action of "swapping the roles of from/to spaces" is not a per-space decision, but a decision made at the plan level. This somewhat conflicts with the idea of "plan is a configuration/composition of spaces" because there does exist code in the plan, making it impossible to, for example, write a plan as a YAML file.
2. What information does all the spaces need? It is hard to work out a union of all information needed by all spaces, at present and in the future. It may be tempting to simply give spaces access to the Plan, or &dyn Plan. But each Plan has specific information, too. In the end, each space may still need to downcast it to some &dyn SomePlanTrait such as &dyn AbstractImmixBasedPlan. Although it is slightly better than &dyn Any, it's still a bit ugly.

Another argument for unifying those functions is that the composition of plans can be dynamic.

For example, with the introduction of #1308, the CommonSpace may have one of three candidates (ImmixSpace, MarkSweepSpace or ImmortalSpace) as the non-moving space. Some of them needs prepare/release/end_of_gc, while others don't. Some need more arguments, while others need fewer. Currently #1308 uses if-else or conditional compilation attributes to control the behaviors, and we already see many decisions become more complicated, including the plan-level prepare/release/end_of_gc and the prepare/release of mutators. A refactored version https://github.com/qinsoon/mmtk-core/compare/nonmoving-space-options...wks:mmtk-core:feature/nonmoving-space-options-trait?expand=1 attempted to use the visitor pattern to remove the if-else, but it assumes that we only needs one parameter, full_heap: bool, for the prepare method, which is not 100% general. Ideally, each space should know how to prepare itself.

Separating plan-level prepare and space-level prepare.

We may separate operations such as swapping the role of from/to spaces and the operations such as clearing side metadata.

By doing so, the Prepare work packet may first call mmtk.plan.prepare(), and then directly go through each space of a plan:

impl<C: GCWorkContext> GCWork<C::VM> for Prepare<C> {
    fn do_work(&mut self, worker: &mut GCWorker<C::VM>, mmtk: &'static MMTK<C::VM>) {
        trace!("Prepare Global");
        let plan_mut: &mut C::PlanType = unsafe { &mut *(self.plan as *const _ as *mut _) };  // Fix me.
        plan_mut.prepare(worker.tls);

        self.plan.for_each_space(|space| {  // Defined in the `HasSpaces` trait, generated by #[derive(HasSpaces)]
            space.prepare(plan);
        });

This will be maximally general.

For SemiSpace, it can let CopySpace define a separate fn set_is_from_space(&self, from_space: bool) method, and Semispace;:prepare will call it on both copyspace0 and copyspace1. Other plans probably do nothing in its own Plan::prepare. They doesn't need to delegate to the prepare methods of its spaces. The Prepare work packet will do it.

Then the Space.prepare will have access to the Plan. If a space is sensitive to whether the current GC is full-heap, it calls

let is_full_heap = !plan.generational.is_some_and(|gen| gen.is_current_gc_nursery());

And all statistics will be available from the plan.

It seems to be a general solution, although it needs quite a lot of refactoring.

[qinsoon]

Similar is true for release. LargeObjectSpace and ImmixSpace take an extra full_heap: bool parameter, while other spaces only needs &self.
And end_of_gc is only needed for ImmixSpace, MarkSweepSpace.

Some of the differences are actually accidental. For example, ImmixSpace::end_of_gc returns a boolean to tell if the GC was a defrag GC or not, just because it is convenient. There could be other ways for a plan to get this info, e.g. getting self.immix_space.in_defrag() before calling self.immix_space.end_of_gc.

For SemiSpace and GenCopy, the action of "swapping the roles of from/to spaces" is not a per-space decision, but a decision made at the plan level. This somewhat conflicts with the idea of "plan is a configuration/composition of spaces" because there does exist code in the plan, making it impossible to, for example, write a plan as a YAML file.

I would say plan is an orchestration of spaces. A plan needs to know how to get those spaces to work together, rather than just creating those spaces and let them figure it out.

What information does all the spaces need? It is hard to work out a union of all information needed by all spaces, at present and in the future.

It seems undesirable to put all the information (unrelated information) together.

---

If we ever want to 'unify' those functions, I think we have to use some form of dynamism such as Box<dyn Any>, or an enum that covers all the cases.

However, we don't have to 'unify' those functions. We can just argue that each space has its own implementation of those routines (prepare/release/end_of_gc). We do not attempt to access those functions in a unified way.

[qinsoon]

I think #1315 is a viable approach if we want to 'unify' those functions to trait Space. But I also agree that those functions are so policy-specific and we should not put them into trait Space at all.

[qinsoon]

plan_mut.prepare(worker.tls);

self.plan.for_each_space(|space| { // Defined in the HasSpaces trait, generated by #[derive(HasSpaces)]
space.prepare(plan);
});

I thought about a similar approach:

self.immix_space.set_defrag_stats(...); // this is specific to certain policies
self.immix_space.prepare(...); // this is unified

This 'unifies' the prepare function, but some policies need another 'prepare' function. I don't think it actually achieves what we want.

[wks]

Some of the differences are actually accidental. For example, ImmixSpace::end_of_gc returns a boolean to tell if the GC was a defrag GC or not, just because it is convenient. There could be other ways for a plan to get this info, e.g. getting self.immix_space.in_defrag() before calling self.immix_space.end_of_gc.

Agreed.

For SemiSpace and GenCopy, the action of "swapping the roles of from/to spaces" is not a per-space decision, but a decision made at the plan level. This somewhat conflicts with the idea of "plan is a configuration/composition of spaces" because there does exist code in the plan, making it impossible to, for example, write a plan as a YAML file.

I would say plan is an orchestration of spaces. A plan needs to know how to get those spaces to work together, rather than just creating those spaces and let them figure it out.

That's right. The plan is definitely more than just a spatial composition of spaces. I updated this Issue a bit and added some discussion of separating things like "swapping from/to spaces" and "letting each space prepare".

What information does all the spaces need? It is hard to work out a union of all information needed by all spaces, at present and in the future.

It seems undesirable to put all the information (unrelated information) together.

If we ever want to 'unify' those functions, I think we have to use some form of dynamism such as Box<dyn Any>, or an enum that covers all the cases.

As for now, all the arguments ultimately comes from the Plan, so I think &dyn Plan should suffice (for now).

However, we don't have to 'unify' those functions. We can just argument that each space has its own implementation of those routines (prepare/release/end_of_gc). We do not attempt to access those functions in a unified way.

This basically implies sticking to the status quo, and let each concrete plan call concrete prepare/release methods of its spaces. I am OK with this (for now).