diff --git a/changelog.d/7043-heap-min-diagnostic.md b/changelog.d/7043-heap-min-diagnostic.md new file mode 100644 index 0000000000..0fd167f080 --- /dev/null +++ b/changelog.d/7043-heap-min-diagnostic.md @@ -0,0 +1,15 @@ +Two GC diagnostics salvaged from the #7022 investigation. + +**`js_array_grow`'s below-`HEAP_MIN` stub skip is no longer silent.** Whether a +growth forwarding stub gets installed depends on where the allocator placed the +array — an address-conditional behaviour change with no signal. It now fires a +`debug_assert` and a once-per-process stderr line. That silence already cost real +debugging time: an experiment that replaced arena blocks with `mmap`'d guard-paged +blocks used a NULL hint, landed below the platform floor, silently disabled every +growth stub, and came back falsely clean. + +**The from-space scan reports snapshot coverage.** Offending slots are now split +into `not_in_snapshot` (the page was never in this cycle's dirty snapshot, so the +in-cycle remembered-set scan never looked at it) and `in_snapshot` (the scan +looked at the page and still did not rewrite the slot). That split is what +established #7022's failure as in-cycle rather than a dropped-then-restored edge. diff --git a/crates/perry-runtime/src/array/push_pop.rs b/crates/perry-runtime/src/array/push_pop.rs index b286c52f28..8b53b4a945 100644 --- a/crates/perry-runtime/src/array/push_pop.rs +++ b/crates/perry-runtime/src/array/push_pop.rs @@ -145,6 +145,8 @@ pub extern "C" fn js_array_grow(arr: *mut ArrayHeader, min_capacity: u32) -> *mu if (*old_header).obj_type == crate::gc::GC_TYPE_ARRAY { crate::gc::set_forwarding_address(old_header, new_ptr as *mut u8); } + } else { + report_growth_stub_skipped_below_heap_min(arr as usize); } new_ptr @@ -823,3 +825,38 @@ pub extern "C" fn js_array_unshift_variadic( #[cfg_attr(feature = "keepalive-anchors", used)] static KEEP_UNSHIFT_VARIADIC: extern "C" fn(*mut ArrayHeader, *const f64, u32) -> *mut ArrayHeader = js_array_unshift_variadic; + +/// The `HEAP_MIN` guard above is an **address-conditional silent divergence**: +/// whether a growth forwarding stub is installed depends on where the allocator +/// happened to place the array. Below the floor the stub is skipped, so a stale +/// pre-grow reference stops resolving (issue #233's whole mechanism) — with no +/// signal at all. +/// +/// That silence has already cost real debugging time. While investigating +/// #7022 an experiment replaced arena blocks with `mmap`'d, guard-paged blocks; +/// `mmap` with a NULL hint lands well below macOS's 2 TB floor, so this branch +/// silently disabled every growth stub and the experiment came back **falsely +/// clean**. It was only caught by re-running with a high `MAP_FIXED` hint. +/// +/// Emit once per process so the next person gets a signal instead of a silent +/// behaviour change. One line on stderr, so it cannot perturb a stdout parity +/// comparison. +#[cold] +fn report_growth_stub_skipped_below_heap_min(arr_addr: usize) { + use std::sync::atomic::{AtomicBool, Ordering}; + static REPORTED: AtomicBool = AtomicBool::new(false); + debug_assert!( + false, + "array-growth forwarding stub skipped: array at {arr_addr:#x} is below the platform heap floor" + ); + if REPORTED.swap(true, Ordering::Relaxed) { + return; + } + eprintln!( + "[perry-gc] array-growth forwarding stub SKIPPED for an array at {arr_addr:#x}: \ +address is below this platform's heap floor. Stale pre-grow array references \ +will no longer resolve through the growth chain (issue #233). This is normally \ +unreachable; it usually means the arena is being backed by an allocator that \ +places blocks outside the expected range. Reported once per process." + ); +} diff --git a/crates/perry-runtime/src/gc/copying.rs b/crates/perry-runtime/src/gc/copying.rs index 62f9ca5094..01a4091524 100644 --- a/crates/perry-runtime/src/gc/copying.rs +++ b/crates/perry-runtime/src/gc/copying.rs @@ -1195,7 +1195,7 @@ pub(super) fn gc_collect_minor_copying_fast_path_with_eligibility( // #7035: whole-heap from-space scan. MUST run here — after the rewrite // pass, before from-space is reset — and it is deliberately independent of // the root enumeration the rewrite pass and the evacuation verifier share. - super::fromspace_scan::run_fromspace_scan(); + super::fromspace_scan::run_fromspace_scan(&snapshot); crate::promise::cleanup_copied_minor_promise_contexts_for_gc(); finalize_dead_copied_minor_from_space_side_allocations(); diff --git a/crates/perry-runtime/src/gc/fromspace_scan.rs b/crates/perry-runtime/src/gc/fromspace_scan.rs index a472bae291..49b218d8de 100644 --- a/crates/perry-runtime/src/gc/fromspace_scan.rs +++ b/crates/perry-runtime/src/gc/fromspace_scan.rs @@ -86,6 +86,12 @@ pub(crate) struct FromSpaceScanReport { /// Offending slots whose page IS currently dirty -> the remembered set had /// it and the scan still missed the slot. pub(crate) dirty_but_missed: usize, + /// Offending slots whose page was NOT in this cycle's dirty snapshot, so + /// the in-cycle remembered-set scan never looked at them. + pub(crate) not_in_snapshot: usize, + /// Offending slots whose page WAS in the snapshot -- the scan looked at the + /// page and still did not rewrite the slot. + pub(crate) in_snapshot: usize, pub(crate) distinct_owners: crate::fast_hash::PtrHashSet, pub(crate) samples: Vec, } @@ -172,6 +178,14 @@ unsafe fn scan_object(header: *mut GcHeader, report: &mut FromSpaceScanReport) { let slot_addr = words.add(i) as usize; let dirty_now = super::barrier::dirty_now_for_addr(slot_addr); let ever_dirty = super::barrier::ever_dirty_for_addr(slot_addr); + if SNAPSHOT_PAGES.with(|c| { + c.borrow() + .contains(&crate::arena::generation_page_for_addr(slot_addr)) + }) { + report.in_snapshot += 1; + } else { + report.not_in_snapshot += 1; + } if !ever_dirty { report.never_dirty += 1; } else if !dirty_now { @@ -285,6 +299,10 @@ pub(super) fn emit_report(report: &FromSpaceScanReport, phase: &str) { report.lost_dirty, report.dirty_but_missed ); + eprintln!( + "[gc-fromspace-scan {}] in_snapshot={} not_in_snapshot={}", + phase, report.in_snapshot, report.not_in_snapshot + ); for sample in &report.samples { eprintln!("{}", describe(sample)); } @@ -292,10 +310,13 @@ pub(super) fn emit_report(report: &FromSpaceScanReport, phase: &str) { /// Entry point called from the copying minor, after the rewrite pass and before /// `copying_reset_from_spaces_and_flip`. -pub(super) fn run_fromspace_scan() { +pub(super) fn run_fromspace_scan(snapshot: &super::RememberedDirtySnapshot) { if !fromspace_scan_enabled() { return; } + SNAPSHOT_PAGES.with(|c| { + *c.borrow_mut() = snapshot.dirty_old_pages.iter().copied().collect(); + }); let report = scan_heap_for_fromspace_refs(); if report.missing_rewrites > 0 || report.dangling > 0 { emit_report(&report, "OFFENDERS"); @@ -303,3 +324,8 @@ pub(super) fn run_fromspace_scan() { emit_report(&report, "clean"); } } + +thread_local! { + static SNAPSHOT_PAGES: std::cell::RefCell> = + std::cell::RefCell::new(crate::fast_hash::new_ptr_hash_set()); +}