Clear side forwarding bits properly (#1138)

This PR fixes bugs related to the clearing of side forwarding bits.

The side forwarding bits of ImmixSpace are now cleared after moving GCs
instead of before every major GC. This eliminates redundant clearing
operations during non-moving full-heap GCs (i.e. when not doing defrag),
and also ensures the subsequent nursery GCs do not see stale forwarding
bits when using StickyImmix in which case young objects are also
allocated in the ImmixSpace.

Also fixed the code for clearing side forwarding bits in discontiguous
CopySpace.

Author: wks

src/policy/copyspace.rs

@@ -168,32 +168,26 @@ impl<VM: VMBinding> CopySpace<VM> {
 
     pub fn prepare(&self, from_space: bool) {
         self.from_space.store(from_space, Ordering::SeqCst);
-        // Clear the metadata if we are using side forwarding status table. Otherwise
-        // objects may inherit forwarding status from the previous GC.
-        // TODO: Fix performance.
-        if let MetadataSpec::OnSide(side_forwarding_status_table) =
-            *<VM::VMObjectModel as ObjectModel<VM>>::LOCAL_FORWARDING_BITS_SPEC
-        {
-            side_forwarding_status_table
-                .bzero_metadata(self.common.start, self.pr.cursor() - self.common.start);
-        }
     }
 
     pub fn release(&self) {
-        unsafe {
+        for (start, size) in self.pr.iterate_allocated_regions() {
+            // Clear the forwarding bits if it is on the side.
+            if let MetadataSpec::OnSide(side_forwarding_status_table) =
+                *<VM::VMObjectModel as ObjectModel<VM>>::LOCAL_FORWARDING_BITS_SPEC
+            {
+                side_forwarding_status_table.bzero_metadata(start, size);
+            }
+
+            // Clear VO bits because all objects in the space are dead.
             #[cfg(feature = "vo_bit")]
-            self.reset_vo_bit();
-            self.pr.reset();
+            crate::util::metadata::vo_bit::bzero_vo_bit(start, size);
         }
-        self.common.metadata.reset();
-        self.from_space.store(false, Ordering::SeqCst);
-    }
 
-    #[cfg(feature = "vo_bit")]
-    unsafe fn reset_vo_bit(&self) {
-        for (start, size) in self.pr.iterate_allocated_regions() {
-            crate::util::metadata::vo_bit::bzero_vo_bit(start, size);
+        unsafe {
+            self.pr.reset();
         }
+        self.from_space.store(false, Ordering::SeqCst);
     }
 
     fn is_from_space(&self) -> bool {

src/policy/immix/defrag.rs

@@ -78,7 +78,7 @@ impl Defrag {
                 || !exhausted_reusable_space
                 || super::STRESS_DEFRAG
                 || (collect_whole_heap && user_triggered && full_heap_system_gc));
-        // println!("Defrag: {}", in_defrag);
+        info!("Defrag: {}", in_defrag);
         self.in_defrag_collection
             .store(in_defrag, Ordering::Release)
     }

src/policy/immix/immixspace.rs

@@ -841,10 +841,6 @@ impl<VM: VMBinding> PrepareBlockState<VM> {
                 unimplemented!("We cannot bulk zero unlogged bit.")
             }
         }
-        // If the forwarding bits are on the side, we need to clear them, too.
-        if let MetadataSpec::OnSide(side) = *VM::VMObjectModel::LOCAL_FORWARDING_BITS_SPEC {
-            side.bzero_metadata(self.chunk.start(), Chunk::BYTES);
-        }
     }
 }
 
@@ -891,14 +887,17 @@ struct SweepChunk<VM: VMBinding> {
 }
 
 impl<VM: VMBinding> GCWork<VM> for SweepChunk<VM> {
-    fn do_work(&mut self, _worker: &mut GCWorker<VM>, _mmtk: &'static MMTK<VM>) {
+    fn do_work(&mut self, _worker: &mut GCWorker<VM>, mmtk: &'static MMTK<VM>) {
         let mut histogram = self.space.defrag.new_histogram();
         if self.space.chunk_map.get(self.chunk) == ChunkState::Allocated {
             let line_mark_state = if super::BLOCK_ONLY {
                 None
             } else {
                 Some(self.space.line_mark_state.load(Ordering::Acquire))
             };
+            // Hints for clearing side forwarding bits.
+            let is_moving_gc = mmtk.get_plan().current_gc_may_move_object();
+            let is_defrag_gc = self.space.defrag.in_defrag();
             // number of allocated blocks.
             let mut allocated_blocks = 0;
             // Iterate over all allocated blocks in this chunk.
@@ -907,6 +906,30 @@ impl<VM: VMBinding> GCWork<VM> for SweepChunk<VM> {
                 .iter_region::<Block>()
                 .filter(|block| block.get_state() != BlockState::Unallocated)
             {
+                // Clear side forwarding bits.
+                // In the beginning of the next GC, no side forwarding bits shall be set.
+                // In this way, we can omit clearing forwarding bits when copying object.
+                // See `GCWorkerCopyContext::post_copy`.
+                // Note, `block.sweep()` overwrites `DEFRAG_STATE_TABLE` with the number of holes,
+                // but we need it to know if a block is a defrag source.
+                // We clear forwarding bits before `block.sweep()`.
+                if let MetadataSpec::OnSide(side) = *VM::VMObjectModel::LOCAL_FORWARDING_BITS_SPEC {
+                    if is_moving_gc {
+                        let objects_may_move = if is_defrag_gc {
+                            // If it is a defrag GC, we only clear forwarding bits for defrag sources.
+                            block.is_defrag_source()
+                        } else {
+                            // Otherwise, it must be a nursery GC of StickyImmix with copying nursery.
+                            // We don't have information about which block contains moved objects,
+                            // so we have to clear forwarding bits for all blocks.
+                            true
+                        };
+                        if objects_may_move {
+                            side.bzero_metadata(block.start(), Block::BYTES);
+                        }
+                    }
+                }
+
                 if !block.sweep(self.space, &mut histogram, line_mark_state) {
                     // Block is live. Increment the allocated block count.
                     allocated_blocks += 1;

src/util/copy/mod.rs

@@ -110,8 +110,15 @@ impl<VM: VMBinding> GCWorkerCopyContext<VM> {
     /// * `bytes`: The size of the object in bytes.
     /// * `semantics`: The copy semantic used for the copying.
     pub fn post_copy(&mut self, object: ObjectReference, bytes: usize, semantics: CopySemantics) {
-        // Clear forwarding bits.
-        object_forwarding::clear_forwarding_bits::<VM>(object);
+        if VM::VMObjectModel::LOCAL_FORWARDING_BITS_SPEC.is_in_header() {
+            // Clear forwarding bits if the forwarding bits are in the header.
+            object_forwarding::clear_forwarding_bits::<VM>(object);
+        } else {
+            // We ensure no stale side forwarding bits exist before tracing.
+            debug_assert!(!object_forwarding::is_forwarded_or_being_forwarded::<VM>(
+                object
+            ));
+        }
         // If we are copying objects in mature space, we would need to mark the object as mature.
         if semantics.is_mature() && self.config.constraints.needs_log_bit {
             // If the plan uses unlogged bit, we set the unlogged bit (the object is unlogged/mature)

src/util/metadata/side_metadata/global.rs

@@ -1080,8 +1080,6 @@ impl SideMetadataContext {
         total
     }
 
-    pub fn reset(&self) {}
-
     // ** NOTE: **
     //  Regardless of the number of bits in a metadata unit, we always represent its content as a word.