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.