From 9a2b6fd57d9d0414c89f71023b4b81735adb7b07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 12 Aug 2026 11:17:31 +0200 Subject: [PATCH] fix(gc): bound the atomic work inside a time-budgeted incremental step (#7903) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `js_gc_step_us` consults its clock BETWEEN work units and never during one, so the budget it advertises is exactly as strong as the most expensive single unit. Two units were unbounded. Weak processing charged one unit per registered HOLDER, and a FinalizationRegistry is one holder however many records it owns — so `process_finreg_after_mark` walked an arbitrarily long record array inside that one unit. Weak processing now charges one unit per RECORD and keeps a cursor into the array (`weakref/sliced.rs`). The previous atomicity was not accidental: `unregister` REBUILDS the entries array without the matching records, so every index after a removed element shifts down, and a resumed index-only cursor would skip exactly as many records as were removed before it. A skipped record is a weak slot that never gets tombstoned, whose target is then swept — a use-after-free, not a latency bug. The cursor therefore carries the identity of the array it indexes (the `entries` field's value word plus that array's length); both mutation paths change one of the two, so a mismatch on resume restarts that registry's scan from 0. Rescanning is idempotent. Restarts are capped, and past the cap a registry is finished in one atomic pass and charged as such, so the worst case is stated rather than implied. Final root remark stays deliberately atomic — slicing the drain invalidates the remark itself, and yielding after the liveness decision is the #7900 race — but it is now MEASURED rather than claimed, and reported separately from the general step maximum so a heap-sized pause cannot hide behind "the collector's worst step". Adds `[gc-step-bounds]` on the existing PERRY_GC_DIAG path (no new env knob, per the GC knob kill-policy). `weak_steps_sliced` is the subject-was-live counter; the five new tests build the pathological registry directly and assert it before asserting any bound. --- .../7938-bounded-budgeted-step-work.md | 70 +++++ crates/perry-runtime/src/gc/cycle.rs | 35 +-- crates/perry-runtime/src/gc/instruments.rs | 154 ++++++++++ crates/perry-runtime/src/gc/mod.rs | 37 +++ crates/perry-runtime/src/gc/policy.rs | 7 + crates/perry-runtime/src/gc/tests/mod.rs | 1 + .../perry-runtime/src/gc/tests/step_bounds.rs | 264 ++++++++++++++++ crates/perry-runtime/src/weakref.rs | 125 ++++---- crates/perry-runtime/src/weakref/sliced.rs | 286 ++++++++++++++++++ docs/src/SUMMARY.md | 1 + docs/src/internals/gc-step-bounds.md | 108 +++++++ 11 files changed, 1004 insertions(+), 84 deletions(-) create mode 100644 changelog.d/7938-bounded-budgeted-step-work.md create mode 100644 crates/perry-runtime/src/gc/tests/step_bounds.rs create mode 100644 crates/perry-runtime/src/weakref/sliced.rs create mode 100644 docs/src/internals/gc-step-bounds.md diff --git a/changelog.d/7938-bounded-budgeted-step-work.md b/changelog.d/7938-bounded-budgeted-step-work.md new file mode 100644 index 0000000000..0e887c2166 --- /dev/null +++ b/changelog.d/7938-bounded-budgeted-step-work.md @@ -0,0 +1,70 @@ +### Fixed + +- **GC: a time-budgeted incremental step no longer contains an unbounded atomic + work unit (#7903).** `js_gc_step_us` consults its clock *between* work units + and never during one, so the budget it advertises is exactly as strong as the + most expensive single unit. Weak processing charged **one unit per registered + holder** — and a `FinalizationRegistry` is one holder however many records it + owns, so `process_finreg_after_mark` walked an arbitrarily long record array + inside that one unit. A single registry with a million registrations was an + atomic, heap-sized "work unit" sitting behind a time-budgeted API, and no + amount of tightening the microsecond budget could reach it. + + Weak processing now charges **one unit per record** and keeps a cursor *into* + the record array (`crates/perry-runtime/src/weakref/sliced.rs`). + + The previous atomicity was not accidental, and preserving what it protected is + most of the change. `FinalizationRegistry.prototype.unregister` **rebuilds** + the entries array without the matching records, so every index after a removed + element shifts down; a resumed index-only cursor would skip exactly as many + records as were removed before it, and a skipped record is a weak slot that + never gets tombstoned — on a non-moving budgeted cycle its target is then + swept and the slot dangles. That is a use-after-free, not a latency bug. + + So the cursor is validated rather than trusted: alongside the record index it + carries the *identity* of the array it indexes (the value word of the + registry's `entries` field plus that array's length). Both mutator mutation + paths change one of the two — `unregister` installs a freshly built array, + `register` pushes — so on resume a mismatch means the held indices are + meaningless and that registry's scan restarts from 0 against the new array. + Restarting is safe because a rescan is idempotent: the first pass writes + `undefined` into a collected record's target and `false` into its pending flag + after enqueueing, so a second pass enqueues nothing and clears nothing twice. + Restart-on-mutation alone is livelock-shaped, so restarts are capped; past the + cap a registry is finished in one atomic pass **and charged as such**. The + worst case is therefore stated rather than implied. + +- **Final root remark is now measured instead of claimed (#7903).** + `AtomicFinalizeSubphase::FinalRootRemark` re-scans the roots and then drains + everything they newly reach, both at `usize::MAX`. Its inline comment claimed + the phase was "bounded by root-set size, not heap size" — true of the scan, + **false of the drain**, since a root installed after the initial scan can + anchor an arbitrarily large graph. Both ways to bound it were rejected on + correctness grounds: yielding mid-drain invalidates the remark itself (the + mutator installs new roots, so the scan must repeat, with no termination + guarantee), and yielding after the liveness decision is the weak-read race + tracked in #7900. The phase therefore stays deliberately atomic, is documented + as such, and its cost is now reported separately from the general step maximum + so a heap-sized pause cannot hide behind "the collector's worst step". + +### Added + +- **`[gc-step-bounds]` diagnostic line (#7903).** Emitted on the existing + `PERRY_GC_DIAG=1` path — **no new environment knob**, per the GC knob + kill-policy. Reports `step_max_us`, `final_remark_max_us` / `final_remarks`, + `weak_records` / `weak_max_records_per_step`, `weak_steps_sliced`, and + `weak_registry_restarts` / `weak_registry_atomic_finishes`. + + `weak_steps_sliced` is the **subject-was-live** counter: a run reporting `0` + has not exercised the sliced path at all, whatever else it reports. Most + programs register no weak holders and will legitimately report zeros across + the whole line, which is exactly why the five new acceptance tests + (`crates/perry-runtime/src/gc/tests/step_bounds.rs`) build the pathological + registry directly and assert the liveness counter *before* asserting any + bound — including an adversarial case where the mutator restructures the + entries array in every single window, which must reach the bounded atomic + fallback rather than restarting forever. + +- `docs/src/internals/gc-step-bounds.md` — which collector phases are + intentionally atomic, what each phase's defensible bound is, and how to read + the new line. diff --git a/crates/perry-runtime/src/gc/cycle.rs b/crates/perry-runtime/src/gc/cycle.rs index 682652ea1f..38fba3906c 100644 --- a/crates/perry-runtime/src/gc/cycle.rs +++ b/crates/perry-runtime/src/gc/cycle.rs @@ -1369,27 +1369,28 @@ impl GcCycleState { if budget == 0 { return; } - // Re-scan every root with the marks nearly final (see the - // enum doc). Reuses the RootScan machinery unbudgeted — - // bounded by root-set size, not heap size. consider_evacuation - // is false: pinning decisions were made in the original scan, - // and budgeted cycles are non-moving anyway. + // Re-scan every root with the marks nearly final (see the enum + // doc). DELIBERATELY ATOMIC and NOT bounded by the step budget: + // the scan is bounded by root-set size, but the drain below is + // bounded by the newly-reachable graph, which a root installed + // after the initial scan can make arbitrarily large. Measured + // rather than claimed — `final_remark_max_us` in + // `[gc-step-bounds]`; docs/src/internals/gc-step-bounds.md has + // the bound and rejected alternatives (#7903). + // consider_evacuation is false: pinning was decided already. { + let _remark_timer = instruments::FinalRemarkTimer::start(); let valid_ptrs = self.valid_ptrs.as_ref().expect("valid pointer set built"); let minor_only = self.minor.is_some(); let remark_scan = self.root_scan.get_or_insert_with(RootScanCycleState::new); - loop { - if remark_scan.step_current_subphase( - valid_ptrs, - &mut self.trace, - /* consider_evacuation = */ false, - usize::MAX, - /* allow_synchronous_scanners = */ true, - minor_only, - ) { - break; - } - } + while !remark_scan.step_current_subphase( + valid_ptrs, + &mut self.trace, + /* consider_evacuation = */ false, + usize::MAX, + /* allow_synchronous_scanners = */ true, + minor_only, + ) {} self.root_scan = None; // Trace everything the remark newly discovered so // WeakProcessing (and the full path's RS rebuild) read a diff --git a/crates/perry-runtime/src/gc/instruments.rs b/crates/perry-runtime/src/gc/instruments.rs index 180ca944e0..64bb2b31d0 100644 --- a/crates/perry-runtime/src/gc/instruments.rs +++ b/crates/perry-runtime/src/gc/instruments.rs @@ -243,3 +243,157 @@ pub fn budgeted_step_skips() -> (u64, u64, u64, u64) { SKIP_RESUME_BLOCKED.load(Ordering::Relaxed), ) } + +// --------------------------------------------------------------------------- +// #7903 — step-boundedness telemetry +// +// `js_gc_step_us` can only check its clock BETWEEN work units, so the honest +// question about a "time-budgeted" collector is not "what budget was requested" +// but "how long did the longest single step actually take, and what was it +// doing". These counters answer that directly, and they are the liveness proof +// for the slicing work: a run where `weak_steps_sliced` is zero has not +// exercised the sliced path at all, however green it looks. +// --------------------------------------------------------------------------- + +/// Longest single `cycle.state.step(...)` observed, microseconds. +static STEP_MAX_US: AtomicU64 = AtomicU64::new(0); +/// Longest single ATOMIC final-remark (root re-scan + transitive drain). +/// +/// Deliberately a separate number from `STEP_MAX_US`: final remark is an +/// intentionally atomic phase whose cost is bounded by the newly-reachable +/// graph, not by the step budget. Folding it into the general maximum would let +/// a heap-sized pause hide behind "the collector's worst step". +static REMARK_MAX_US: AtomicU64 = AtomicU64::new(0); +static REMARK_COUNT: AtomicU64 = AtomicU64::new(0); +/// FinalizationRegistry records scanned during weak processing. +static WEAK_RECORDS: AtomicU64 = AtomicU64::new(0); +/// Most records charged to a single weak-processing step. +static WEAK_MAX_RECORDS_PER_STEP: AtomicU64 = AtomicU64::new(0); +/// Steps that ended PARTWAY THROUGH one registry's record array — the sliced +/// path actually running. Zero means the slicing never happened. +static WEAK_STEPS_SLICED: AtomicU64 = AtomicU64::new(0); +/// A registry's record cursor was invalidated by mutator restructuring. +static WEAK_REGISTRY_RESTARTS: AtomicU64 = AtomicU64::new(0); +/// A registry hit the restart cap and was finished in one atomic pass. +static WEAK_REGISTRY_ATOMIC_FINISHES: AtomicU64 = AtomicU64::new(0); + +#[inline] +fn bump_max(slot: &AtomicU64, value: u64) { + let mut cur = slot.load(Ordering::Relaxed); + while value > cur { + match slot.compare_exchange_weak(cur, value, Ordering::Relaxed, Ordering::Relaxed) { + Ok(_) => return, + Err(observed) => cur = observed, + } + } +} + +/// Record one budgeted step's wall duration. +#[inline] +pub(crate) fn note_budgeted_step_duration(us: u64) { + bump_max(&STEP_MAX_US, us); +} + +/// Record one atomic final-remark's wall duration. +#[inline] +pub(crate) fn note_final_remark_duration(us: u64) { + bump_max(&REMARK_MAX_US, us); + REMARK_COUNT.fetch_add(1, Ordering::Relaxed); +} + +/// Record the FinalizationRegistry records one weak-processing step charged, +/// and whether that step stopped mid-registry. +#[inline] +pub(crate) fn note_weak_step_records(records: u64, sliced: bool) { + if records > 0 { + WEAK_RECORDS.fetch_add(records, Ordering::Relaxed); + bump_max(&WEAK_MAX_RECORDS_PER_STEP, records); + } + if sliced { + WEAK_STEPS_SLICED.fetch_add(1, Ordering::Relaxed); + } +} + +#[inline] +pub(crate) fn note_weak_registry_restart() { + WEAK_REGISTRY_RESTARTS.fetch_add(1, Ordering::Relaxed); +} + +#[inline] +pub(crate) fn note_weak_registry_atomic_finish() { + WEAK_REGISTRY_ATOMIC_FINISHES.fetch_add(1, Ordering::Relaxed); +} + +/// Longest single budgeted step, microseconds. +pub fn step_max_us() -> u64 { + STEP_MAX_US.load(Ordering::Relaxed) +} + +/// Longest single atomic final remark, microseconds. +pub fn final_remark_max_us() -> u64 { + REMARK_MAX_US.load(Ordering::Relaxed) +} + +/// Atomic final remarks performed. +pub fn final_remark_count() -> u64 { + REMARK_COUNT.load(Ordering::Relaxed) +} + +/// FinalizationRegistry records scanned during weak processing. +pub fn weak_records_scanned() -> u64 { + WEAK_RECORDS.load(Ordering::Relaxed) +} + +/// Most records charged to a single weak-processing step. +pub fn weak_max_records_per_step() -> u64 { + WEAK_MAX_RECORDS_PER_STEP.load(Ordering::Relaxed) +} + +/// Steps that ended partway through one registry's record array. +pub fn weak_steps_sliced() -> u64 { + WEAK_STEPS_SLICED.load(Ordering::Relaxed) +} + +/// Record cursors invalidated by mutator restructuring. +pub fn weak_registry_restarts() -> u64 { + WEAK_REGISTRY_RESTARTS.load(Ordering::Relaxed) +} + +/// Registries that hit the restart cap and were finished atomically. +pub fn weak_registry_atomic_finishes() -> u64 { + WEAK_REGISTRY_ATOMIC_FINISHES.load(Ordering::Relaxed) +} + +/// Reset the #7903 step-boundedness counters. Test-only: the process-wide +/// statics would otherwise leak between in-process test cases. +#[cfg(test)] +pub(crate) fn reset_step_bound_counters() { + STEP_MAX_US.store(0, Ordering::Relaxed); + REMARK_MAX_US.store(0, Ordering::Relaxed); + REMARK_COUNT.store(0, Ordering::Relaxed); + WEAK_RECORDS.store(0, Ordering::Relaxed); + WEAK_MAX_RECORDS_PER_STEP.store(0, Ordering::Relaxed); + WEAK_STEPS_SLICED.store(0, Ordering::Relaxed); + WEAK_REGISTRY_RESTARTS.store(0, Ordering::Relaxed); + WEAK_REGISTRY_ATOMIC_FINISHES.store(0, Ordering::Relaxed); +} + +/// Times one atomic final remark and records it on drop. +/// +/// An RAII guard rather than a pair of calls so that an early `return` out of +/// the phase cannot silently drop the sample — an unmeasured atomic phase is +/// exactly the state #7903 exists to end. +pub(crate) struct FinalRemarkTimer(std::time::Instant); + +impl FinalRemarkTimer { + #[inline] + pub(crate) fn start() -> Self { + Self(std::time::Instant::now()) + } +} + +impl Drop for FinalRemarkTimer { + fn drop(&mut self) { + note_final_remark_duration(self.0.elapsed().as_micros().min(u128::from(u64::MAX)) as u64); + } +} diff --git a/crates/perry-runtime/src/gc/mod.rs b/crates/perry-runtime/src/gc/mod.rs index 20a12395ad..ba9455631d 100644 --- a/crates/perry-runtime/src/gc/mod.rs +++ b/crates/perry-runtime/src/gc/mod.rs @@ -1102,6 +1102,43 @@ fn emit_incremental_liveness_diag() { poll_arm::poll_arm_events(), poll_arm::poll_armed_count(), ); + emit_step_bounds_diag(); +} + +/// What the "time-budgeted" collector actually cost, as opposed to what it was +/// asked to cost (#7903). +/// +/// `js_gc_step_us` and mutator assist can only consult the clock BETWEEN work +/// units, so a budget is only as good as the largest single unit. These are the +/// measured maxima plus the liveness counters for the sliced weak path: +/// +/// * `step_max_us` — longest single budgeted step. +/// * `final_remark_max_us` / `final_remarks` — the deliberately ATOMIC phase, +/// reported separately so a heap-sized pause cannot hide inside the general +/// maximum. +/// * `weak_records` / `weak_max_records_per_step` — FinalizationRegistry +/// records scanned, and the worst single step's share of them. Before #7903 +/// one registry was one work unit, so this maximum was the whole registry. +/// * `weak_steps_sliced` — steps that ended PARTWAY THROUGH a registry. **This +/// is the subject-was-live counter**: a run reporting zero has not exercised +/// the sliced path, whatever else it reports. +/// * `weak_registry_restarts` / `weak_registry_atomic_finishes` — cursors +/// invalidated by mutator restructuring, and the bounded fallback taken when +/// one registry exhausted its restart budget. +fn emit_step_bounds_diag() { + eprintln!( + "[gc-step-bounds] step_max_us={} final_remark_max_us={} final_remarks={} \ + weak_records={} weak_max_records_per_step={} weak_steps_sliced={} \ + weak_registry_restarts={} weak_registry_atomic_finishes={}", + instruments::step_max_us(), + instruments::final_remark_max_us(), + instruments::final_remark_count(), + instruments::weak_records_scanned(), + instruments::weak_max_records_per_step(), + instruments::weak_steps_sliced(), + instruments::weak_registry_restarts(), + instruments::weak_registry_atomic_finishes(), + ); } /// Print what the rate-1 schedule endpoint actually did, and **fail the diff --git a/crates/perry-runtime/src/gc/policy.rs b/crates/perry-runtime/src/gc/policy.rs index 18adc7fff6..aaa33facf5 100644 --- a/crates/perry-runtime/src/gc/policy.rs +++ b/crates/perry-runtime/src/gc/policy.rs @@ -3271,7 +3271,14 @@ fn gc_budgeted_step_work_units_inner_with_progress( return BudgetedStepOutcome::Result(gc_idle_step_result()); }; + // #7903: the step's own wall duration, not the budget that was asked + // for. `js_gc_step_us` can only consult its clock BETWEEN units, so the + // only honest statement about pause is a measured maximum. + let step_started = std::time::Instant::now(); let step = cycle.state.step(GcWorkBudget::bounded(work_units)); + super::instruments::note_budgeted_step_duration( + step_started.elapsed().as_micros().min(u128::from(u64::MAX)) as u64, + ); super::instruments::note_incremental_step(); if step.completed { super::instruments::note_incremental_completion(); diff --git a/crates/perry-runtime/src/gc/tests/mod.rs b/crates/perry-runtime/src/gc/tests/mod.rs index 48b61cce99..5895154be6 100644 --- a/crates/perry-runtime/src/gc/tests/mod.rs +++ b/crates/perry-runtime/src/gc/tests/mod.rs @@ -40,6 +40,7 @@ mod scan_fallback; mod schedule; mod shadow_stack_ops; mod smoke; +mod step_bounds; pub(super) mod support; mod teardown; mod telemetry_verifier; diff --git a/crates/perry-runtime/src/gc/tests/step_bounds.rs b/crates/perry-runtime/src/gc/tests/step_bounds.rs new file mode 100644 index 0000000000..0c2857a94a --- /dev/null +++ b/crates/perry-runtime/src/gc/tests/step_bounds.rs @@ -0,0 +1,264 @@ +//! #7903 — a "time-budgeted" step is only as bounded as its largest single +//! work unit. +//! +//! Two units used to be unbounded. Weak processing charged one unit per +//! registered *holder*, and a `FinalizationRegistry` is one holder however many +//! records it owns — so a single registry was an atomic, heap-sized unit behind +//! `js_gc_step_us`. Final root remark re-scans the roots and then drains +//! everything they newly reach, both with `usize::MAX`. +//! +//! These tests are adversarial in the sense the issue asked for: they build the +//! pathological shape rather than assert that an ordinary run stays quiet. +//! Every one of them asserts a **liveness counter** — that the sliced path +//! actually ran — before asserting the bound, because a run where the sliced +//! path never executed reports the same zeros as a run where it worked +//! perfectly. + +use super::super::*; +use super::support::*; + +fn reset_old_reclaim_pressure() { + GC_OLD_RECLAIM_PENDING.with(|pending| pending.set(false)); + GC_LAST_OLD_RECLAIM_IN_USE_BYTES.with(|bytes| bytes.set(0)); +} + +extern "C" fn finreg_step_bounds_callback( + _closure: *const crate::closure::ClosureHeader, + _held: f64, +) -> f64 { + f64::from_bits(crate::value::TAG_UNDEFINED) +} + +/// Root a FinalizationRegistry in shadow slot 0 and give it `records` +/// registrations. Targets are deliberately unrooted: a registry whose records +/// all resolve is the cheap case, and the expensive one is what needs bounding. +fn rooted_registry_with_records(records: usize) { + let cb = crate::closure::js_closure_alloc(finreg_step_bounds_callback as *const u8, 0); + let reg = crate::weakref::js_finreg_new(f64::from_bits(ptr_bits(cb as usize))); + js_shadow_slot_set(0, ptr_bits(reg as usize)); + for _ in 0..records { + add_one_registration(); + } +} + +/// Register one more record against the slot-0 registry. Used both to build the +/// fixture and, mid-cycle, as the *mutator* restructuring the entries array +/// under a live record cursor. +fn add_one_registration() { + let target = crate::object::js_object_alloc(0, 0); + let reg_v = f64::from_bits(js_shadow_slot_get(0)); + let _ = crate::weakref::js_finreg_register( + reg_v, + f64::from_bits(ptr_bits(target as usize)), + f64::from_bits(crate::value::TAG_TRUE), + f64::from_bits(crate::value::TAG_UNDEFINED), + ); +} + +/// Start a budgeted cycle and return once it is active. +fn start_budgeted_cycle(result: &mut JsGcStepResult) { + GC_OLD_RECLAIM_PENDING.with(|pending| pending.set(true)); + assert_eq!( + js_gc_step_work_units(1, result), + JS_GC_STEP_STATUS_ACTIVE, + "old-gen pressure must start a budgeted cycle" + ); +} + +/// Drive the active cycle one work unit at a time, calling `between` after each +/// step. Returns the number of steps taken. +fn drive_one_unit_at_a_time(mut between: impl FnMut(usize)) -> usize { + let mut result = JsGcStepResult::default(); + let mut steps = 0usize; + loop { + let status = js_gc_step_work_units(1, &mut result); + if status != JS_GC_STEP_STATUS_ACTIVE { + return steps; + } + steps += 1; + assert!( + steps < 200_000, + "budgeted cycle did not complete in a sane number of one-unit steps" + ); + between(steps); + } +} + +/// The bound itself: one registry's record array must be spread across steps, +/// with no single step charging more records than its budget. +/// +/// Before #7903 the whole array was scanned inside the one work unit that +/// resolved the holder, so `weak_max_records_per_step` would equal `RECORDS` +/// no matter how small the budget was. +#[test] +fn one_registry_record_array_is_sliced_across_budgeted_steps() { + const RECORDS: usize = 64; + let _guard = CopyingNurseryTestGuard::new(2); + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + reset_old_reclaim_pressure(); + crate::weakref::test_support::clear_weak_holders(); + instruments::reset_step_bound_counters(); + + rooted_registry_with_records(RECORDS); + + let mut result = JsGcStepResult::default(); + start_budgeted_cycle(&mut result); + drive_one_unit_at_a_time(|_| {}); + + // LIVENESS FIRST: without this the two bounds below are satisfied by a run + // that never reached weak processing at all. + assert!( + instruments::weak_steps_sliced() > 0, + "no step ended mid-registry — the sliced path did not run, so the \ + bounds asserted below are vacuous" + ); + assert!( + instruments::weak_records_scanned() >= RECORDS as u64, + "every registered record must be scanned: scanned={} expected>={RECORDS}", + instruments::weak_records_scanned() + ); + assert!( + instruments::weak_max_records_per_step() <= 1, + "a one-work-unit step must charge at most one record; charged {}", + instruments::weak_max_records_per_step() + ); +} + +/// A bigger budget must scale the slice, not remove it: the per-step ceiling +/// tracks the budget rather than the registry size. +#[test] +fn record_slice_size_tracks_the_work_budget() { + const RECORDS: usize = 128; + const BUDGET: u64 = 8; + let _guard = CopyingNurseryTestGuard::new(2); + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + reset_old_reclaim_pressure(); + crate::weakref::test_support::clear_weak_holders(); + instruments::reset_step_bound_counters(); + + rooted_registry_with_records(RECORDS); + + let mut result = JsGcStepResult::default(); + GC_OLD_RECLAIM_PENDING.with(|pending| pending.set(true)); + assert_eq!( + js_gc_step_work_units(BUDGET, &mut result), + JS_GC_STEP_STATUS_ACTIVE + ); + let mut steps = 0usize; + while js_gc_step_work_units(BUDGET, &mut result) == JS_GC_STEP_STATUS_ACTIVE { + steps += 1; + assert!(steps < 200_000, "budgeted cycle did not complete"); + } + + assert!( + instruments::weak_steps_sliced() > 0, + "the registry must still be sliced at budget {BUDGET}, not swallowed whole" + ); + assert!( + instruments::weak_max_records_per_step() <= BUDGET, + "a {BUDGET}-unit step charged {} records", + instruments::weak_max_records_per_step() + ); + assert!( + instruments::weak_max_records_per_step() < RECORDS as u64, + "the whole {RECORDS}-record array went into one step — not sliced at all" + ); +} + +/// The correctness half. The mutator restructures the entries array between two +/// slices; the cursor must notice and restart rather than resume against +/// indices that now denote different records. +/// +/// `js_finreg_register` changes the array's length (and usually its identity +/// word), which is exactly the signal the cursor validates against. +#[test] +fn mutating_the_entries_array_between_slices_restarts_the_cursor() { + const RECORDS: usize = 32; + let _guard = CopyingNurseryTestGuard::new(2); + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + reset_old_reclaim_pressure(); + crate::weakref::test_support::clear_weak_holders(); + instruments::reset_step_bound_counters(); + + rooted_registry_with_records(RECORDS); + + let mut result = JsGcStepResult::default(); + start_budgeted_cycle(&mut result); + // Mutate once, as soon as the first record slice has been charged. + let mut mutated = false; + drive_one_unit_at_a_time(|_| { + if !mutated && instruments::weak_records_scanned() > 0 { + mutated = true; + add_one_registration(); + } + }); + + assert!( + mutated, + "the fixture never reached a record slice to mutate" + ); + assert!( + instruments::weak_registry_restarts() > 0, + "restructuring the entries array under a live cursor was not detected — \ + a resumed cursor would silently skip records, leaving weak slots \ + un-tombstoned" + ); +} + +/// The hard bound on the restart loop. A mutator that restructures the array in +/// every window would restart the scan forever; past `MAX_REGISTRY_RESTARTS` +/// the registry is finished in one atomic pass and *charged as such*. +#[test] +fn relentless_mutation_falls_back_to_a_bounded_atomic_finish() { + const RECORDS: usize = 32; + let _guard = CopyingNurseryTestGuard::new(2); + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + reset_old_reclaim_pressure(); + crate::weakref::test_support::clear_weak_holders(); + instruments::reset_step_bound_counters(); + + rooted_registry_with_records(RECORDS); + + let mut result = JsGcStepResult::default(); + start_budgeted_cycle(&mut result); + // Restructure after EVERY step for as long as weak processing is running. + drive_one_unit_at_a_time(|_| { + if instruments::weak_records_scanned() > 0 + && instruments::weak_registry_atomic_finishes() == 0 + { + add_one_registration(); + } + }); + + assert!( + instruments::weak_registry_restarts() > 0, + "the adversary never managed to invalidate a cursor" + ); + assert!( + instruments::weak_registry_atomic_finishes() > 0, + "a registry mutated in every window restarted forever instead of \ + falling back to the bounded atomic finish" + ); +} + +/// Final root remark is intentionally atomic (see +/// `docs/src/internals/gc-step-bounds.md`). The obligation this test enforces +/// is that it is *measured* rather than claimed: an atomic phase that nobody +/// times is indistinguishable from one that does not exist. +#[test] +fn final_root_remark_is_accounted_as_a_separate_atomic_phase() { + let _guard = CopyingNurseryTestGuard::new(2); + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + reset_old_reclaim_pressure(); + instruments::reset_step_bound_counters(); + + let mut result = JsGcStepResult::default(); + start_budgeted_cycle(&mut result); + drive_one_unit_at_a_time(|_| {}); + + assert!( + instruments::final_remark_count() > 0, + "a completed budgeted cycle must record at least one atomic final \ + remark; zero means the phase is unmeasured again" + ); +} diff --git a/crates/perry-runtime/src/weakref.rs b/crates/perry-runtime/src/weakref.rs index 7d1d20c2e9..bc2f5bed8d 100644 --- a/crates/perry-runtime/src/weakref.rs +++ b/crates/perry-runtime/src/weakref.rs @@ -21,6 +21,7 @@ use crate::value::{ }; use std::cell::RefCell; +pub(crate) mod sliced; #[cfg(test)] pub(crate) mod test_support; @@ -871,71 +872,10 @@ unsafe fn resolve_weak_holder_copied( } } -/// Resumable full/fallback weak processing. The holder registry is snapshotted -/// once, then each call consumes at most `budget` holders. This makes the work -/// O(registered weak holders), rather than O(all arena objects), and lets a -/// budgeted GC return to the mutator between holders. -/// -/// Snapshotting is intentional: budgeted cycles are non-moving, while -/// synchronous moving cycles pass an unlimited budget and cannot expose a -/// mutator window. Holders allocated after the snapshot are allocate-black and -/// therefore cannot lose a target in the current cycle; the next collection -/// processes them. -pub(crate) struct FullWeakProcessingState { - holders: Vec, - cursor: usize, -} - -impl FullWeakProcessingState { - pub(crate) fn new() -> Self { - let holders = WEAK_HOLDERS.with(|holders| holders.borrow().iter().copied().collect()); - #[cfg(test)] - test_support::reset_full_weak_processing_work_units(); - Self { holders, cursor: 0 } - } - - /// Process up to `budget` registered holders. A FinalizationRegistry is - /// one holder/work unit; its record array stays atomic so unregistering - /// cannot interleave with and reorder an in-progress registry scan. - pub(crate) fn step( - &mut self, - valid_ptrs: &crate::gc::ValidPointerSet, - minor_only: bool, - enqueue_callbacks: bool, - budget: usize, - ) -> bool { - if budget == 0 { - return self.cursor == self.holders.len(); - } - let stop = self.holders.len().min(self.cursor.saturating_add(budget)); - let liveness = FullCycleLiveness { - valid_ptrs, - minor_only, - }; - while self.cursor < stop { - let addr = self.holders[self.cursor]; - self.cursor += 1; - #[cfg(test)] - test_support::note_full_weak_processing_work_unit(); - match unsafe { resolve_weak_holder_full(valid_ptrs, addr, minor_only) } { - HolderDisposition::Drop => { - WEAK_HOLDERS.with(|holders| { - holders.borrow_mut().remove(&addr); - }); - } - HolderDisposition::Keep => {} - HolderDisposition::Process(current) => unsafe { - dispatch_weak_holder( - current as *mut ObjectHeader, - &liveness, - enqueue_callbacks, - ); - }, - } - } - self.cursor == self.holders.len() - } -} +/// Resumable full/fallback weak processing. Lives in [`sliced`], which carries +/// the whole rationale for why a FinalizationRegistry's record array is now +/// cursored rather than atomic (#7903). +pub(crate) use sliced::FullWeakProcessingState; /// Validate a registry entry before dereferencing it. Full cycles can prove /// every unmarked holder dead. Fallback minors may only prove that for nursery @@ -1119,14 +1059,64 @@ unsafe fn process_finreg_after_mark( liveness: &dyn WeakLiveness, enqueue_callbacks: bool, ) { + let Some(identity) = finreg_entries_identity(registry, liveness) else { + return; + }; + process_finreg_record_range(registry, liveness, enqueue_callbacks, 0, identity.len); +} + +/// The value word of a registry's `entries` field plus that array's length. +/// +/// This pair is the *identity* a sliced record cursor is validated against — +/// see `sliced`'s module docs. Both mutator-side mutation paths change one of +/// the two (`unregister` installs a rebuilt array, `register` pushes), so a +/// match means the indices held across a mutator window still denote the same +/// records. +#[derive(Clone, Copy, PartialEq, Eq)] +struct FinregEntriesIdentity { + bits: u64, + len: usize, +} + +/// # Safety +/// `registry` must be a live `CLASS_ID_FINALIZATION_REGISTRY` object. +unsafe fn finreg_entries_identity( + registry: *mut ObjectHeader, + liveness: &dyn WeakLiveness, +) -> Option { + let bits = object_field_bits(registry, FINREG_ENTRIES_FIELD); + let entries = liveness.as_live_array(bits)?; + Some(FinregEntriesIdentity { + bits, + len: js_array_length(entries) as usize, + }) +} + +/// Scan `count` records starting at `start`. Returns how many indices were +/// visited — the work-unit charge, which counts skipped (dead / non-record) +/// slots too, because reading and rejecting one is the same cost as processing +/// it and a budget that only charged for hits would not bound anything. +/// +/// # Safety +/// `registry` must be a live `CLASS_ID_FINALIZATION_REGISTRY` object. +unsafe fn process_finreg_record_range( + registry: *mut ObjectHeader, + liveness: &dyn WeakLiveness, + enqueue_callbacks: bool, + start: usize, + count: usize, +) -> usize { let callback = f64::from_bits(object_field_bits(registry, FINREG_CALLBACK_FIELD)); let entries_bits = object_field_bits(registry, FINREG_ENTRIES_FIELD); let Some(entries) = liveness.as_live_array(entries_bits) else { - return; + return 0; }; let len = js_array_length(entries) as usize; + let stop = len.min(start.saturating_add(count)); let registry_value = f64::from_bits(JSValue::pointer(registry as *const u8).bits()); - for i in 0..len { + let mut scanned = 0usize; + for i in start..stop { + scanned += 1; let record_value = js_array_get_f64(entries, i as u32); let Some(record) = liveness .as_live_object_with_class(record_value.to_bits(), CLASS_ID_FINALIZATION_RECORD) @@ -1141,6 +1131,7 @@ unsafe fn process_finreg_after_mark( enqueue_callbacks, ); } + scanned } unsafe fn process_finreg_record_after_mark( diff --git a/crates/perry-runtime/src/weakref/sliced.rs b/crates/perry-runtime/src/weakref/sliced.rs new file mode 100644 index 0000000000..b5cfe769c3 --- /dev/null +++ b/crates/perry-runtime/src/weakref/sliced.rs @@ -0,0 +1,286 @@ +//! Resumable ("sliced") weak processing for budgeted GC cycles. +//! +//! # Why this module exists (#7903) +//! +//! `js_gc_step_us` and the mutator-assist paths advertise a *time* budget, but +//! they can only check elapsed time **between** work units. Any work unit whose +//! cost is unbounded therefore makes the advertised budget a fiction: the step +//! overshoots by however long that one unit ran, and no amount of tightening the +//! microsecond budget can help. +//! +//! Weak processing used to charge **one work unit per registered holder**. A +//! `FinalizationRegistry` is one holder — but its record array is arbitrarily +//! long, and [`super::process_finreg_after_mark`] walked all of it inside that +//! single unit. One registry holding a million registrations was therefore one +//! atomic, heap-sized "work unit" behind a time-budgeted API. +//! +//! This module charges **one work unit per record** and keeps a cursor *into* +//! the record array, so a large registry is spread across as many steps as it +//! needs and every step honours its budget. +//! +//! # The correctness constraint this has to preserve +//! +//! The previous code's atomicity was not accidental. Its comment read: +//! +//! > A FinalizationRegistry is one holder/work unit; its record array stays +//! > atomic so unregistering cannot interleave with and reorder an in-progress +//! > registry scan. +//! +//! That hazard is real. Between two steps the mutator runs, and +//! `FinalizationRegistry.prototype.unregister` **rebuilds** the entries array +//! without the matching records — every index after a removed element shifts +//! down. A naive resumed cursor would skip exactly as many records as were +//! removed before it, and a skipped record is a weak slot that never gets +//! tombstoned: on a non-moving budgeted cycle its target is swept and the slot +//! is left dangling. +//! +//! So the cursor is validated, not trusted. Alongside the record index we keep +//! the **identity** of the array it indexes: the value word of the registry's +//! `entries` field plus that array's length. Both mutation paths change one of +//! them — `unregister` installs a freshly built array (new value word), +//! `register` pushes (new length, and usually a new word too). On resume the +//! identity is re-read and compared; a mismatch means the indices we hold are +//! meaningless, and the registry's scan restarts from 0 against the new array. +//! +//! Restarting is safe because a rescan is idempotent. The first pass writes +//! `undefined` into a collected record's target slot and `false` into its +//! pending flag after enqueueing, so a second pass over the same record sees a +//! target that is no longer a collectable pointer and a pending flag that is no +//! longer set — it enqueues nothing and clears nothing twice. +//! +//! # The hard bound +//! +//! Restart-on-mutation alone is livelock-shaped: a mutator that touches the +//! registry in every window would restart the scan forever. So restarts are +//! capped at [`MAX_REGISTRY_RESTARTS`]; past that the registry is finished in +//! one atomic pass and *charged as such* in the telemetry +//! (`registry_atomic_finishes`). The bound this module offers is therefore +//! explicit rather than implied: per-step weak work is at most +//! `budget + (the one atomic finish that a pathological mutator can force, +//! at most once per registry per cycle)`. + +use super::{ + dispatch_weak_holder, resolve_weak_holder_full, FullCycleLiveness, HolderDisposition, + ObjectHeader, CLASS_ID_FINALIZATION_REGISTRY, WEAK_HOLDERS, +}; + +/// How many times one registry's scan may restart because the mutator changed +/// its entries array under us before we stop slicing it and finish atomically. +/// +/// Four is not tuned — it is small enough that the atomic fallback is reachable +/// in a test and large enough that ordinary `unregister` traffic never reaches +/// it. What matters is that the number is finite, so the phase has a stated +/// worst case instead of an unbounded retry loop. +const MAX_REGISTRY_RESTARTS: u32 = 4; + +/// A cursor into one FinalizationRegistry's record array. +/// +/// The identity it carries ([`super::FinregEntriesIdentity`]) is two words, both +/// re-readable from the registry object without dereferencing anything held +/// across a mutator window. That matters: the cursor survives a return to the +/// mutator, so it must not cache a raw `*mut ArrayHeader` — it caches the +/// *value word* and re-validates it through `WeakLiveness::as_live_array` on +/// every resume, exactly as the unsliced code did on every call. +struct RegistryCursor { + /// The holder's current address (post-`resolve_weak_holder_full`). + holder: usize, + identity: super::FinregEntriesIdentity, + /// Next record index to scan. + next: usize, + restarts: u32, +} + +/// Resumable full/fallback weak processing. The holder registry is snapshotted +/// once, then each call consumes at most `budget` work units — where a unit is +/// one holder resolved **or one FinalizationRegistry record scanned**. This +/// makes the work O(registered weak holders + registered records) with a +/// per-step ceiling, rather than O(all arena objects) with a per-holder ceiling +/// that one large registry could blow through. +/// +/// Snapshotting is intentional: budgeted cycles are non-moving, while +/// synchronous moving cycles pass an unlimited budget and cannot expose a +/// mutator window. Holders allocated after the snapshot are allocate-black and +/// therefore cannot lose a target in the current cycle; the next collection +/// processes them. +pub(crate) struct FullWeakProcessingState { + holders: Vec, + cursor: usize, + /// Set when a step ran out of budget partway through a registry's records. + registry: Option, +} + +impl FullWeakProcessingState { + pub(crate) fn new() -> Self { + let holders = WEAK_HOLDERS.with(|holders| holders.borrow().iter().copied().collect()); + #[cfg(test)] + super::test_support::reset_full_weak_processing_work_units(); + Self { + holders, + cursor: 0, + registry: None, + } + } + + fn holders_drained(&self) -> bool { + self.cursor == self.holders.len() && self.registry.is_none() + } + + /// Process up to `budget` work units. Returns true when this cycle's weak + /// processing is complete. + pub(crate) fn step( + &mut self, + valid_ptrs: &crate::gc::ValidPointerSet, + minor_only: bool, + enqueue_callbacks: bool, + budget: usize, + ) -> bool { + if budget == 0 { + return self.holders_drained(); + } + let liveness = FullCycleLiveness { + valid_ptrs, + minor_only, + }; + let mut remaining = budget; + let mut records_this_step = 0usize; + + // An in-flight registry always gets the budget first: leaving it parked + // while new holders are resolved would let the number of open cursors + // grow, and only one can be represented. + if let Some(mut cursor) = self.registry.take() { + let finished = advance_registry( + &mut cursor, + &liveness, + enqueue_callbacks, + &mut remaining, + &mut records_this_step, + ); + if !finished { + self.registry = Some(cursor); + note_step_records(records_this_step, true); + return false; + } + } + + while remaining > 0 && self.cursor < self.holders.len() { + let addr = self.holders[self.cursor]; + self.cursor += 1; + remaining -= 1; + #[cfg(test)] + super::test_support::note_full_weak_processing_work_unit(); + match unsafe { resolve_weak_holder_full(valid_ptrs, addr, minor_only) } { + HolderDisposition::Drop => { + WEAK_HOLDERS.with(|holders| { + holders.borrow_mut().remove(&addr); + }); + } + HolderDisposition::Keep => {} + HolderDisposition::Process(current) => { + let obj = current as *mut ObjectHeader; + if unsafe { (*obj).class_id } == CLASS_ID_FINALIZATION_REGISTRY { + let Some(identity) = + (unsafe { super::finreg_entries_identity(obj, &liveness) }) + else { + continue; + }; + let mut cursor = RegistryCursor { + holder: current, + identity, + next: 0, + restarts: 0, + }; + let finished = advance_registry( + &mut cursor, + &liveness, + enqueue_callbacks, + &mut remaining, + &mut records_this_step, + ); + if !finished { + self.registry = Some(cursor); + note_step_records(records_this_step, true); + return false; + } + } else { + unsafe { dispatch_weak_holder(obj, &liveness, enqueue_callbacks) }; + } + } + } + } + note_step_records(records_this_step, false); + self.holders_drained() + } +} + +/// Scan records from `cursor` until the registry is exhausted or `remaining` +/// hits zero. Returns true when the registry is fully scanned. +fn advance_registry( + cursor: &mut RegistryCursor, + liveness: &FullCycleLiveness<'_>, + enqueue_callbacks: bool, + remaining: &mut usize, + records_this_step: &mut usize, +) -> bool { + let registry = cursor.holder as *mut ObjectHeader; + loop { + // Re-derive the entries array on every resume. A mismatch means the + // mutator restructured the array between steps and our index is stale. + let Some(identity) = (unsafe { super::finreg_entries_identity(registry, liveness) }) else { + // The array died or stopped being an array: nothing left to scan. + return true; + }; + if identity != cursor.identity { + crate::gc::instruments::note_weak_registry_restart(); + if cursor.restarts >= MAX_REGISTRY_RESTARTS { + // Hard bound: stop slicing this registry and finish it in one + // atomic pass, accounted as such. + crate::gc::instruments::note_weak_registry_atomic_finish(); + let scanned = unsafe { + super::process_finreg_record_range( + registry, + liveness, + enqueue_callbacks, + 0, + identity.len, + ) + }; + *records_this_step = records_this_step.saturating_add(scanned); + *remaining = remaining.saturating_sub(scanned); + return true; + } + cursor.restarts += 1; + cursor.identity = identity; + cursor.next = 0; + } + if cursor.next >= cursor.identity.len { + return true; + } + if *remaining == 0 { + return false; + } + let take = (cursor.identity.len - cursor.next).min(*remaining); + let scanned = unsafe { + super::process_finreg_record_range( + registry, + liveness, + enqueue_callbacks, + cursor.next, + take, + ) + }; + cursor.next += take; + *records_this_step = records_this_step.saturating_add(scanned); + *remaining -= take; + if cursor.next >= cursor.identity.len { + return true; + } + if *remaining == 0 { + return false; + } + } +} + +#[inline] +fn note_step_records(records: usize, sliced: bool) { + crate::gc::instruments::note_weak_step_records(records as u64, sliced); +} diff --git a/docs/src/SUMMARY.md b/docs/src/SUMMARY.md index a6afec0807..4fefff30b5 100644 --- a/docs/src/SUMMARY.md +++ b/docs/src/SUMMARY.md @@ -180,6 +180,7 @@ - [Garbage Collector](internals/garbage-collector.md) - [Explicit Memory Control](internals/explicit-memory.md) - [The GC rooting invariant (codegen)](internals/gc-rooting-invariant.md) +- [Incremental GC step bounds](internals/gc-step-bounds.md) - [RFC: rooting by construction](internals/rfc-rooting-by-construction.md) # Contributing diff --git a/docs/src/internals/gc-step-bounds.md b/docs/src/internals/gc-step-bounds.md new file mode 100644 index 0000000000..895a9af763 --- /dev/null +++ b/docs/src/internals/gc-step-bounds.md @@ -0,0 +1,108 @@ +# What the incremental collector's step budget actually bounds + +`js_gc_step_us(budget_us)` and the mutator-assist paths advertise a *time* +budget. They implement it like this: + +```rust +let mut result = gc_budgeted_step_work_units_inner(1); +while result.status == ACTIVE && start.elapsed().as_micros() < budget_us { + result = gc_budgeted_step_work_units_inner(1); +} +``` + +The clock is consulted **between** work units and never during one. So the +advertised budget is only as strong as the most expensive single unit, and any +unit whose cost scales with the heap makes the budget a statement of intent +rather than a bound. This page records which phases are bounded, which are +deliberately not, and how to see the difference in a real run. + +## The three regimes + +| phase | per-unit cost | bounded by the step budget? | +|---|---|---| +| marking / trace drain | one object per unit | **yes** | +| weak processing | one holder **or one FinalizationRegistry record** per unit | **yes**, since #7903 | +| final root remark | root-set scan **plus** the transitive drain of everything it newly reaches | **no — deliberately atomic** | + +### Weak processing was unbounded until #7903 + +A `FinalizationRegistry` is one registered weak *holder*, and weak processing +used to charge one work unit per holder while +`process_finreg_after_mark` walked that holder's entire record array inside the +unit. One registry with a million registrations was therefore one atomic, +heap-sized "work unit" sitting behind a time-budgeted API. + +`crates/perry-runtime/src/weakref/sliced.rs` now keeps a cursor *into* the +record array and charges one unit per record. The module docs carry the full +argument; the part worth repeating here is why the cursor cannot simply be an +index. + +Between two steps the mutator runs. +`FinalizationRegistry.prototype.unregister` **rebuilds** the entries array +without the matching records, so every index after a removed element shifts +down. A resumed index-only cursor would skip exactly as many records as were +removed before it — and a skipped record is a weak slot that never gets +tombstoned. On a non-moving budgeted cycle its target is then swept and the slot +is left dangling. That hazard is precisely why the array was atomic in the first +place; the old code said so in a comment. + +So the cursor carries the **identity** of the array it indexes: the value word +of the registry's `entries` field plus that array's length. Both mutation paths +change one of the two (`unregister` installs a freshly built array; `register` +pushes). On resume the identity is re-read and compared, and a mismatch restarts +that registry's scan from index 0 against the new array. Restarting is safe +because a rescan is idempotent — the first pass writes `undefined` into a +collected record's target and `false` into its pending flag after enqueueing, so +a second pass over the same record enqueues nothing and clears nothing twice. + +Restart-on-mutation alone is livelock-shaped, so restarts are capped +(`MAX_REGISTRY_RESTARTS`); past the cap the registry is finished in one atomic +pass and **charged as such** in `weak_registry_atomic_finishes`. The bound is +therefore explicit: per-step weak work is at most the requested budget, plus at +most one forced atomic registry pass per registry per cycle. + +### Final root remark is atomic on purpose + +`AtomicFinalizeSubphase::FinalRootRemark` re-scans every root with the marks +nearly final, then drains everything that re-scan newly discovered, both with +`usize::MAX`. The root scan really is bounded by root-set size. **The drain is +not** — a root installed after the initial scan can anchor an arbitrarily large +graph, and the code's older inline claim that the phase was "bounded by +root-set size, not heap size" did not cover it. + +Two ways to make it bounded were considered and rejected: + +- **Yield mid-drain.** Returning to the mutator between the remark scan and weak + processing invalidates the remark itself: the mutator can install new roots, + so the scan would have to be repeated, and repeating it to a fixpoint is not + guaranteed to terminate under an adversarial mutator. +- **Yield after the liveness decision.** This is the correctness race tracked in + #7900. Weak processing must observe a *complete* mark set; handing control + back once liveness has been decided but before the weak slots are tombstoned + lets the mutator observe a target that the collector has already condemned. + +So the phase stays atomic, and the project's obligation is to **measure it +rather than claim it**. `final_remark_max_us` is that measurement. + +## Seeing it in a run + +`PERRY_GC_DIAG=1` (no `PERRY_GC_TRACE` needed, and no new env knob) prints: + +``` +[gc-step-bounds] step_max_us= final_remark_max_us= final_remarks= \ + weak_records= weak_max_records_per_step= weak_steps_sliced= \ + weak_registry_restarts= weak_registry_atomic_finishes= +``` + +- `step_max_us` is the honest answer to "how long is a step". Compare it against + `GC_NORMAL_INCREMENTAL_SOFT_PAUSE_US` (2 000) and + `GC_MUTATOR_ASSIST_SOFT_PAUSE_US` (500). +- `final_remark_max_us` is reported **separately and on purpose**. Folding an + intentionally-atomic phase into the general maximum would let a heap-sized + pause hide behind "the collector's worst step". +- `weak_steps_sliced` is the **subject-was-live counter**. A run reporting `0` + has not exercised the sliced path at all, however green everything else looks. + Do not read a zero here as "slicing works"; read it as "slicing did not + happen". Most programs register no weak holders and will legitimately report + zeros across this whole line — which is exactly why the acceptance tests drive + the counters directly rather than inferring them from a corpus run.