Remove GCRequester, move related code to GCTrigger (#1405)

This PR moves GC triggering related code to `GCTrigger`, including:
* The old `GCRequester` which handles the request for GCs.
* The check for `is_collection_enabled` in `memory_manager` and
`Space::acquire`.
* Code for user collection request and internal collection request in
`MMTK`.

Author: qinsoon

src/memory_manager.rs

@@ -543,7 +543,7 @@ pub fn gc_poll<VM: VMBinding>(mmtk: &MMTK<VM>, tls: VMMutatorThread) {
         "gc_poll() can only be called by a mutator thread."
     );
 
-    if VM::VMCollection::is_collection_enabled() && mmtk.gc_trigger.poll(false, None) {
+    if mmtk.gc_trigger.poll(false, None) {
         debug!("Collection required");
         if !mmtk.state.is_initialized() {
             panic!("GC is not allowed here: collection is not initialized (did you call initialize_collection()?).");

src/mmtk.rs

@@ -1,6 +1,5 @@
 //! MMTk instance.
 use crate::global_state::{GcStatus, GlobalState};
-use crate::plan::gc_requester::GCRequester;
 use crate::plan::CreateGeneralPlanArgs;
 use crate::plan::Plan;
 use crate::policy::sft_map::{create_sft_map, SFTMap};
@@ -123,7 +122,6 @@ pub struct MMTK<VM: VMBinding> {
     #[cfg(feature = "extreme_assertions")]
     pub(crate) slot_logger: SlotLogger<VM::VMSlot>,
     pub(crate) gc_trigger: Arc<GCTrigger<VM>>,
-    pub(crate) gc_requester: Arc<GCRequester<VM>>,
     pub(crate) stats: Arc<Stats>,
     #[cfg(feature = "sanity")]
     inside_sanity: AtomicBool,
@@ -156,11 +154,9 @@ impl<VM: VMBinding> MMTK<VM> {
 
         let state = Arc::new(GlobalState::default());
 
-        let gc_requester = Arc::new(GCRequester::new(scheduler.clone()));
-
         let gc_trigger = Arc::new(GCTrigger::new(
             options.clone(),
-            gc_requester.clone(),
+            scheduler.clone(),
             state.clone(),
         ));
 
@@ -229,7 +225,6 @@ impl<VM: VMBinding> MMTK<VM> {
             #[cfg(feature = "analysis")]
             analysis_manager: Arc::new(AnalysisManager::new(stats.clone())),
             gc_trigger,
-            gc_requester,
             stats,
         }
     }
@@ -422,44 +417,22 @@ impl<VM: VMBinding> MMTK<VM> {
         force: bool,
         exhaustive: bool,
     ) -> bool {
-        use crate::vm::Collection;
-        if !self.get_plan().constraints().collects_garbage {
-            warn!("User attempted a collection request, but the plan can not do GC. The request is ignored.");
-            return false;
-        }
-
-        if force || !*self.options.ignore_system_gc && VM::VMCollection::is_collection_enabled() {
-            info!("User triggering collection");
-            if exhaustive {
-                if let Some(gen) = self.get_plan().generational() {
-                    gen.force_full_heap_collection();
-                }
-            }
-
-            self.state
-                .user_triggered_collection
-                .store(true, Ordering::Relaxed);
-            self.gc_requester.request();
+        if self
+            .gc_trigger
+            .handle_user_collection_request(force, exhaustive)
+        {
+            use crate::vm::Collection;
             VM::VMCollection::block_for_gc(tls);
-            return true;
+            true
+        } else {
+            false
         }
-
-        false
     }
 
     /// MMTK has requested stop-the-world activity (e.g., stw within a concurrent gc).
-    // This is not used, as we do not have a concurrent plan.
     #[allow(unused)]
     pub fn trigger_internal_collection_request(&self) {
-        self.state
-            .last_internal_triggered_collection
-            .store(true, Ordering::Relaxed);
-        self.state
-            .internal_triggered_collection
-            .store(true, Ordering::Relaxed);
-        // TODO: The current `GCRequester::request()` is probably incorrect for internally triggered GC.
-        // Consider removing functions related to "internal triggered collection".
-        self.gc_requester.request();
+        self.gc_trigger.trigger_internal_collection_request();
     }
 
     /// Get a reference to the plan.

src/plan/gc_requester.rs

@@ -16,8 +16,6 @@
 mod barriers;
 pub use barriers::BarrierSelector;
 
-pub(crate) mod gc_requester;
-
 mod gc_work;
 
 mod global;

src/plan/mod.rs

@@ -115,9 +115,7 @@ pub trait Space<VM: VMBinding>: 'static + SFT + Sync + Downcast {
 
         // Should we poll before acquring pages from page resources so that it can trigger a GC?
         // - If tls is collector, we cannot attempt a GC.
-        // - If gc is disabled, we cannot attempt a GC.
-        let should_poll =
-            VM::VMActivePlan::is_mutator(tls) && VM::VMCollection::is_collection_enabled();
+        let should_poll = VM::VMActivePlan::is_mutator(tls);
 
         // If we should poll, do it now.  Record if it has triggered a GC.
         // If we should not poll, GC is not triggered.

src/policy/space.rs

@@ -122,7 +122,7 @@ impl<VM: VMBinding> GCWorkScheduler<VM> {
         self.affinity.resolve_affinity(thread);
     }
 
-    /// Request a GC to be scheduled.  Called by mutator via `GCRequester`.
+    /// Request a GC to be scheduled.  Called by mutator via `GCTrigger`.
     pub(crate) fn request_schedule_collection(&self) {
         debug!("A mutator is sending GC-scheduling request to workers...");
         self.worker_monitor.make_request(WorkerGoal::Gc);
@@ -642,7 +642,7 @@ impl<VM: VMBinding> GCWorkScheduler<VM> {
     }
 
     pub fn notify_mutators_paused(&self, mmtk: &'static MMTK<VM>) {
-        mmtk.gc_requester.clear_request();
+        mmtk.gc_trigger.clear_request();
         let first_stw_bucket = &self.work_buckets[WorkBucketStage::FIRST_STW_STAGE];
         debug_assert!(!first_stw_bucket.is_open());
         // Note: This is the only place where a bucket is opened without having all workers parked.

src/scheduler/scheduler.rs

@@ -1,16 +1,17 @@
 use atomic::Ordering;
 
 use crate::global_state::GlobalState;
-use crate::plan::gc_requester::GCRequester;
 use crate::plan::Plan;
 use crate::policy::space::Space;
+use crate::scheduler::GCWorkScheduler;
 use crate::util::constants::BYTES_IN_PAGE;
 use crate::util::conversions;
 use crate::util::options::{GCTriggerSelector, Options, DEFAULT_MAX_NURSERY, DEFAULT_MIN_NURSERY};
+use crate::vm::Collection;
 use crate::vm::VMBinding;
 use crate::MMTK;
 use std::mem::MaybeUninit;
-use std::sync::atomic::AtomicUsize;
+use std::sync::atomic::{AtomicBool, AtomicUsize};
 use std::sync::Arc;
 
 /// GCTrigger is responsible for triggering GCs based on the given policy.
@@ -23,15 +24,18 @@ pub struct GCTrigger<VM: VMBinding> {
     plan: MaybeUninit<&'static dyn Plan<VM = VM>>,
     /// The triggering policy.
     pub policy: Box<dyn GCTriggerPolicy<VM>>,
-    gc_requester: Arc<GCRequester<VM>>,
+    /// Set by mutators to trigger GC.  It is atomic so that mutators can check if GC has already
+    /// been requested efficiently in `poll` without acquiring any mutex.
+    request_flag: AtomicBool,
+    scheduler: Arc<GCWorkScheduler<VM>>,
     options: Arc<Options>,
     state: Arc<GlobalState>,
 }
 
 impl<VM: VMBinding> GCTrigger<VM> {
     pub fn new(
         options: Arc<Options>,
-        gc_requester: Arc<GCRequester<VM>>,
+        scheduler: Arc<GCWorkScheduler<VM>>,
         state: Arc<GlobalState>,
     ) -> Self {
         GCTrigger {
@@ -58,7 +62,8 @@ impl<VM: VMBinding> GCTrigger<VM> {
                 }
             },
             options,
-            gc_requester,
+            request_flag: AtomicBool::new(false),
+            scheduler,
             state,
         }
     }
@@ -72,6 +77,28 @@ impl<VM: VMBinding> GCTrigger<VM> {
         unsafe { self.plan.assume_init() }
     }
 
+    /// Request a GC.  Called by mutators when polling (during allocation) and when handling user
+    /// GC requests (e.g. `System.gc();` in Java).
+    fn request(&self) {
+        if self.request_flag.load(Ordering::Relaxed) {
+            return;
+        }
+
+        if !self.request_flag.swap(true, Ordering::Relaxed) {
+            // `GCWorkScheduler::request_schedule_collection` needs to hold a mutex to communicate
+            // with GC workers, which is expensive for functions like `poll`.  We use the atomic
+            // flag `request_flag` to elide the need to acquire the mutex in subsequent calls.
+            probe!(mmtk, gc_requested);
+            self.scheduler.request_schedule_collection();
+        }
+    }
+
+    /// Clear the "GC requested" flag so that mutators can trigger the next GC.
+    /// Called by a GC worker when all mutators have come to a stop.
+    pub fn clear_request(&self) {
+        self.request_flag.store(false, Ordering::Relaxed);
+    }
+
     /// This method is called periodically by the allocation subsystem
     /// (by default, each time a page is consumed), and provides the
     /// collector with an opportunity to collect.
@@ -80,7 +107,11 @@ impl<VM: VMBinding> GCTrigger<VM> {
     /// * `space_full`: Space request failed, must recover pages within 'space'.
     /// * `space`: The space that triggered the poll. This could `None` if the poll is not triggered by a space.
     pub fn poll(&self, space_full: bool, space: Option<&dyn Space<VM>>) -> bool {
-        let plan = unsafe { self.plan.assume_init() };
+        if !VM::VMCollection::is_collection_enabled() {
+            return false;
+        }
+
+        let plan = self.plan();
         if self
             .policy
             .is_gc_required(space_full, space.map(|s| SpaceStats::new(s)), plan)
@@ -96,12 +127,62 @@ impl<VM: VMBinding> GCTrigger<VM> {
                 plan.get_reserved_pages(),
                 plan.get_total_pages(),
             );
-            self.gc_requester.request();
+            self.request();
             return true;
         }
         false
     }
 
+    /// This method is called when the user manually requests a collection, such as `System.gc()` in Java.
+    /// Returns true if a collection is actually requested.
+    ///
+    /// # Arguments
+    /// * `force`: If true, we force a collection regardless of the settings. If false, we only trigger a collection if the settings allow it.
+    /// * `exhaustive`: If true, we try to make the collection exhaustive (e.g. full heap collection). If false, the collection kind is determined internally.
+    pub fn handle_user_collection_request(&self, force: bool, exhaustive: bool) -> bool {
+        if !self.plan().constraints().collects_garbage {
+            warn!("User attempted a collection request, but the plan can not do GC. The request is ignored.");
+            return false;
+        }
+
+        if force || !*self.options.ignore_system_gc && VM::VMCollection::is_collection_enabled() {
+            info!("User triggering collection");
+            // TODO: this may not work reliably. If a GC has been triggered, this will not force it to be a full heap GC.
+            if exhaustive {
+                if let Some(gen) = self.plan().generational() {
+                    gen.force_full_heap_collection();
+                }
+            }
+
+            self.state
+                .user_triggered_collection
+                .store(true, Ordering::Relaxed);
+            self.request();
+            return true;
+        }
+
+        false
+    }
+
+    /// MMTK has requested stop-the-world activity (e.g., stw within a concurrent gc).
+    // TODO: We should use this for concurrent GC. E.g. in concurrent Immix, when the initial mark is done, we
+    // can use this function to immediately trigger the final mark pause. The current implementation uses
+    // normal collection_required check, which may delay the final mark unnecessarily.
+    #[allow(unused)]
+    pub fn trigger_internal_collection_request(&self) {
+        self.state
+            .last_internal_triggered_collection
+            .store(true, Ordering::Relaxed);
+        self.state
+            .internal_triggered_collection
+            .store(true, Ordering::Relaxed);
+        // TODO: The current `request()` is probably incorrect for internally triggered GC.
+        // Consider removing functions related to "internal triggered collection".
+        self.request();
+        // TODO: Make sure this function works correctly for concurrent GC.
+        unimplemented!()
+    }
+
     pub fn should_do_stress_gc(&self) -> bool {
         Self::should_do_stress_gc_inner(&self.state, &self.options)
     }

src/util/heap/gc_trigger.rs

@@ -138,8 +138,8 @@ usdt:$MMTK:mmtk:concurrent_trace_objects {
     }
 }
 
-usdt:$MMTK:mmtk:gcrequester_request {
-    printf("gcrequester_request,i,%d,%lu\n", tid, nsecs);
+usdt:$MMTK:mmtk:gc_requested {
+    printf("gc_requested,i,%d,%lu\n", tid, nsecs);
 }
 
 usdt:$MMTK:mmtk:add_schedule_collection_packet {

tools/tracing/timeline/capture.bt

@@ -161,7 +161,7 @@ def enrich_event(self, name, ph, tid, ts, result, args):
                     "stage": int(args[0]),
                 }
 
-            case "gcrequester_request":
+            case "gc_requested":
                 result["tid"] = 1
 
             case _: