Remove unsafe code related to get_allocator

[wks]

The following methods are labelled as unsafe:

impl<VM: VMBinding> Mutator<VM> {
    pub unsafe fn allocator(&self, selector: AllocatorSelector) -> &dyn Allocator<VM> {...}
    pub unsafe fn allocator_mut(&mut self, selector: AllocatorSelector) -> &mut dyn Allocator<VM> {...}
    pub unsafe fn allocator_impl<T: Allocator<VM>>(&self, selector: AllocatorSelector) -> &T {...}
    pub unsafe fn allocator_impl_mut<T: Allocator<VM>>(&mut self, selector: AllocatorSelector) -> &mut T {...}
}

impl<VM: VMBinding> Allocators<VM> {
    pub unsafe fn get_allocator(&self, selector: AllocatorSelector) -> &dyn Allocator<VM> {...}
    pub unsafe fn get_typed_allocator<T: Allocator<VM>>(&self, selector: AllocatorSelector) -> &T {...}
    pub unsafe fn get_allocator_mut(&mut self, selector: AllocatorSelector) -> &mut dyn Allocator<VM> {...}
    pub unsafe fn get_typed_allocator_mut<T: Allocator<VM>>(&mut self, selector: AllocatorSelector) -> &mut T {...}
}

And those functions are called in many places. (Actually only Allocators::get_allocator_mut is properly used. The rest are only used in wrappers or tests.)

• mutator prepare/release functions (6 call sites), such as immix_mutator_release. It gets the appropriate allocators and call release or reset functions.
• alloc and post_alloc functions (5 call sites) of Mutator. Users call alloc and post_alloc with AllocatorSemantics. The first thing those alloc functions do is looking up the proper allocator (as &dyn Allocator), and then it calls Allocator::alloc (with dynamic dispatch).
• Mutator::on_destroy. It iterates through all allocators and call on_mutator_destroy.

Why are they unsafe?

Those functions all serve the purpose of getting an allocator from a given AllocatorSelector. They can't guarantee to return an allocator because

1. An AllocatorSelector may not always be valid. It may select an allocator that does not exist for the current Plan.
2. The Allocators struct holds allocators in [MaybeUninit<T>; MAX_T_ALLOCATOR] where T is the allocator type. The raw array does not record the number of elements "pushed" into the array, and MaybeUninit has no way to check if it is initialized at run time.

If #1318 is fixed, the Allocators struct will be able to hold the concrete Allocator instances in vectors. Then we will be able to use Vec::get to return an Option<&allocator> which gracefully handles invalid AllocatorSelector and solves both problems above.

Impacts on allocation site.

Mutator::alloc and related functions currently contain the following call site:

    fn alloc(&mut self, size: usize, align: usize, offset: usize, semantics: AllocationSemantics) -> Address {
        let allocator = unsafe {
            self.allocators
                .get_allocator_mut(self.config.allocator_mapping[semantics])
        };

        allocator.alloc(size, align, offset)
    }

After the change, get_allocator_mut will return an Option<&dyn Allocator>. It will become

let allocator = self.allocators.get_allocator_mut(self.config.allocator_mapping[semantics]).unwrap();

This involves an extra unwrap() operation. But I assume the cost of this is trivial compared to the allocator_mapping look-up and the dynamic dispatching. Any VM binding that cares about performance should have inlined the allocation fast path, and used the BumpPointer directly instead of relying on those dynamism. In my opinion, it is not worth trade safety for such trivial performance gain in uninlined code.

Regarding mutator prepare/release functions

It could be possible to simply iterate through all Allocators using for loops (which becomes possible because we are now using Vec with no uninitialized elements) and call their prepare/release/end_of_gc methods. (Note that currently Mutator::on_destroy is doing this indirectly via Mutators::get_all_allocator_selectors().) This should be enough to handle most plans. And this should make the common_release_func introduced in #1308 easier to implement.

One exception is SemiSpace which needs to rebind the BumpAllocator with a different space. This can be handled specially.

[k-sareen]

One exception is SemiSpace which needs to rebind the BumpAllocator with a different space. This can be handled specially.

I'm not entirely sure what you're suggesting, but I use this trick for ART as well since I rebind the default allocators after the zygote has been forked for the first time (from an immix allocator pointing to the zygote space to whatever the collector requires for their default space).

[wks]

I'm not entirely sure what you're suggesting,

In simple terms, I am suggesting

fn common_release_mutator(...) {
    for allocator in mutator.all_allocators() {
        allocator.release();
    }
}

fn semispace_release_mutator(...) {
    mutator.allocators.bump_allocators[0].rebind(semispace.get_to_space());
    common_release_mutator(...);
}

So that common_release_mutator can be used by all plans except semispace (and maybe gencopy as well).

but I use this trick for ART as well since I rebind the default allocators after the zygote has been forked for the first time (from an immix allocator pointing to the zygote space to whatever the collector requires for their default space).

This behavior is interesting. Maybe it is specific to Zygote space. But it is interesting that we can implement it this way.

[k-sareen]

This behavior is interesting. Maybe it is specific to Zygote space. But it is interesting that we can implement it this way.

See here and here for examples for SemiSpace and Immix in my binding. The SemiSpace one is interesting since the allocator map is also being changed.

[qinsoon]

We can rethink about the safety in some of the methods, e.g. allowing panic does not mean the function is not safe. If we want to use Vec, we need to get #1318 resolved first.