Ask from binding if GC is disabled (#1075)

MMTk currently supports disabling GC (a.k.a. allowing the heap to grow
above its limit) via two api functions:
[`disable_collection`](https://github.com/mmtk/mmtk-core/blob/ef2bd6d043d8675badaa415db89be7b52439725f/src/memory_manager.rs#L539)
and
[`enable_collection`](https://github.com/mmtk/mmtk-core/blob/ef2bd6d043d8675badaa415db89be7b52439725f/src/memory_manager.rs#L519).

This PR replaces this strategy since currently, MMTk relies on the
binding ensuring [thread
safety](https://github.com/mmtk/mmtk-core/blob/ef2bd6d043d8675badaa415db89be7b52439725f/src/memory_manager.rs#L515)
when calling those functions. In this refactoring, MMTk asks if GC is
disabled in `VM::VMCollection::is_collection_disabled()` function, and
expects any thread safety logic to be implemented in the VM itself, and
not necessarily depend on MMTk.

---------

Co-authored-by: Yi Lin <qinsoon@gmail.com>
Co-authored-by: Kunshan Wang <wks1986@gmail.com>

benches/alloc.rs

@@ -2,12 +2,24 @@ use criterion::Criterion;
 
 use mmtk::memory_manager;
 use mmtk::util::test_util::fixtures::*;
+use mmtk::util::test_util::mock_method::*;
+use mmtk::util::test_util::mock_vm::{write_mockvm, MockVM};
 use mmtk::AllocationSemantics;
 
 pub fn bench(c: &mut Criterion) {
-    // Disable GC so we won't trigger GC
+    // Setting a larger heap, although the GC should be disabled in the MockVM
     let mut fixture = MutatorFixture::create_with_heapsize(1 << 30);
-    memory_manager::disable_collection(fixture.mmtk());
+    {
+        write_mockvm(|mock| {
+            *mock = {
+                MockVM {
+                    is_collection_enabled: MockMethod::new_fixed(Box::new(|_| false)),
+                    ..MockVM::default()
+                }
+            }
+        });
+    }
+
     c.bench_function("alloc", |b| {
         b.iter(|| {
             let _addr =

docs/header/mmtk.h

@@ -34,12 +34,6 @@ extern void mmtk_flush_mutator(MMTk_Mutator mutator);
 // Initialize MMTk scheduler and GC workers
 extern void mmtk_initialize_collection(void* tls);
 
-// Allow MMTk to perform a GC when the heap is full
-extern void mmtk_enable_collection();
-
-// Disallow MMTk to perform a GC when the heap is full
-extern void mmtk_disable_collection();
-
 // Allocate memory for an object
 extern void* mmtk_alloc(MMTk_Mutator mutator,
                         size_t size,

src/global_state.rs

@@ -13,9 +13,6 @@ use std::sync::Mutex;
 pub struct GlobalState {
     /// Whether MMTk is now ready for collection. This is set to true when initialize_collection() is called.
     pub(crate) initialized: AtomicBool,
-    /// Should we trigger a GC when the heap is full? It seems this should always be true. However, we allow
-    /// bindings to temporarily disable GC, at which point, we do not trigger GC even if the heap is full.
-    pub(crate) trigger_gc_when_heap_is_full: AtomicBool,
     /// The current GC status.
     pub(crate) gc_status: Mutex<GcStatus>,
     /// Is the current GC an emergency collection? Emergency means we may run out of memory soon, and we should
@@ -54,11 +51,6 @@ impl GlobalState {
         self.initialized.load(Ordering::SeqCst)
     }
 
-    /// Should MMTK trigger GC when heap is full? If GC is disabled, we wont trigger GC even if the heap is full.
-    pub fn should_trigger_gc_when_heap_is_full(&self) -> bool {
-        self.trigger_gc_when_heap_is_full.load(Ordering::SeqCst)
-    }
-
     /// Set the collection kind for the current GC. This is called before
     /// scheduling collection to determin what kind of collection it will be.
     pub fn set_collection_kind(
@@ -202,7 +194,6 @@ impl Default for GlobalState {
     fn default() -> Self {
         Self {
             initialized: AtomicBool::new(false),
-            trigger_gc_when_heap_is_full: AtomicBool::new(true),
             gc_status: Mutex::new(GcStatus::NotInGC),
             stacks_prepared: AtomicBool::new(false),
             emergency_collection: AtomicBool::new(false),

src/memory_manager.rs

@@ -35,9 +35,8 @@ use std::sync::atomic::Ordering;
 /// 2. Set command line options for MMTKBuilder by [`crate::memory_manager::process`] or [`crate::memory_manager::process_bulk`].
 /// 3. Initialize MMTk by calling this function, `mmtk_init()`, and pass the builder earlier. This call will return an MMTK instance.
 ///    Usually a binding store the MMTK instance statically as a singleton. We plan to allow multiple instances, but this is not yet fully
-///    supported. Currently we assume a binding will only need one MMTk instance.
-/// 4. Enable garbage collection in MMTk by [`crate::memory_manager::enable_collection`]. A binding should only call this once its
-///    thread system is ready. MMTk will not trigger garbage collection before this call.
+///    supported. Currently we assume a binding will only need one MMTk instance. Note that GC is enabled by default and the binding should
+///    implement `VMCollection::is_collection_enabled()` if it requires that the GC should be disabled at a particular time.
 ///
 /// Note that this method will attempt to initialize a logger. If the VM would like to use its own logger, it should initialize the logger before calling this method.
 /// Note that, to allow MMTk to do GC properly, `initialize_collection()` needs to be called after this call when
@@ -454,7 +453,7 @@ pub fn gc_poll<VM: VMBinding>(mmtk: &MMTK<VM>, tls: VMMutatorThread) {
         "gc_poll() can only be called by a mutator thread."
     );
 
-    if mmtk.state.should_trigger_gc_when_heap_is_full() && mmtk.gc_trigger.poll(false, None) {
+    if VM::VMCollection::is_collection_enabled() && mmtk.gc_trigger.poll(false, None) {
         debug!("Collection required");
         assert!(mmtk.state.is_initialized(), "GC is not allowed here: collection is not initialized (did you call initialize_collection()?).");
         VM::VMCollection::block_for_gc(tls);
@@ -510,42 +509,6 @@ pub fn initialize_collection<VM: VMBinding>(mmtk: &'static MMTK<VM>, tls: VMThre
     probe!(mmtk, collection_initialized);
 }
 
-/// Allow MMTk to trigger garbage collection when heap is full. This should only be used in pair with disable_collection().
-/// See the comments on disable_collection(). If disable_collection() is not used, there is no need to call this function at all.
-/// Note this call is not thread safe, only one VM thread should call this.
-///
-/// Arguments:
-/// * `mmtk`: A reference to an MMTk instance.
-pub fn enable_collection<VM: VMBinding>(mmtk: &'static MMTK<VM>) {
-    debug_assert!(
-        !mmtk.state.should_trigger_gc_when_heap_is_full(),
-        "enable_collection() is called when GC is already enabled."
-    );
-    mmtk.state
-        .trigger_gc_when_heap_is_full
-        .store(true, Ordering::SeqCst);
-}
-
-/// Disallow MMTk to trigger garbage collection. When collection is disabled, you can still allocate through MMTk. But MMTk will
-/// not trigger a GC even if the heap is full. In such a case, the allocation will exceed the MMTk's heap size (the soft heap limit).
-/// However, there is no guarantee that the physical allocation will succeed, and if it succeeds, there is no guarantee that further allocation
-/// will keep succeeding. So if a VM disables collection, it needs to allocate with careful consideration to make sure that the physical memory
-/// allows the amount of allocation. We highly recommend not using this method. However, we support this to accomodate some VMs that require this
-/// behavior. This call does not disable explicit GCs (through handle_user_collection_request()).
-/// Note this call is not thread safe, only one VM thread should call this.
-///
-/// Arguments:
-/// * `mmtk`: A reference to an MMTk instance.
-pub fn disable_collection<VM: VMBinding>(mmtk: &'static MMTK<VM>) {
-    debug_assert!(
-        mmtk.state.should_trigger_gc_when_heap_is_full(),
-        "disable_collection() is called when GC is not enabled."
-    );
-    mmtk.state
-        .trigger_gc_when_heap_is_full
-        .store(false, Ordering::SeqCst);
-}
-
 /// Process MMTk run-time options. Returns true if the option is processed successfully.
 ///
 /// Arguments:

src/mmtk.rs

@@ -318,7 +318,7 @@ impl<VM: VMBinding> MMTK<VM> {
             return;
         }
 
-        if force || !*self.options.ignore_system_gc {
+        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() {

src/policy/space.rs

@@ -86,11 +86,8 @@ pub trait Space<VM: VMBinding>: 'static + SFT + Sync + Downcast {
         // Should we poll to attempt to 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)
-            && self
-                .common()
-                .global_state
-                .should_trigger_gc_when_heap_is_full();
+        let should_poll =
+            VM::VMActivePlan::is_mutator(tls) && VM::VMCollection::is_collection_enabled();
         // Is a GC allowed here? If we should poll but are not allowed to poll, we will panic.
         // initialize_collection() has to be called so we know GC is initialized.
         let allow_gc = should_poll && self.common().global_state.is_initialized();

src/util/test_util/mock_vm.rs

@@ -204,6 +204,7 @@ pub struct MockVM {
     pub schedule_finalization: MockMethod<VMWorkerThread, ()>,
     pub post_forwarding: MockMethod<VMWorkerThread, ()>,
     pub vm_live_bytes: MockMethod<(), usize>,
+    pub is_collection_enabled: MockMethod<(), bool>,
     // object model
     pub copy_object: MockMethod<
         (
@@ -280,6 +281,7 @@ impl Default for MockVM {
             schedule_finalization: MockMethod::new_default(),
             post_forwarding: MockMethod::new_default(),
             vm_live_bytes: MockMethod::new_default(),
+            is_collection_enabled: MockMethod::new_fixed(Box::new(|_| true)),
 
             copy_object: MockMethod::new_unimplemented(),
             copy_object_to: MockMethod::new_unimplemented(),
@@ -450,6 +452,10 @@ impl crate::vm::Collection<MockVM> for MockVM {
         mock!(post_forwarding(tls))
     }
 
+    fn is_collection_enabled() -> bool {
+        mock!(is_collection_enabled())
+    }
+
     fn vm_live_bytes() -> usize {
         mock!(vm_live_bytes())
     }

src/vm/collection.rs

@@ -139,4 +139,23 @@ pub trait Collection<VM: VMBinding> {
         // By default, MMTk assumes the amount of memory the VM allocates off-heap is negligible.
         0
     }
+
+    /// Callback function to ask the VM whether GC is enabled or disabled, allowing or disallowing MMTk
+    /// to trigger garbage collection. When collection is disabled, you can still allocate through MMTk,
+    /// but MMTk will not trigger a GC even if the heap is full. In such a case, the allocation will
+    /// exceed MMTk's heap size (the soft heap limit). However, there is no guarantee that the physical
+    /// allocation will succeed, and if it succeeds, there is no guarantee that further allocation will
+    /// keep succeeding. So if a VM disables collection, it needs to allocate with careful consideration
+    /// to make sure that the physical memory allows the amount of allocation. We highly recommend
+    /// to have GC always enabled (i.e. that this method always returns true). However, we support
+    /// this to accomodate some VMs that require this behavior. Note that
+    /// `handle_user_collection_request()` calls this function, too.  If this function returns
+    /// false, `handle_user_collection_request()` will not trigger GC, either. Note also that any synchronization
+    /// involving enabling and disabling collections by mutator threads should be implemented by the VM.
+    fn is_collection_enabled() -> bool {
+        // By default, MMTk assumes that collections are always enabled, and the binding should define
+        // this method if the VM supports disabling GC, or if the VM cannot safely trigger GC until some
+        // initialization is done, such as initializing class metadata for scanning objects.
+        true
+    }
 }

src/vm/tests/mock_tests/mock_test_allocate_with_disable_collection.rs

@@ -1,14 +1,20 @@
 use crate::memory_manager;
 use crate::util::test_util::fixtures::*;
 use crate::util::test_util::mock_vm::*;
+use crate::vm::tests::mock_tests::mock_test_prelude::MockMethod;
 use crate::AllocationSemantics;
 
 /// This test allocates after calling disable_collection(). When we exceed the heap limit, MMTk will NOT trigger a GC.
 /// And the allocation will succeed.
 #[test]
 pub fn allocate_with_disable_collection() {
     with_mockvm(
-        default_setup,
+        || -> MockVM {
+            MockVM {
+                is_collection_enabled: MockMethod::new_fixed(Box::new(|_| false)),
+                ..MockVM::default()
+            }
+        },
         || {
             // 1MB heap
             const MB: usize = 1024 * 1024;
@@ -24,8 +30,6 @@ pub fn allocate_with_disable_collection() {
             );
             assert!(!addr.is_zero());
 
-            // Disable GC
-            memory_manager::disable_collection(fixture.mmtk());
             // Allocate another MB. This exceeds the heap size. But as we have disabled GC, MMTk will not trigger a GC, and allow this allocation.
             let addr =
                 memory_manager::alloc(&mut fixture.mutator, MB, 8, 0, AllocationSemantics::Default);

src/vm/tests/mock_tests/mock_test_allocate_with_re_enable_collection.rs

@@ -4,8 +4,9 @@ use crate::util::test_util::mock_method::*;
 use crate::util::test_util::mock_vm::*;
 use crate::AllocationSemantics;
 
-// This test allocates after calling initialize_collection(). When we exceed the heap limit, MMTk will trigger a GC. And block_for_gc will be called.
-// We havent implemented block_for_gc so it will panic. This test is similar to allocate_with_initialize_collection, except that we once disabled GC in the test.
+/// This test allocates after calling `initialize_collection()`. When we exceed the heap limit for the first time, MMTk will not trigger GC since GC has been disabled
+/// However, the second 1MB allocation will trigger a GC since GC is enabled again. And `block_for_gc` will be called.
+/// We haven't implemented `block_for_gc` so it will panic. This test is similar to `allocate_with_initialize_collection`, except that GC is disabled once in the test.
 #[test]
 #[should_panic(expected = "block_for_gc is called")]
 pub fn allocate_with_re_enable_collection() {
@@ -14,6 +15,11 @@ pub fn allocate_with_re_enable_collection() {
         || -> MockVM {
             MockVM {
                 block_for_gc: MockMethod::new_fixed(Box::new(|_| panic!("block_for_gc is called"))),
+                is_collection_enabled: MockMethod::new_sequence(vec![
+                    Box::new(|()| -> bool { true }), // gc is enabled but it shouldn't matter here
+                    Box::new(|()| -> bool { false }), // gc is disabled
+                    Box::new(|()| -> bool { true }), // gc is enabled again
+                ]),
                 ..MockVM::default()
             }
         },
@@ -31,29 +37,22 @@ pub fn allocate_with_re_enable_collection() {
             );
             assert!(!addr.is_zero());
 
-            // Disable GC. So we can keep allocate without triggering a GC.
-            memory_manager::disable_collection(fixture.mmtk());
+            // In the next allocation GC is disabled. So we can keep allocate without triggering a GC.
             // Fill up the heap
-            let _ = memory_manager::alloc(
-                &mut fixture.mutator,
-                MB >> 1,
-                8,
-                0,
-                AllocationSemantics::Default,
-            );
+            let _ =
+                memory_manager::alloc(&mut fixture.mutator, MB, 8, 0, AllocationSemantics::Default);
 
-            // Enable GC again.
-            memory_manager::enable_collection(fixture.mmtk());
-            // Attempt another allocation. This will trigger GC.
+            // Attempt another allocation. This will trigger GC since GC is enabled again.
             let addr =
                 memory_manager::alloc(&mut fixture.mutator, MB, 8, 0, AllocationSemantics::Default);
             assert!(!addr.is_zero());
         },
         || {
-            // This is actually redundant, as we defined block_for_gc for this test.
-            // This just demostrates that we can check if the method is called.
+            // This ensures that block_for_gc is called for this test, and that the second allocation
+            // does not trigger GC since we expect is_collection_enabled to be called three times.
             read_mockvm(|mock| {
                 assert!(mock.block_for_gc.is_called());
+                assert!(mock.is_collection_enabled.call_count() == 3);
             });
         },
     )