alloc_no_gc will not call out_of_memory when allocating an erroneously

large object. Minor fixes for comments

Author: qinsoon

src/memory_manager.rs

@@ -160,12 +160,6 @@ pub fn flush_mutator<VM: VMBinding>(mutator: &mut Mutator<VM>) {
 /// not limited to throwing exceptions or errors. If [`crate::vm::Collection::out_of_memory`] returns
 /// normally without panicking or throwing exceptions, this function will return zero.
 ///
-/// This function in most cases returns a valid memory address.
-/// This function may return a zero address iif 1. MMTk attempts at least one GC,
-/// 2. the GC does not free up enough memory, 3. MMTk calls [`crate::vm::Collection::out_of_memory`]
-/// to the binding, and 4. the binding returns from [`crate::vm::Collection::out_of_memory`]
-/// instead of throwing an exception/error.
-///
 /// For performance reasons, a VM should implement the allocation fast-path on their side rather
 /// than just calling this function.
 ///
@@ -183,7 +177,7 @@ pub fn alloc<VM: VMBinding>(
     semantics: AllocationSemantics,
 ) -> Address {
     #[cfg(debug_assertions)]
-    crate::util::alloc::allocator::asset_allocation_args::<VM>(size, align, offset);
+    crate::util::alloc::allocator::assert_allocation_args::<VM>(size, align, offset);
 
     mutator.alloc(size, align, offset, semantics)
 }
@@ -212,7 +206,7 @@ pub fn alloc_no_gc<VM: VMBinding>(
     semantics: AllocationSemantics,
 ) -> Address {
     #[cfg(debug_assertions)]
-    crate::util::alloc::allocator::asset_allocation_args::<VM>(size, align, offset);
+    crate::util::alloc::allocator::assert_allocation_args::<VM>(size, align, offset);
 
     mutator.alloc_no_gc(size, align, offset, semantics)
 }

src/plan/mutator_context.rs

@@ -300,7 +300,7 @@ pub trait MutatorContext<VM: VMBinding>: Send + 'static {
     fn prepare(&mut self, tls: VMWorkerThread);
     /// Do the release work for this mutator.
     fn release(&mut self, tls: VMWorkerThread);
-    /// Allocate memory for an object. This is a GC safepoint. GC will be triggered if the allocation fails.
+    /// Allocate memory for an object. This function will trigger a GC on failed allocation.
     ///
     /// Arguments:
     /// * `size`: the number of bytes required for the object.
@@ -314,8 +314,7 @@ pub trait MutatorContext<VM: VMBinding>: Send + 'static {
         offset: usize,
         allocator: AllocationSemantics,
     ) -> Address;
-    /// Allocate memory for an object. This is NOT a GC safepoint. If the allocation fails,
-    /// the function returns a zero value without triggering a GC.
+    /// Allocate memory for an object. This function will not trigger a GC on failed allocation.
     ///
     /// Arguments:
     /// * `size`: the number of bytes required for the object.
@@ -329,7 +328,7 @@ pub trait MutatorContext<VM: VMBinding>: Send + 'static {
         offset: usize,
         allocator: AllocationSemantics,
     ) -> Address;
-    /// The slow path allocation. This is a GC safepoint. GC will be triggered if the allocation fails.
+    /// The slow path allocation for [`MutatorContext::alloc`]. This function will trigger a GC on failed allocation.
     ///
     ///  This is only useful when the binding
     /// implements the fast path allocation, and would like to explicitly
@@ -341,8 +340,7 @@ pub trait MutatorContext<VM: VMBinding>: Send + 'static {
         offset: usize,
         allocator: AllocationSemantics,
     ) -> Address;
-    /// The slow path allocation. This is NOT a GC safepoint. If the allocation fails,
-    /// the function returns a zero value without triggering a GC.
+    /// The slow path allocation for [`MutatorContext::alloc_no_gc`]. This function will not trigger a GC on failed allocation.
     ///
     /// This is only useful when the binding
     /// implements the fast path allocation, and would like to explicitly

src/policy/space.rs

@@ -67,14 +67,16 @@ pub trait Space<VM: VMBinding>: 'static + SFT + Sync + Downcast {
     /// avoid arithmatic overflow. If we have to do computation in the allocation fastpath and
     /// overflow happens there, there is nothing we can do about it.
     /// Return a boolean to indicate if we will be out of memory, determined by the check.
-    fn will_oom_on_acquire(&self, tls: VMThread, size: usize) -> bool {
+    fn will_oom_on_acquire(&self, tls: VMThread, size: usize, allow_oom_call: bool) -> bool {
         let max_pages = self.get_gc_trigger().policy.get_max_heap_size_in_pages();
         let requested_pages = size >> LOG_BYTES_IN_PAGE;
         if requested_pages > max_pages {
-            VM::VMCollection::out_of_memory(
-                tls,
-                crate::util::alloc::AllocationError::HeapOutOfMemory,
-            );
+            if allow_oom_call {
+                VM::VMCollection::out_of_memory(
+                    tls,
+                    crate::util::alloc::AllocationError::HeapOutOfMemory,
+                );
+            }
             return true;
         }
         false
@@ -88,7 +90,7 @@ pub trait Space<VM: VMBinding>: 'static + SFT + Sync + Downcast {
         );
 
         debug_assert!(
-            !self.will_oom_on_acquire(tls, pages << LOG_BYTES_IN_PAGE),
+            !self.will_oom_on_acquire(tls, pages << LOG_BYTES_IN_PAGE, !no_gc_on_fail),
             "The requested pages is larger than the max heap size. Is will_go_oom_on_acquire used before acquring memory?"
         );
 

src/util/alloc/allocator.rs

@@ -132,7 +132,7 @@ pub fn get_maximum_aligned_size_inner<VM: VMBinding>(
 }
 
 #[cfg(debug_assertions)]
-pub(crate) fn asset_allocation_args<VM: VMBinding>(size: usize, align: usize, offset: usize) {
+pub(crate) fn assert_allocation_args<VM: VMBinding>(size: usize, align: usize, offset: usize) {
     // MMTk has assumptions about minimal object size.
     // We need to make sure that all allocations comply with the min object size.
     // Ideally, we check the allocation size, and if it is smaller, we transparently allocate the min
@@ -150,7 +150,7 @@ pub(crate) fn asset_allocation_args<VM: VMBinding>(size: usize, align: usize, of
 
 /// The context an allocator needs to access in order to perform allocation.
 pub struct AllocatorContext<VM: VMBinding> {
-    /// Whether we should trigger a GC when the current alloccation fails.
+    /// Whether we should avoid trigger a GC when the current allocation fails.
     /// The default value is `false`. This value is only set to `true` when
     /// the binding requests a 'no-gc' alloc, and will be set back to `false`
     /// when the allocation is done (successfully or not).

src/util/alloc/bumpallocator.rs

@@ -194,7 +194,10 @@ impl<VM: VMBinding> BumpAllocator<VM> {
         offset: usize,
         stress_test: bool,
     ) -> Address {
-        if self.space.will_oom_on_acquire(self.tls, size) {
+        if self
+            .space
+            .will_oom_on_acquire(self.tls, size, !self.get_context().is_no_gc_on_fail())
+        {
             return Address::ZERO;
         }
 

src/util/alloc/large_object_allocator.rs

@@ -49,7 +49,10 @@ impl<VM: VMBinding> Allocator<VM> for LargeObjectAllocator<VM> {
     }
 
     fn alloc_slow_once(&mut self, size: usize, align: usize, _offset: usize) -> Address {
-        if self.space.will_oom_on_acquire(self.tls, size) {
+        if self
+            .space
+            .will_oom_on_acquire(self.tls, size, !self.get_context().is_no_gc_on_fail())
+        {
             return Address::ZERO;
         }
 

src/vm/tests/mock_tests/mock_test_allocate_no_gc_oom_on_acquire.rs

@@ -0,0 +1,35 @@
+use super::mock_test_prelude::*;
+
+use crate::AllocationSemantics;
+
+/// This test will allocate an object that is larger than the heap size. The call will fail.
+#[test]
+pub fn allocate_no_gc_oom_on_acquire() {
+    // 1MB heap
+    with_mockvm(
+        default_setup,
+        || {
+            const KB: usize = 1024;
+            let mut fixture = MutatorFixture::create_with_heapsize(KB);
+
+            // Attempt to allocate an object that is larger than the heap size.
+            let addr = memory_manager::alloc_no_gc(
+                &mut fixture.mutator,
+                1024 * 10,
+                8,
+                0,
+                AllocationSemantics::Default,
+            );
+            // We should get zero.
+            assert!(addr.is_zero());
+            // block_for_gc and out_of_memory won't be called.
+            read_mockvm(|mock| {
+                assert!(!mock.block_for_gc.is_called());
+            });
+            read_mockvm(|mock| {
+                assert!(!mock.out_of_memory.is_called());
+            });
+        },
+        no_cleanup,
+    )
+}

src/vm/tests/mock_tests/mock_test_allocate_no_gc.rs src/vm/tests/mock_tests/mock_test_allocate_no_gc_simple.rs

@@ -5,7 +5,7 @@ use crate::AllocationSemantics;
 /// This test will do alloc_no_gc in a loop, and evetually fill up the heap.
 /// As alloc_no_gc will not trigger a GC, we expect to see a return value of zero, and no GC is triggered.
 #[test]
-pub fn allocate_no_gc() {
+pub fn allocate_no_gc_simple() {
     // 1MB heap
     with_mockvm(
         default_setup,

src/vm/tests/mock_tests/mod.rs

@@ -24,7 +24,8 @@ pub(crate) mod mock_test_prelude {
 }
 
 mod mock_test_allocate_align_offset;
-mod mock_test_allocate_no_gc;
+mod mock_test_allocate_no_gc_oom_on_acquire;
+mod mock_test_allocate_no_gc_simple;
 mod mock_test_allocate_with_disable_collection;
 mod mock_test_allocate_with_initialize_collection;
 mod mock_test_allocate_with_re_enable_collection;