Make GC triggering and heap resizing consistent (mmtk#1266)

This PR fixes a bug where the MemBalancer did not increase the heap size
enough to accommodate the amount of side metadata needed by the pending
allocation. It manifested as looping infinitely between triggering GC
and (not actually) resizing the heap size after a GC when the minimum
heap size is too small.

Now it always includes the side metadata amount when increasing heap
size.

This PR also refactors the calculation of "shifting right and rounding
up" which is used in multiple places. We also replace `alloc_rshift`
with `log_data_meta_ratio` for two reasons. (1) The previous
implementation would cause unsigned overflow before converting the
result to `i32`. (2) `log_data_meta_ratio` has clearer semantics.

Author: wks

src/policy/lockfreeimmortalspace.rs

@@ -129,10 +129,14 @@ impl<VM: VMBinding> Space<VM> for LockFreeImmortalSpace<VM> {
         unsafe { sft_map.eager_initialize(self.as_sft(), self.start, self.total_bytes) };
     }
 
+    fn estimate_side_meta_pages(&self, data_pages: usize) -> usize {
+        self.metadata.calculate_reserved_pages(data_pages)
+    }
+
     fn reserved_pages(&self) -> usize {
         let cursor = self.cursor.load(Ordering::Relaxed);
         let data_pages = conversions::bytes_to_pages_up(self.limit - cursor);
-        let meta_pages = self.metadata.calculate_reserved_pages(data_pages);
+        let meta_pages = self.estimate_side_meta_pages(data_pages);
         data_pages + meta_pages
     }
 

src/policy/marksweepspace/malloc_ms/global.rs

@@ -215,14 +215,18 @@ impl<VM: VMBinding> Space<VM> for MallocSpace<VM> {
         "MallocSpace"
     }
 
+    fn estimate_side_meta_pages(&self, data_pages: usize) -> usize {
+        self.metadata.calculate_reserved_pages(data_pages)
+    }
+
     #[allow(clippy::assertions_on_constants)]
     fn reserved_pages(&self) -> usize {
         use crate::util::constants::LOG_BYTES_IN_PAGE;
         // Assume malloc pages are no smaller than 4K pages. Otherwise the substraction below will fail.
         debug_assert!(LOG_BYTES_IN_MALLOC_PAGE >= LOG_BYTES_IN_PAGE);
         let data_pages = self.active_pages.load(Ordering::SeqCst)
             << (LOG_BYTES_IN_MALLOC_PAGE - LOG_BYTES_IN_PAGE);
-        let meta_pages = self.metadata.calculate_reserved_pages(data_pages);
+        let meta_pages = self.estimate_side_meta_pages(data_pages);
         data_pages + meta_pages
     }
 

src/policy/space.rs

@@ -109,9 +109,12 @@ pub trait Space<VM: VMBinding>: 'static + SFT + Sync + Downcast {
 
             // Clear the request, and inform GC trigger about the pending allocation.
             pr.clear_request(pages_reserved);
+
+            let meta_pages_reserved = self.estimate_side_meta_pages(pages_reserved);
+            let total_pages_reserved = pages_reserved + meta_pages_reserved;
             self.get_gc_trigger()
                 .policy
-                .on_pending_allocation(pages_reserved);
+                .on_pending_allocation(total_pages_reserved);
 
             VM::VMCollection::block_for_gc(VMMutatorThread(tls)); // We have checked that this is mutator
             unsafe { Address::zero() }
@@ -313,9 +316,20 @@ pub trait Space<VM: VMBinding>: 'static + SFT + Sync + Downcast {
             .mark_as_mapped(self.common().start, self.common().extent);
     }
 
+    /// Estimate the amount of side metadata memory needed for a give data memory size in pages. The
+    /// result will over-estimate the amount of metadata pages needed, with at least one page per
+    /// side metadata.  This relatively accurately describes the number of side metadata pages the
+    /// space actually consumes.
+    ///
+    /// This function is used for both triggering GC (via [`Space::reserved_pages`]) and resizing
+    /// the heap (via [`crate::util::heap::GCTriggerPolicy::on_pending_allocation`]).
+    fn estimate_side_meta_pages(&self, data_pages: usize) -> usize {
+        self.common().metadata.calculate_reserved_pages(data_pages)
+    }
+
     fn reserved_pages(&self) -> usize {
         let data_pages = self.get_page_resource().reserved_pages();
-        let meta_pages = self.common().metadata.calculate_reserved_pages(data_pages);
+        let meta_pages = self.estimate_side_meta_pages(data_pages);
         data_pages + meta_pages
     }
 

src/util/conversions.rs

@@ -94,6 +94,15 @@ pub fn bytes_to_formatted_string(bytes: usize) -> String {
     format!("{}{}", num, UNITS.last().unwrap())
 }
 
+/// Shift `num` by `bits` to the right.  Add 1 to the result if any `1` bits are shifted out to the
+/// right.  This is equivalent to dividing `num` by 2 to the power of `bits`, rounding up.
+///
+/// This function has undefined behavior if `bits` is greater or equal to the number of bits in
+/// `usize`.
+pub const fn rshift_align_up(num: usize, bits: usize) -> usize {
+    (num + ((1 << bits) - 1)) >> bits
+}
+
 #[cfg(test)]
 mod tests {
     use crate::util::conversions::*;

src/util/metadata/side_metadata/global.rs

@@ -1362,12 +1362,13 @@ impl SideMetadataContext {
     pub fn calculate_reserved_pages(&self, data_pages: usize) -> usize {
         let mut total = 0;
         for spec in self.global.iter() {
-            let rshift = addr_rshift(spec);
-            total += (data_pages + ((1 << rshift) - 1)) >> rshift;
+            // This rounds up.  No matter how small `data_pages` is, the side metadata size will be
+            // at least one page.  This behavior is *intended*.  This over-estimated amount is used
+            // for triggering GC and resizing the heap.
+            total += data_to_meta_size_round_up(spec, data_pages);
         }
         for spec in self.local.iter() {
-            let rshift = addr_rshift(spec);
-            total += (data_pages + ((1 << rshift) - 1)) >> rshift;
+            total += data_to_meta_size_round_up(spec, data_pages);
         }
         total
     }

src/util/metadata/side_metadata/helpers.rs

@@ -2,6 +2,7 @@ use super::ranges::BitOffset;
 use super::SideMetadataSpec;
 use crate::util::constants::LOG_BYTES_IN_PAGE;
 use crate::util::constants::{BITS_IN_WORD, BYTES_IN_PAGE, LOG_BITS_IN_BYTE};
+use crate::util::conversions::rshift_align_up;
 use crate::util::heap::layout::vm_layout::VMLayout;
 use crate::util::memory::{MmapAnnotation, MmapStrategy};
 #[cfg(target_pointer_width = "32")]
@@ -110,7 +111,7 @@ pub(crate) fn ensure_munmap_contiguous_metadata_space(
     let metadata_start = address_to_meta_address(spec, start);
     let mmap_start = metadata_start.align_down(BYTES_IN_PAGE);
     // nearest page-aligned ending address
-    let metadata_size = (size + ((1 << addr_rshift(spec)) - 1)) >> addr_rshift(spec);
+    let metadata_size = data_to_meta_size_round_up(spec, size);
     let mmap_size = (metadata_start + metadata_size).align_up(BYTES_IN_PAGE) - mmap_start;
     if mmap_size > 0 {
         ensure_munmap_metadata(mmap_start, mmap_size);
@@ -135,7 +136,7 @@ pub(super) fn try_mmap_contiguous_metadata_space(
     let metadata_start = address_to_meta_address(spec, start);
     let mmap_start = metadata_start.align_down(BYTES_IN_PAGE);
     // nearest page-aligned ending address
-    let metadata_size = (size + ((1 << addr_rshift(spec)) - 1)) >> addr_rshift(spec);
+    let metadata_size = data_to_meta_size_round_up(spec, size);
     let mmap_size = (metadata_start + metadata_size).align_up(BYTES_IN_PAGE) - mmap_start;
     if mmap_size > 0 {
         if !no_reserve {
@@ -185,14 +186,42 @@ pub(crate) fn address_to_meta_address(
     res
 }
 
-pub(super) const fn addr_rshift(metadata_spec: &SideMetadataSpec) -> i32 {
-    ((LOG_BITS_IN_BYTE as usize) + metadata_spec.log_bytes_in_region
-        - (metadata_spec.log_num_of_bits)) as i32
+/// Return the base-2 logarithm of the ratio of data bits and metadata bits per region.
+///
+/// Suppose a memory region has `data_bits` bits of data, and `meta_bits` bits of metadata for
+/// `metadata_spec`, and the result of `log_data_meta_ratio(metadata_spec)` is `shift`, then
+///
+/// -   `data_bits >> shift == meta_bits`
+/// -   `meta_bits << shift == data_bits`
+pub(super) const fn log_data_meta_ratio(metadata_spec: &SideMetadataSpec) -> usize {
+    let log_data_bits_in_region = (LOG_BITS_IN_BYTE as usize) + metadata_spec.log_bytes_in_region;
+    let log_meta_bits_in_region = metadata_spec.log_num_of_bits;
+
+    // TODO: In theory, it is possible to construct a side metadata that has more metadata bits than
+    // data bits per region.  But such pathological side metadata consumes way too much memory, and
+    // should never be used in any useful applications.  It should be forbidden.
+    log_data_bits_in_region - log_meta_bits_in_region
+}
+
+/// Calculate the amount of metadata needed for the give amount of data memory, round up to nearest
+/// integer. `data_size` can be in any unit, e.g. bits, bytes, pages, blocks, chunks, etc., and the
+/// result has the same unit.
+pub(super) const fn data_to_meta_size_round_up(
+    metadata_spec: &SideMetadataSpec,
+    data_size: usize,
+) -> usize {
+    rshift_align_up(data_size, log_data_meta_ratio(metadata_spec))
+}
+
+/// Calculate the amount of data governed by the give amount of metadata.  `meta_size` can be in any
+/// unit, e.g. bits, bytes, pages, blocks, chunks, etc., and the result has the same unit.
+pub(super) const fn meta_to_data_size(metadata_spec: &SideMetadataSpec, meta_size: usize) -> usize {
+    meta_size << log_data_meta_ratio(metadata_spec)
 }
 
 #[allow(dead_code)]
 pub(super) const fn metadata_address_range_size(metadata_spec: &SideMetadataSpec) -> usize {
-    1usize << (VMLayout::LOG_ARCH_ADDRESS_SPACE - addr_rshift(metadata_spec) as usize)
+    1usize << (VMLayout::LOG_ARCH_ADDRESS_SPACE - log_data_meta_ratio(metadata_spec))
 }
 
 pub(super) fn meta_byte_lshift(metadata_spec: &SideMetadataSpec, data_addr: Address) -> u8 {