Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce ArcFlexMut for Space references #965

Draft
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

qinsoon
Copy link
Member

@qinsoon qinsoon commented Sep 29, 2023

This PR is based on #949.

It introduces ArcFlexMut for some shared references where 1. their mutability is managed by the programmer, 2. mutability is hard to reason about statically, 3. using locks or RefCell/AtomicRefCell is not plausible for the sake of performance, and 4. the shared reference could be both statically typed and a dyn ref to a trait object, depending on where it is used. ArcFlexMut is a replacement for UnsafeCell. Basically ArcFlexMut can be used for both Plan and Space. This PR only uses ArcFlexMut for Space, as currently with the refactoring, we do not need to mutate on a plan.

In my preliminary evaluation, the PR should have no performance overhead over #949 on immix. I will rerun the benchmarks and provide results before we attempt to merge the PR.

Changes:

  • Introduce ArcFlexMut and use it for spaces. With the change, we always access the plan with an immutable reference, and no longer need UnsafeCell for the plan. We acquire mutable/immutable references to the spaces instead. This closes Remove cast_ref_to_mut related to Prepare and Release #852.
  • We no longer block for GC in Space::acquire(). This avoids holding a reference to the space during GC. acquire() now returns a result. For an error return value, the caller (the allocators) need to block for GC.
  • We use SFT to dispatch initialize_object_metadata() in post_alloc. With this change, we can remove Allocator::get_space(). This actually has performance improvement over the current code. This closes Use SFT for initialize_object_metadata #963.

@qinsoon
Copy link
Member Author

qinsoon commented Sep 29, 2023

Any comment and critics are welcome. I would need to know if this is on the right track to solve #852.

My general feeling is that our current code is quite messy when dealing with mutability and lifetime for Plan and Space, and this PR can be a good start. It uses Arc to manage the lifetime of spaces, allows us to acquire mutability to spaces and optionally detect possible data races. It also makes it easier to do further refactoring (such as the abusive use of atomics for interior mutability, #852 (comment), and owning region types #696).

Just let me know what you think. If this looks like a good direction to continue on, I will start solving issues for PRs
that this PR depends on (#957 and #949), and eventually get this merged.

Some plans do nothing when preparing mutators. We add a boolean flag so
that we do not create the PrepareMutator work packets in the first
place.

We also remove the function that prepares Mutator for Immix. It is
unnecessary to reset the ImmixAllocator of a mutator when preparing
mutator because the mutator's ImmixAllocator is not used during GC. When
a GC worker defragments the heap and promotes young objects, it uses the
ImmixAllocator instances in ImmixCopyContext or ImmixHybridCopyContext.

Fixes: mmtk#959
@qinsoon
Copy link
Member Author

qinsoon commented Oct 26, 2023

I added the following micro benchmark to test the performance of post_alloc.

    c.bench_function("post_alloc", |b| {
        b.iter(|| {
            api::mmtk_post_alloc(fixture.mutator, obj, 8, AllocationSemantics::Default);
        })
    });

Using SFT to dispatch initialize_object_metadata (this PR)

    fn post_alloc(
        &mut self,
        refer: ObjectReference,
        _bytes: usize,
        _allocator: AllocationSemantics,
    ) {
        unsafe { crate::mmtk::SFT_MAP.get_unchecked(refer.to_address::<VM>()) }
            .initialize_object_metadata(refer, true)
    }
Running benches/main.rs (target/release/deps/main-041e7708dfacb17d)
post_alloc              time:   [4.0283 ns 4.0297 ns 4.0316 ns]
Found 11 outliers among 100 measurements (11.00%)
  1 (1.00%) low severe
  1 (1.00%) low mild
  4 (4.00%) high mild
  5 (5.00%) high severe

Using allocator selector and invoke initialize_object_metadata on the space (master)

    fn post_alloc(
        &mut self,
        refer: ObjectReference,
        _bytes: usize,
        allocator: AllocationSemantics,
    ) {
        unsafe {
            self.allocators
                .get_allocator_mut(self.config.allocator_mapping[allocator])
        }
        .get_space()
        .initialize_object_metadata(refer, true)
    }
Running benches/main.rs (target/release/deps/main-9b17a5e12d50ed82)
post_alloc              time:   [4.4596 ns 4.4609 ns 4.4623 ns]
                        change: [+10.622% +10.701% +10.775%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 6 outliers among 100 measurements (6.00%)
  1 (1.00%) low mild
  2 (2.00%) high mild
  3 (3.00%) high severe

@qinsoon qinsoon added the PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) label Oct 26, 2023
@qinsoon qinsoon mentioned this pull request Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use SFT for initialize_object_metadata Remove cast_ref_to_mut related to Prepare and Release
1 participant