Plan::common and Space::common should return Option<T>.

[wks]

CommonPlan holds the common parts of most but not all plans, and CommonSpace is similar for spaces. Back in the days of JikesRVM-based MMTk, we used object-oriented programming, and they were abstract base classes.

In Rust MMTk, if a plan is based on CommonPlan, it contains a field of type CommonPlan. The Plan trait has the common() method:

pub trait Plan: ... {
    fn common(&self) -> &CommonPlan<Self::VM> {
        panic!("Common Plan not handled!")
    }
}

It returns a reference &CommonPlan, and will panic if the plan is not based on CommonPlan.

Similarly, a space may contain a CommonSpace field, and the Space trait has the common()` method, too.

pub trait Space: ... {
    fn common(&self) -> &CommonSpace<VM>;
}

Spaces that are not based on CommonSpace (notably MallocSpace) will panic if common() is called.

The problem

While this approach is convenient, it gives the developers a false impression that every Plan has a CommonPlan and every Space has a CommonSpace. This can lead to bugs, and in fact has already caused a bug where we iterate through all spaces and call .common() without checking if a space is really based on CommonSpace.

To be safe, the common method should return Option<&CommonPlan> or Option<&CommonSpace>. This gives the caller a chance to handle None specially if the plan/space is not based on CommonPlan/CommonSpace.

Use cases of Plan::common()

There are two use cases of Plan::common()

One is create_space_mapping.

pub(crate) fn create_space_mapping<VM: VMBinding>(
    mut reserved: ReservedAllocators,
    include_common_plan: bool,
    plan: &'static dyn Plan<VM = VM>,
) -> Vec<(AllocatorSelector, &'static dyn Space<VM>)> {
    // ...
        if include_common_plan {
        vec.push((
            AllocatorSelector::BumpPointer(reserved.n_bump_pointer),
            plan.common().get_immortal(),
        ));
        reserved.n_bump_pointer += 1;
        vec.push((
            AllocatorSelector::LargeObject(reserved.n_large_object),
            plan.common().get_los(),
        ));
        reserved.n_large_object += 1;
        // TODO: This should be freelist allocator once we use marksweep for nonmoving space.
        vec.push((
            AllocatorSelector::BumpPointer(reserved.n_bump_pointer),
            plan.common().get_nonmoving(),
        ));
        reserved.n_bump_pointer += 1;
    }
    // .../
}

Here plan is a &dyn, so it needs common() to get the CommonPlan. If common() returns Option<&CommonPlan>, it can use match against Some and None to know if it is based on CommonPlan. But this function uses another argument, namely include_common_plan: bool, to know if it should call .common() or not. This argument is true for all plans except NoGC.

The other use case is StickyImmix::trace_object_nursery(&self, ...). It uses self.immix.common().get_los() to forward trace_object_nursery to the LOS. Because self is exactly StickyImmix (i.e. not &dyn), it can just use self.immix.common.get_los(), instead.

Use cases of Space::common()

The use cases of Space::common() fall into three categories.

The first is self.common() for concrete self types. These can be replace with self.common.

The second is CommonGenPlan. Since CommonGenPlan is also a trait, it needs an abstract method to access the CommonPlan. The only call site is in CommonGenPlan::collection_required, and it uses the space descriptor in CommonSpace to test if a given space is the nursery (i.e. comparing spaces by identities). Currently all generational plans are based on CommonPlan.

The third is the methods of the Space trait itself.

Particularly, Space::acquire accesses the CommonSpace to do mmap, use the acquire lock and look up space descriptors (for debugging) when GC is triggered. LockFreeImmortalSpace and MallocSpace are not based on CommonSpace. LockFreeImmortalSpace is only used in NoGC and cannot trigger GC. MallocSpace triggers GC eagerly so that it never calls Space::acquire to block for GC. Since Space::acquire depends so much on CommonSpace, it is arguable that acquire should be a method of CommonSpace rather than a trait method. In JikesRVM, spaces, such as ImmixSpace, inherit Space directly, and there is no CommonSpace.

Space::in_space uses CommonSpace to get the space descriptor. MallocSpace overrides this method and uses the VO bit instead. LockFreeImmortalSpace cannot do GC, and this may be why it doesn't care about the in_space method.

Space::ensure_mapped is also about mmap, and it uses CommonSpace for start and extent. Space::reserved_pages accesses the SideMetadataContext. Space::get_name(), Space::get_descriptor(), ... are delegated to CommonSpace. Spaces that are not based on CommonSpace override those method and implement differently.

Among those methods, the only method that is on the fast path is Space::in_space. It is used in trace_object to select the space (unless SFT is used).

Refactoring

Plan::common() should be straightforward. It is never used on fast paths, and its return value can be changed to Option<T> directly.

Space::common() needs more careful thoughts. We can move Space::acquire() to CommonSpace so that spaces that are not based on CommonSpace cannot call self.acquire(). For other methods, it may make sense to delegate to CommonSpace, too, instead of relying on default implementations in the Space trait that depend on a non-failing common() call.

[k-sareen]

See also: #1173