Let MarkSweepSpace use BlockPageResource (#1150)

Let `MarkSweepSpace` use the `BlockPageResource` instead of the raw
`FreeListPageResource`. This solves a problem where multiple GC workers
contend for the mutex `FreeListPageResource::sync` when calling
`FreeListPageResource::release_pages` concurrently during the `Release`
stage.

We flush the `BlockPageResource` in `MarkSweepSpace::end_of_gc`. To
monitor the performance, we added a pair of USDT trace points before and
after calling `Plan.end_of_gc`. We also count the invocation of
`end_of_gc` into the GC time.

We changed `BlockQueue` so that it uses `MaybeUninit` to hold
uninitialized elements instead of relying on
`B::from_aligned_address(Address::ZERO)` because the `Block` type used
by `MarkSweepSpace` is based on `NonZeroUsize`, and cannot be zero.

Fixes: #1145

Author: wks

src/policy/marksweepspace/native_ms/block.rs

@@ -4,6 +4,7 @@ use atomic::Ordering;
 
 use super::BlockList;
 use super::MarkSweepSpace;
+use crate::util::constants::LOG_BYTES_IN_PAGE;
 use crate::util::heap::chunk_map::*;
 use crate::util::linear_scan::Region;
 use crate::vm::ObjectModel;
@@ -48,6 +49,9 @@ impl Region for Block {
 }
 
 impl Block {
+    /// Log pages in block
+    pub const LOG_PAGES: usize = Self::LOG_BYTES - LOG_BYTES_IN_PAGE as usize;
+
     pub const METADATA_SPECS: [SideMetadataSpec; 7] = [
         Self::MARK_TABLE,
         Self::NEXT_BLOCK_TABLE,

src/policy/marksweepspace/native_ms/global.rs

@@ -7,7 +7,7 @@ use crate::{
     scheduler::{GCWorkScheduler, GCWorker},
     util::{
         copy::CopySemantics,
-        heap::{FreeListPageResource, PageResource},
+        heap::{BlockPageResource, PageResource},
         metadata::{self, side_metadata::SideMetadataSpec, MetadataSpec},
         ObjectReference,
     },
@@ -64,7 +64,7 @@ pub enum BlockAcquireResult {
 /// | GC - End of GC | -                                               | Merge the temp global lists                  | -         |
 pub struct MarkSweepSpace<VM: VMBinding> {
     pub common: CommonSpace<VM>,
-    pr: FreeListPageResource<VM>,
+    pr: BlockPageResource<VM, Block>,
     /// Allocation status for all chunks in MS space
     chunk_map: ChunkMap,
     /// Work packet scheduler
@@ -80,6 +80,8 @@ pub struct MarkSweepSpace<VM: VMBinding> {
     abandoned_in_gc: Mutex<AbandonedBlockLists>,
 }
 
+unsafe impl<VM: VMBinding> Sync for MarkSweepSpace<VM> {}
+
 pub struct AbandonedBlockLists {
     pub available: BlockLists,
     pub unswept: BlockLists,
@@ -278,9 +280,19 @@ impl<VM: VMBinding> MarkSweepSpace<VM> {
         let common = CommonSpace::new(args.into_policy_args(false, false, local_specs));
         MarkSweepSpace {
             pr: if is_discontiguous {
-                FreeListPageResource::new_discontiguous(vm_map)
+                BlockPageResource::new_discontiguous(
+                    Block::LOG_PAGES,
+                    vm_map,
+                    scheduler.num_workers(),
+                )
             } else {
-                FreeListPageResource::new_contiguous(common.start, common.extent, vm_map)
+                BlockPageResource::new_contiguous(
+                    Block::LOG_PAGES,
+                    common.start,
+                    common.extent,
+                    vm_map,
+                    scheduler.num_workers(),
+                )
             },
             common,
             chunk_map: ChunkMap::new(),
@@ -347,14 +359,19 @@ impl<VM: VMBinding> MarkSweepSpace<VM> {
 
         #[cfg(debug_assertions)]
         from.assert_empty();
+
+        // BlockPageResource uses worker-local block queues to eliminate contention when releasing
+        // blocks, similar to how the MarkSweepSpace caches blocks in `abandoned_in_gc` before
+        // returning to the global pool.  We flush the BlockPageResource, too.
+        self.pr.flush_all();
     }
 
     /// Release a block.
     pub fn release_block(&self, block: Block) {
         self.block_clear_metadata(block);
 
         block.deinit();
-        self.pr.release_pages(block.start());
+        self.pr.release_block(block);
     }
 
     pub fn block_clear_metadata(&self, block: Block) {

src/scheduler/scheduler.rs

@@ -530,6 +530,12 @@ impl<VM: VMBinding> GCWorkScheduler<VM> {
         // Tell GC trigger that GC ended - this happens before we resume mutators.
         mmtk.gc_trigger.policy.on_gc_end(mmtk);
 
+        // All other workers are parked, so it is safe to access the Plan instance mutably.
+        probe!(mmtk, plan_end_of_gc_begin);
+        let plan_mut: &mut dyn Plan<VM = VM> = unsafe { mmtk.get_plan_mut() };
+        plan_mut.end_of_gc(worker.tls);
+        probe!(mmtk, plan_end_of_gc_end);
+
         // Compute the elapsed time of the GC.
         let start_time = {
             let mut gc_start_time = worker.mmtk.state.gc_start_time.borrow_mut();
@@ -565,10 +571,6 @@ impl<VM: VMBinding> GCWorkScheduler<VM> {
             );
         }
 
-        // All other workers are parked, so it is safe to access the Plan instance mutably.
-        let plan_mut: &mut dyn Plan<VM = VM> = unsafe { mmtk.get_plan_mut() };
-        plan_mut.end_of_gc(worker.tls);
-
         #[cfg(feature = "extreme_assertions")]
         if crate::util::slot_logger::should_check_duplicate_slots(mmtk.get_plan()) {
             // reset the logging info at the end of each GC

src/util/heap/blockpageresource.rs

@@ -8,10 +8,12 @@ use crate::util::heap::pageresource::CommonPageResource;
 use crate::util::heap::space_descriptor::SpaceDescriptor;
 use crate::util::linear_scan::Region;
 use crate::util::opaque_pointer::*;
+use crate::util::rust_util::zeroed_alloc::new_zeroed_vec;
 use crate::vm::*;
 use atomic::Ordering;
 use spin::RwLock;
 use std::cell::UnsafeCell;
+use std::mem::MaybeUninit;
 use std::sync::atomic::AtomicUsize;
 use std::sync::Mutex;
 
@@ -179,17 +181,29 @@ impl<VM: VMBinding, B: Region> BlockPageResource<VM, B> {
 
 /// A block list that supports fast lock-free push/pop operations
 struct BlockQueue<B: Region> {
+    /// The number of elements in the queue.
     cursor: AtomicUsize,
-    data: UnsafeCell<Vec<B>>,
+    /// The underlying data storage.
+    ///
+    /// -   `UnsafeCell<T>`: It may be accessed by multiple threads.
+    /// -   `Box<[T]>`: It holds an array allocated on the heap.  It cannot be resized, but can be
+    ///     replaced with another array as a whole.
+    /// -   `MaybeUninit<T>`: It may contain uninitialized elements.
+    ///
+    /// The implementaiton of `BlockQueue` must ensure there is no data race, and it never reads
+    /// uninitialized elements.
+    data: UnsafeCell<Box<[MaybeUninit<B>]>>,
 }
 
 impl<B: Region> BlockQueue<B> {
     /// Create an array
     fn new() -> Self {
-        let default_block = B::from_aligned_address(Address::ZERO);
+        let zeroed_vec = unsafe { new_zeroed_vec(Self::CAPACITY) };
+        let boxed_slice = zeroed_vec.into_boxed_slice();
+        let data = UnsafeCell::new(boxed_slice);
         Self {
             cursor: AtomicUsize::new(0),
-            data: UnsafeCell::new(vec![default_block; Self::CAPACITY]),
+            data,
         }
     }
 }
@@ -199,14 +213,14 @@ impl<B: Region> BlockQueue<B> {
 
     /// Get an entry
     fn get_entry(&self, i: usize) -> B {
-        unsafe { (*self.data.get())[i] }
+        unsafe { (*self.data.get())[i].assume_init() }
     }
 
     /// Set an entry.
     ///
     /// It's unsafe unless the array is accessed by only one thread (i.e. used as a thread-local array).
     unsafe fn set_entry(&self, i: usize, block: B) {
-        (*self.data.get())[i] = block
+        (*self.data.get())[i].write(block);
     }
 
     /// Non-atomically push an element.

src/util/heap/freelistpageresource.rs

@@ -320,6 +320,14 @@ impl<VM: VMBinding> FreeListPageResource<VM> {
         self.common.release_discontiguous_chunks(chunk);
     }
 
+    /// Release pages previously allocated by `alloc_pages`.
+    ///
+    /// Warning: This method acquires the mutex `self.sync`.  If multiple threads release pages
+    /// concurrently, the lock contention will become a performance bottleneck.  This is especially
+    /// problematic for plans that sweep objects in bulk in the `Release` stage.  Spaces except the
+    /// large object space are recommended to use [`BlockPageResource`] whenever possible.
+    ///
+    /// [`BlockPageResource`]: crate::util::heap::blockpageresource::BlockPageResource
     pub fn release_pages(&self, first: Address) {
         debug_assert!(conversions::is_page_aligned(first));
         let mut sync = self.sync.lock().unwrap();

tools/tracing/README.md

@@ -38,6 +38,8 @@ Currently, the core provides the following tracepoints.
     argument is the length of the string.
 -   `mmtk:alloc_slow_once_start()`: the allocation slow path starts.
 -   `mmtk:alloc_slow_once_end()`: the allocation slow path ends.
+-   `mmtk:plan_end_of_gc_begin()`: before executing `Plan::end_of_gc`.
+-   `mmtk:plan_end_of_gc_end()`: after executing `Plan::end_of_gc`.
 
 ## Tracing tools
 

tools/tracing/timeline/capture.bt

@@ -79,4 +79,16 @@ usdt:$MMTK:mmtk:process_slots {
     }
 }
 
+usdt:$MMTK:mmtk:plan_end_of_gc_begin {
+    if (@enable_print) {
+        printf("plan_end_of_gc,B,%d,%lu\n", tid, nsecs);
+    }
+}
+
+usdt:$MMTK:mmtk:plan_end_of_gc_end {
+    if (@enable_print) {
+        printf("plan_end_of_gc,E,%d,%lu\n", tid, nsecs);
+    }
+}
+
 // vim: ft=bpftrace ts=4 sw=4 sts=4 et