From 3b83846bfcee6c4967a5e2bbe639f3a1471c6496 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Mon, 14 Sep 2026 01:52:04 +0200 Subject: [PATCH 1/2] fix(gc): report actual restore coverage work --- changelog.d/9877-restore-coverage-metrics.md | 5 + crates/perry-runtime/src/gc/tests/mod.rs | 1 + .../src/gc/tests/restore_coverage.rs | 115 ++++++++++++++++++ crates/perry-runtime/src/gc/verify.rs | 55 +++++++-- 4 files changed, 169 insertions(+), 7 deletions(-) create mode 100644 changelog.d/9877-restore-coverage-metrics.md create mode 100644 crates/perry-runtime/src/gc/tests/restore_coverage.rs diff --git a/changelog.d/9877-restore-coverage-metrics.md b/changelog.d/9877-restore-coverage-metrics.md new file mode 100644 index 0000000000..c19d8fdb6e --- /dev/null +++ b/changelog.d/9877-restore-coverage-metrics.md @@ -0,0 +1,5 @@ +Correct the `[gc-restore-coverage]` diagnostic to report the actual old-page input (`dirty_old_pages`) separately from raw external entries and the covered-object skip set. The former `dirty_pages` field included external pages that the old-arena walk did not traverse. + +Add admitted parent visits, enumerated slots, and strong slots whose children still require tracking. Slot productivity counts edges even when their page was already dirty; it is separate from `pages_added`. These counters compile out of the diagnostics-off walk, and collection/remembered-set behavior is unchanged. + +A subprocess regression exercises diagnostics on and off, unequal old/external page inputs, duplicate stale external owners, skipped parents, mixed primitive/old/young slots, and repeated repair of an already-dirty page. diff --git a/crates/perry-runtime/src/gc/tests/mod.rs b/crates/perry-runtime/src/gc/tests/mod.rs index b7555bde71..ec960abbbc 100644 --- a/crates/perry-runtime/src/gc/tests/mod.rs +++ b/crates/perry-runtime/src/gc/tests/mod.rs @@ -50,6 +50,7 @@ mod oldgen; mod os_tag; mod promote_in_place; mod proxy_registry; +mod restore_coverage; mod retention_9628_9629; mod root_words; mod rooted_container_values; diff --git a/crates/perry-runtime/src/gc/tests/restore_coverage.rs b/crates/perry-runtime/src/gc/tests/restore_coverage.rs new file mode 100644 index 0000000000..45b3ceabfa --- /dev/null +++ b/crates/perry-runtime/src/gc/tests/restore_coverage.rs @@ -0,0 +1,115 @@ +//! #9877: the diagnostic must describe the actual repair walk and its slots. +use super::super::*; +use super::support::*; + +fn exercise_restore() { + let _isolation = copying_nursery_isolation_lock(); + let _trigger = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + reset_remembered_set(); + + unsafe { + // Two walked parents have four slots, but only two young edges. + // The third parent's primitive slot is already covered by the scan. + let (one, one_slot) = alloc_old_test_object(1); + let (three, three_slots) = alloc_old_test_object(3); + let (skipped, skipped_slot) = alloc_old_test_object(1); + let (young, _) = alloc_nursery_test_object(0); + assert!(!crate::arena::pointer_in_old_gen(young as usize)); + let young_bits = POINTER_TAG | (young as u64 & POINTER_MASK); + *one_slot = young_bits; + *three_slots = young_bits; + *three_slots.add(1) = POINTER_TAG | (one as u64 & POINTER_MASK); + *three_slots.add(2) = 42.0f64.to_bits(); + *skipped_slot = 7.0f64.to_bits(); + + let page = crate::arena::generation_page_for_addr(one as usize); + assert_eq!(page, crate::arena::generation_page_for_addr(three as usize)); + assert_eq!( + page, + crate::arena::generation_page_for_addr(skipped as usize) + ); + let snapshot = RememberedDirtySnapshot { + dirty_old_pages: [page].into_iter().collect(), + // A repeated stale external owner is deduplicated, then rejected + // without dereferencing it. These pages are not old walk inputs. + external_dirty_entries: vec![(page + 1, 0), (page + 2, 0)], + dirty_pages: [page, page + 1, page + 2].into_iter().collect(), + fallback_headers: Vec::new(), + }; + let covered = [header_from_user_ptr(skipped as *const u8) as usize] + .into_iter() + .collect(); + remembered_set_clear(); + restore_surviving_dirty_coverage(&snapshot, &covered, "first"); + assert!( + barrier::DIRTY_OLD_PAGES.with(|s| s.borrow().contains(&page)), + "repair must restore the page containing the young edges" + ); + // Productivity counts edges, even when their page is already dirty. + restore_surviving_dirty_coverage(&snapshot, &covered, "repeat"); + assert_eq!(barrier::DIRTY_OLD_PAGES.with(|s| s.borrow().len()), 1); + } + remembered_set_clear(); + println!("restore coverage witness completed"); +} + +fn field(line: &str, name: &str) -> usize { + let prefix = format!("{name}="); + line.split_whitespace() + .find_map(|word| word.strip_prefix(&prefix)) + .and_then(|value| value.parse().ok()) + .unwrap_or_else(|| panic!("missing numeric {name} in {line}")) +} + +#[test] +fn restore_coverage_diagnostic_matches_walk_and_off_arm() { + const CHILD: &str = "PERRY_TEST_RESTORE_COVERAGE_CHILD"; + let thread = std::thread::current(); + let name = thread.name().expect("libtest thread name"); + if std::env::var(CHILD).ok().as_deref() == Some(name) { + exercise_restore(); + return; + } + for enabled in ["0", "1"] { + let output = std::process::Command::new(std::env::current_exe().unwrap()) + .args(["--exact", name, "--nocapture", "--test-threads=1"]) + .env(CHILD, name) + .env("PERRY_GC_DIAG", enabled) + .output() + .expect("run isolated diagnostic witness"); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + output.status.success() && stdout.contains("restore coverage witness completed"), + "witness failed (diag={enabled}): {}\n{stdout}\n{stderr}", + output.status + ); + let lines: Vec<_> = stderr + .lines() + .filter(|line| line.starts_with("[gc-restore-coverage] ")) + .collect(); + if enabled == "0" { + assert!( + lines.is_empty(), + "diagnostics off must stay silent: {stderr}" + ); + continue; + } + assert_eq!(lines.len(), 2, "one diagnostic per repair: {stderr}"); + for (line, added) in lines.iter().zip([1, 0]) { + assert_eq!(field(line, "dirty_old_pages"), 1); + assert_eq!(field(line, "external_entries"), 2); + assert_eq!(field(line, "covered"), 1); + assert_eq!(field(line, "objects_walked"), 3); + assert_eq!(field(line, "objects_skipped"), 1); + assert_eq!(field(line, "parents_visited"), 2); + assert_eq!(field(line, "slots_visited"), 4); + assert_eq!(field(line, "slots_tracking"), 2); + assert_eq!(field(line, "pages_added"), added); + assert!( + !line.contains(" dirty_pages="), + "do not mislabel the walk input" + ); + } + } +} diff --git a/crates/perry-runtime/src/gc/verify.rs b/crates/perry-runtime/src/gc/verify.rs index f9add564d7..36e6038046 100644 --- a/crates/perry-runtime/src/gc/verify.rs +++ b/crates/perry-runtime/src/gc/verify.rs @@ -232,9 +232,9 @@ pub(super) unsafe fn remember_evacuated_old_to_young_slot( sticky: &mut StickyRememberedSet, parent_header: *mut GcHeader, slot: *mut u64, -) { +) -> bool { if slot.is_null() { - return; + return false; } let child_addr = decode_heap_addr(*slot); // Nursery AND malloc-GC children both need their pages kept dirty: @@ -242,13 +242,17 @@ pub(super) unsafe fn remember_evacuated_old_to_young_slot( // leaves — dropping an old→malloc page here would free the malloc // child on the next minor (see remembered_child_needs_tracking). if child_addr == 0 || !crate::gc::barrier::remembered_child_needs_tracking(child_addr) { - return; + return false; } sticky.remember_slot( parent_header, slot, slot_is_external_to(parent_header, slot), ); + // Report the child's tracking requirement, not whether its page was new. + // The repair diagnostic can count productive slots without decoding and + // classifying the same child a second time. + true } /// Is `slot` outside `parent_header`'s own allocation, or on a page the @@ -336,10 +340,27 @@ pub(super) fn restore_surviving_dirty_coverage( snapshot: &RememberedDirtySnapshot, covered: &crate::fast_hash::PtrHashSet, cycle_label: &str, +) { + // Keep slot accounting out of the normal GC walk. Both instantiations + // perform exactly the same repair; only the diagnostic one counts it. + if crate::gc::gc_diag_enabled() { + restore_surviving_dirty_coverage_impl::(snapshot, covered, cycle_label); + } else { + restore_surviving_dirty_coverage_impl::(snapshot, covered, cycle_label); + } +} + +fn restore_surviving_dirty_coverage_impl( + snapshot: &RememberedDirtySnapshot, + covered: &crate::fast_hash::PtrHashSet, + cycle_label: &str, ) { let mut sticky = StickyRememberedSet::default(); let mut walked = 0usize; let mut skipped = 0usize; + let mut parents_visited = 0usize; + let mut slots_visited = 0usize; + let mut slots_tracking = 0usize; #[cfg(debug_assertions)] let mut skipped_sticky = StickyRememberedSet::default(); // Mirror scan_remembered_dirty_slots_copying's scan_header guards: the @@ -367,12 +388,23 @@ pub(super) fn restore_surviving_dirty_coverage( { return; } + if DIAGNOSTICS { + parents_visited += 1; + } visit_gc_rewrite_slots(header, |slot| { + if DIAGNOSTICS { + // Count all enumerated slots, including unproductive weak + // targets and primitive values. This measures traversal work. + slots_visited += 1; + } if crate::weakref::is_weak_target_trace_slot(header, slot.slot) { return; } slot.record_layout_read(); - remember_evacuated_old_to_young_slot(&mut sticky, header, slot.slot); + let tracking = remember_evacuated_old_to_young_slot(&mut sticky, header, slot.slot); + if DIAGNOSTICS && tracking { + slots_tracking += 1; + } }); }; if !snapshot.dirty_old_pages.is_empty() { @@ -429,10 +461,19 @@ pub(super) fn restore_surviving_dirty_coverage( object `scan_dirty_object_slots` reported complete" ); } - if crate::gc::gc_diag_enabled() { + if DIAGNOSTICS { + // These are the two actual snapshot inputs and the skip-set size. + // `dirty_pages` also includes external pages and is NOT the set the + // old-arena walk iterates. Candidate counts precede validity guards; + // parent visits and slot counts describe the admitted traversal. eprintln!( - "[gc-restore-coverage] {cycle_label} dirty_pages={} objects_walked={walked} objects_skipped={skipped} pages_added={added}", - snapshot.dirty_pages.len() + "[gc-restore-coverage] {cycle_label} dirty_old_pages={} external_entries={} \ + covered={} objects_walked={walked} objects_skipped={skipped} \ + parents_visited={parents_visited} slots_visited={slots_visited} \ + slots_tracking={slots_tracking} pages_added={added}", + snapshot.dirty_old_pages.len(), + snapshot.external_dirty_entries.len(), + covered.len(), ); } } From 7f187533c9e432a1514f7e83b514eb09e0d4c3d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Mon, 14 Sep 2026 01:52:38 +0200 Subject: [PATCH 2/2] docs: key restore coverage changelog to PR 10237 --- ...tore-coverage-metrics.md => 10237-restore-coverage-metrics.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{9877-restore-coverage-metrics.md => 10237-restore-coverage-metrics.md} (100%) diff --git a/changelog.d/9877-restore-coverage-metrics.md b/changelog.d/10237-restore-coverage-metrics.md similarity index 100% rename from changelog.d/9877-restore-coverage-metrics.md rename to changelog.d/10237-restore-coverage-metrics.md