Set SFT for MallocSpace (#476)

Set SFT entries for MallocSpace (closes #473), and use the newly added `is_mmtk_object` SFT method to correctly check whether an object in a MallocSpace chunk is allocated by `malloc`.

Fix an issue that if the address is chunk aligned, the SFT entries may not be set properly (closes #374). The `update` method of SFT accepts the number of bytes as the size argument, rather than the number of chunks.

Author: qinsoon

src/plan/marksweep/global.rs

@@ -61,6 +61,8 @@ impl<VM: VMBinding> Plan for MarkSweep<VM> {
     fn schedule_collection(&'static self, scheduler: &GCWorkScheduler<VM>) {
         self.base().set_collection_kind::<Self>(self);
         self.base().set_gc_status(GcStatus::GcPrepare);
+        self.common()
+            .schedule_common::<MSProcessEdges<VM>>(&MS_CONSTRAINTS, scheduler);
         // Stop & scan mutators (mutator scanning can happen before STW)
         scheduler.work_buckets[WorkBucketStage::Unconstrained]
             .add(StopMutators::<MSProcessEdges<VM>>::new());

src/policy/lockfreeimmortalspace.rs

@@ -1,7 +1,6 @@
 use crate::mmtk::SFT_MAP;
 use crate::policy::space::{CommonSpace, Space, SFT};
 use crate::util::address::Address;
-use crate::util::conversions::bytes_to_chunks_up;
 use crate::util::heap::PageResource;
 
 use crate::util::ObjectReference;
@@ -104,11 +103,7 @@ impl<VM: VMBinding> Space<VM> for LockFreeImmortalSpace<VM> {
             // TODO(Javad): handle meta space allocation failure
             panic!("failed to mmap meta memory");
         }
-        SFT_MAP.update(
-            self.as_sft(),
-            AVAILABLE_START,
-            bytes_to_chunks_up(total_bytes),
-        );
+        SFT_MAP.update(self.as_sft(), AVAILABLE_START, total_bytes);
     }
 
     fn reserved_pages(&self) -> usize {

src/policy/mallocspace/global.rs

@@ -71,6 +71,11 @@ impl<VM: VMBinding> SFT for MallocSpace<VM> {
         true
     }
 
+    // For malloc space, we need to further check the alloc bit.
+    fn is_mmtk_object(&self, object: ObjectReference) -> bool {
+        is_alloced_by_malloc(object)
+    }
+
     fn initialize_object_metadata(&self, object: ObjectReference, _alloc: bool) {
         trace!("initialize_object_metadata for object {}", object);
         let page_addr = conversions::page_align_down(object.to_address());
@@ -204,6 +209,8 @@ impl<VM: VMBinding> MallocSpace<VM> {
             if !is_meta_space_mapped(address, actual_size) {
                 // Map the metadata space for the associated chunk
                 self.map_metadata_and_update_bound(address, actual_size);
+                // Update SFT
+                crate::mmtk::SFT_MAP.update(self, address, actual_size);
             }
             self.active_bytes.fetch_add(actual_size, Ordering::SeqCst);
 
@@ -327,6 +334,14 @@ impl<VM: VMBinding> MallocSpace<VM> {
         }
     }
 
+    /// Clean up for an empty chunk
+    fn clean_up_empty_chunk(&self, chunk_start: Address) {
+        // Since the chunk mark metadata is a byte, we don't need synchronization
+        unsafe { unset_chunk_mark_unsafe(chunk_start) };
+        // Clear the SFT entry
+        crate::mmtk::SFT_MAP.clear(chunk_start);
+    }
+
     /// This function is called when the mark bits sit on the side metadata.
     /// This has been optimized with the use of bulk loading and bulk zeroing of
     /// metadata.
@@ -495,8 +510,7 @@ impl<VM: VMBinding> MallocSpace<VM> {
         bzero_metadata(&mark_bit_spec, chunk_start, BYTES_IN_CHUNK);
 
         if chunk_is_empty {
-            // Since the chunk mark metadata is a byte, we don't need synchronization
-            unsafe { unset_chunk_mark_unsafe(chunk_start) };
+            self.clean_up_empty_chunk(chunk_start);
         }
 
         debug!(
@@ -614,8 +628,7 @@ impl<VM: VMBinding> MallocSpace<VM> {
         }
 
         if chunk_is_empty {
-            // Since the chunk mark metadata is a byte, we don't need synchronization
-            unsafe { unset_chunk_mark_unsafe(chunk_start) };
+            self.clean_up_empty_chunk(chunk_start);
         }
 
         debug!(

src/policy/mallocspace/mod.rs

@@ -3,4 +3,3 @@ mod global;
 pub mod metadata;
 
 pub use global::*;
-pub(crate) use metadata::is_alloced_by_malloc;

src/policy/space.rs

@@ -69,6 +69,14 @@ pub trait SFT {
     /// object - the sanity checker will fail if an object is not sane.
     #[cfg(feature = "sanity")]
     fn is_sane(&self) -> bool;
+    /// Is the object managed by MMTk? For most cases, if we find the sft for an object, that means
+    /// the object is in the space and managed by MMTk. However, for some spaces, like MallocSpace,
+    /// we mark the entire chunk in the SFT table as a malloc space, but only some of the addresses
+    /// in the space contain actual MMTk objects. So they need a further check.
+    #[inline(always)]
+    fn is_mmtk_object(&self, _object: ObjectReference) -> bool {
+        true
+    }
     /// Initialize object metadata (in the header, or in the side metadata).
     fn initialize_object_metadata(&self, object: ObjectReference, alloc: bool);
 }
@@ -106,6 +114,10 @@ impl SFT for EmptySpaceSFT {
          */
         false
     }
+    #[inline(always)]
+    fn is_mmtk_object(&self, _object: ObjectReference) -> bool {
+        false
+    }
 
     fn initialize_object_metadata(&self, object: ObjectReference, _alloc: bool) {
         panic!(
@@ -153,19 +165,20 @@ impl<'a> SFTMap<'a> {
         res
     }
 
-    fn log_update(&self, space: &(dyn SFT + Sync + 'static), start: Address, chunks: usize) {
+    fn log_update(&self, space: &(dyn SFT + Sync + 'static), start: Address, bytes: usize) {
+        debug!(
+            "Update SFT for [{}, {}) as {}",
+            start,
+            start + bytes,
+            space.name()
+        );
         let first = start.chunk_index();
-        let end = start + (chunks << LOG_BYTES_IN_CHUNK);
-        debug!("Update SFT for [{}, {}) as {}", start, end, space.name());
+        let last = conversions::chunk_align_up(start + bytes).chunk_index();
         let start_chunk = chunk_index_to_address(first);
-        let end_chunk = chunk_index_to_address(first + chunks);
+        let end_chunk = chunk_index_to_address(last);
         debug!(
-            "Update SFT for {} chunks of [{} #{}, {} #{})",
-            chunks,
-            start_chunk,
-            first,
-            end_chunk,
-            first + chunks
+            "Update SFT for {} bytes of [{} #{}, {} #{})",
+            bytes, start_chunk, first, end_chunk, last
         );
     }
 
@@ -188,13 +201,15 @@ impl<'a> SFTMap<'a> {
     }
 
     /// Update SFT map for the given address range.
-    /// It should be used in these cases: 1. when a space grows, 2. when initializing a contiguous space, 3. when ensure_mapped() is called on a space.
-    pub fn update(&self, space: &(dyn SFT + Sync + 'static), start: Address, chunks: usize) {
+    /// It should be used when we acquire new memory and use it as part of a space. For example, the cases include:
+    /// 1. when a space grows, 2. when initializing a contiguous space, 3. when ensure_mapped() is called on a space.
+    pub fn update(&self, space: &(dyn SFT + Sync + 'static), start: Address, bytes: usize) {
         if DEBUG_SFT {
-            self.log_update(space, start, chunks);
+            self.log_update(space, start, bytes);
         }
         let first = start.chunk_index();
-        for chunk in first..(first + chunks) {
+        let last = conversions::chunk_align_up(start + bytes).chunk_index();
+        for chunk in first..last {
             self.set(chunk, space);
         }
         if DEBUG_SFT {
@@ -204,10 +219,18 @@ impl<'a> SFTMap<'a> {
 
     // TODO: We should clear a SFT entry when a space releases a chunk.
     #[allow(dead_code)]
-    pub fn clear(&self, chunk_idx: usize) {
+    pub fn clear(&self, chunk_start: Address) {
+        assert!(chunk_start.is_aligned_to(BYTES_IN_CHUNK));
+        let chunk_idx = chunk_start.chunk_index();
         self.set(chunk_idx, &EMPTY_SPACE_SFT);
     }
 
+    // Currently only used by 32 bits vm map
+    #[allow(dead_code)]
+    pub fn clear_by_index(&self, chunk_idx: usize) {
+        self.set(chunk_idx, &EMPTY_SPACE_SFT)
+    }
+
     fn set(&self, chunk: usize, sft: &(dyn SFT + Sync + 'static)) {
         /*
          * This is safe (only) because a) this is only called during the
@@ -227,7 +250,8 @@ impl<'a> SFTMap<'a> {
             // in which case, we still set SFT map again.
             debug_assert!(
                 old == EMPTY_SFT_NAME || new == EMPTY_SFT_NAME || old == new,
-                "attempt to overwrite a non-empty chunk in SFT map (from {} to {})",
+                "attempt to overwrite a non-empty chunk {} in SFT map (from {} to {})",
+                chunk,
                 old,
                 new
             );
@@ -236,16 +260,10 @@ impl<'a> SFTMap<'a> {
     }
 
     pub fn is_in_space(&self, object: ObjectReference) -> bool {
-        let not_in_space = object.to_address().chunk_index() >= self.sft.len()
-            || self.get(object.to_address()).name() == EMPTY_SPACE_SFT.name();
-
-        if not_in_space {
-            // special case - we do not yet have SFT entries for malloc space
-            use crate::policy::mallocspace::is_alloced_by_malloc;
-            is_alloced_by_malloc(object)
-        } else {
-            true
+        if object.to_address().chunk_index() >= self.sft.len() {
+            return false;
         }
+        self.get(object.to_address()).is_mmtk_object(object)
     }
 }
 
@@ -361,8 +379,7 @@ pub trait Space<VM: VMBinding>: 'static + SFT + Sync + Downcast {
         //     "should only grow space for new chunks at chunk-aligned start address"
         // );
         if new_chunk {
-            let chunks = conversions::bytes_to_chunks_up(bytes);
-            SFT_MAP.update(self.as_sft(), start, chunks);
+            SFT_MAP.update(self.as_sft(), start, bytes);
         }
     }
 
@@ -371,7 +388,6 @@ pub trait Space<VM: VMBinding>: 'static + SFT + Sync + Downcast {
      *  mapped (e.g. for a vm image which is externally mmapped.)
      */
     fn ensure_mapped(&self) {
-        let chunks = conversions::bytes_to_chunks_up(self.common().extent);
         if self
             .common()
             .metadata
@@ -381,7 +397,7 @@ pub trait Space<VM: VMBinding>: 'static + SFT + Sync + Downcast {
             // TODO(Javad): handle meta space allocation failure
             panic!("failed to mmap meta memory");
         }
-        SFT_MAP.update(self.as_sft(), self.common().start, chunks);
+        SFT_MAP.update(self.as_sft(), self.common().start, self.common().extent);
         use crate::util::heap::layout::mmapper::Mmapper;
         self.common()
             .mmapper
@@ -602,7 +618,7 @@ impl<VM: VMBinding> CommonSpace<VM> {
                 // TODO(Javad): handle meta space allocation failure
                 panic!("failed to mmap meta memory");
             }
-            SFT_MAP.update(space.as_sft(), self.start, bytes_to_chunks_up(self.extent));
+            SFT_MAP.update(space.as_sft(), self.start, self.extent);
         }
     }
 

src/util/heap/layout/map32.rs

@@ -269,7 +269,7 @@ impl Map32 {
         self.next_link[chunk as usize] = 0;
         for offset in 0..chunks {
             self.descriptor_map[(chunk + offset) as usize] = SpaceDescriptor::UNINITIALIZED;
-            SFT_MAP.clear((chunk + offset) as usize);
+            SFT_MAP.clear_by_index((chunk + offset) as usize);
             // VM.barriers.objectArrayStoreNoGCBarrier(spaceMap, chunk + offset, null);
         }
         chunks as _