From 1d750248eba0c2c58a3cae58de24937c54d64a1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Mon, 13 Jul 2026 07:29:40 +0200 Subject: [PATCH 1/3] fix(gc): prune DATA_VIEW_REGISTRY and SHARED_ARRAY_BUFFER_REGISTRY on buffer death (#6337) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `finalize_collected_dead_buffer` exists to drop "every registry/side-table entry keyed by a dead buffer's address" — its doc comment cites preventing the #6080 ABA class — but it only pruned BUFFER_REGISTRY, ARRAY_BUFFER_REGISTRY and BUFFER_AB_ALIAS. `DATA_VIEW_REGISTRY` and `SHARED_ARRAY_BUFFER_REGISTRY` had zero `.remove`/`.retain` sites anywhere in the tree. Two consequences, the same pair proven for the WebCrypto tables in #6302/#6310: * unbounded leak — one permanent entry per DataView / SAB-flagged buffer ever created, for the life of the process; * ABA / address reuse — `arena_reset_empty_blocks` resets a fully-empty block's offset to 0 while keeping its base pointer, so a reset block re-issues the same addresses and a stale entry answers for an unrelated object. `is_data_view`/`is_shared_array_buffer` gate `util.types.isDataView` / `isSharedArrayBuffer`, `ArrayBuffer.isView`, the `[object DataView]` tag, and the structuredClone / `.slice()` re-marking paths. Both are now pruned in the post-full-trace sweep, alongside the existing three. SharedArrayBuffer needed a second, separate fix. A process-global SAB backing (`shared_sab::alloc_shared_sab`) is NOT a GC allocation: it comes straight from `alloc_zeroed`, carries no GcHeader, and is never freed — that is what lets the bytes alias across `perry/thread` agents (#4913). But `js_shared_array_buffer_new` does `register_buffer` it, so it reaches `registered_buffer_is_dead_post_trace` on every full trace, where `try_read_gc_header` reads the 8 bytes BEFORE the malloc block and interprets the allocator's own metadata as a GcHeader: one arbitrary byte compared against GC_TYPE_BUFFER (10), the next against the mark/pin/forward bits. A chance match declares a LIVE, never-freed SAB dead — and `finalize_collected_dead_buffer` then runs `view::remove_entries_for_dead_buffer`, which retains on `info.backing != addr` and so unregisters EVERY live typed-array view over that SAB: exactly the views cross-agent `Atomics` wait/notify resolve their absolute slot addresses through. So the backing is now vetoed as a dead candidate before any header is sniffed, rather than pruned. The entries that DO get pruned from SHARED_ARRAY_BUFFER_REGISTRY are the arena-allocated SAB-flagged copies (`SharedArrayBuffer.prototype.slice`, structuredClone), which are ordinary GC-heap buffers that genuinely die and whose addresses genuinely get recycled. Cross-thread aliasing, the process-global registry and the Atomics futex table are untouched. The veto sits behind a `SHARED_SAB_NONEMPTY` latch mirroring `EXTERNAL_BUFFERS_NONEMPTY`, so processes that never allocate a SAB — nearly all of them — answer from one relaxed atomic load and never take the registry lock. That also removes an unconditional mutex acquisition from `is_registered_buffer`'s fallback, which JSON.stringify runs per serialized pointer (#6009). Tests (`gc/tests/buffer_side_tables.rs`): dead DataView / SAB-flagged buffer pruned on full GC; live (rooted) ones survive; a 4096-entry leak regression that asserts both registries DRAIN to baseline after their owners are swept; and two SAB-liveness tests, one seeding the shared-SAB registry to prove the veto deterministically (the malloc-metadata coincidence cannot be forced without writing outside the allocation), one end-to-end over a real `new SharedArrayBuffer(64)`. --- crates/perry-runtime/src/buffer/header.rs | 65 +++++ crates/perry-runtime/src/buffer/mod.rs | 2 + .../src/gc/tests/buffer_side_tables.rs | 231 ++++++++++++++++++ crates/perry-runtime/src/gc/tests/mod.rs | 1 + crates/perry-runtime/src/shared_sab.rs | 50 +++- 5 files changed, 348 insertions(+), 1 deletion(-) create mode 100644 crates/perry-runtime/src/gc/tests/buffer_side_tables.rs diff --git a/crates/perry-runtime/src/buffer/header.rs b/crates/perry-runtime/src/buffer/header.rs index 20f620e4ac..407c547cc5 100644 --- a/crates/perry-runtime/src/buffer/header.rs +++ b/crates/perry-runtime/src/buffer/header.rs @@ -161,6 +161,19 @@ pub fn is_data_view(addr: usize) -> bool { DATA_VIEW_REGISTRY.with(|r| r.borrow().contains(&addr)) } +/// Live entry counts for the two registries the GC buffer sweep prunes (#6337). +/// Test-only: the leak regression asserts these DRAIN after the owning buffers +/// are collected, which a per-address `is_*` probe cannot show. +#[cfg(test)] +pub(crate) fn test_data_view_registry_len() -> usize { + DATA_VIEW_REGISTRY.with(|r| r.borrow().len()) +} + +#[cfg(test)] +pub(crate) fn test_shared_array_buffer_registry_len() -> usize { + SHARED_ARRAY_BUFFER_REGISTRY.with(|r| r.borrow().len()) +} + /// Register a buffer pointer in the thread-local registry pub fn register_buffer(ptr: *const BufferHeader) { BUFFER_REGISTRY.with(|r| r.borrow_mut().insert(ptr as usize)); @@ -465,6 +478,30 @@ pub(crate) fn collect_dead_registered_buffers_post_trace(full_trace: bool) -> Ve } unsafe fn registered_buffer_is_dead_post_trace(addr: usize) -> bool { + // A process-global `SharedArrayBuffer` backing is NOT a GC allocation: + // `shared_sab::alloc_shared_sab` takes it straight from `alloc_zeroed`, it + // carries no `GcHeader`, and it is never freed (#4913 — that is what lets + // the same bytes alias across `perry/thread` agents). But + // `js_shared_array_buffer_new` DOES `register_buffer` it, so it lands in + // `BUFFER_REGISTRY` and reaches this scan on every full trace. + // + // `try_read_gc_header` below would then read the 8 bytes BEFORE the malloc + // block — the allocator's own metadata — and interpret them as a `GcHeader`: + // one arbitrary byte compared against `GC_TYPE_BUFFER` (10), the next + // against the mark/pin/forward bits. A chance match declares a LIVE, + // never-freed SAB dead, and `finalize_collected_dead_buffer` then runs on + // it — including `view::remove_entries_for_dead_buffer`, which retains on + // `info.backing != addr` and so unregisters EVERY live typed-array view + // over that SAB. Those views are exactly how cross-agent `Atomics` + // wait/notify resolve their absolute slot addresses. + // + // So: veto first, and never sniff a header the object does not have. The + // registry is behind a latch (`SHARED_SAB_NONEMPTY`), so the processes that + // never allocate a SAB — nearly all of them — pay one relaxed atomic load + // here and never take the lock. + if crate::shared_sab::is_shared_sab(addr) { + return false; + } let Some(header) = crate::value::addr_class::try_read_gc_header(addr) else { return false; }; @@ -487,6 +524,34 @@ pub(crate) fn finalize_collected_dead_buffer(addr: usize) { ARRAY_BUFFER_REGISTRY.with(|r| { r.borrow_mut().remove(&addr); }); + // #6337: the two sibling buffer-identity registries were missing from this + // list — they had no `.remove`/`.retain` site anywhere in the tree. Like + // the three above they are plain address-keyed sets that never rooted the + // `BufferHeader`, so a collected view left its entry behind forever: + // + // * an unbounded leak — one permanent entry per `DataView` (and per + // SAB-flagged buffer) ever created; + // * the #6080 ABA class this function exists to prevent — + // `arena_reset_empty_blocks` resets a fully-empty block's offset to 0 + // while KEEPING its base pointer, so a reset block re-issues the same + // addresses. A recycled address then inherits the dead view's identity: + // `is_data_view`/`is_shared_array_buffer` gate `util.types.isDataView`/ + // `isSharedArrayBuffer`, `ArrayBuffer.isView`, the `[object DataView]` + // tag, and the structuredClone/`.slice()` re-marking above — an + // unrelated fresh Buffer landing there would answer to all of them. + // + // Only GC-heap buffers reach here. A process-global SAB backing is never + // freed and is vetoed as a dead candidate in + // `registered_buffer_is_dead_post_trace`, so the entries pruned from + // SHARED_ARRAY_BUFFER_REGISTRY are the arena-allocated SAB-flagged copies + // (`SharedArrayBuffer.prototype.slice`, structuredClone) — the ones that + // genuinely die and whose addresses genuinely get recycled. + SHARED_ARRAY_BUFFER_REGISTRY.with(|r| { + r.borrow_mut().remove(&addr); + }); + DATA_VIEW_REGISTRY.with(|r| { + r.borrow_mut().remove(&addr); + }); BUFFER_AB_ALIAS.with(|r| { r.borrow_mut().remove(&addr); }); diff --git a/crates/perry-runtime/src/buffer/mod.rs b/crates/perry-runtime/src/buffer/mod.rs index c02255851b..bd96b5be04 100644 --- a/crates/perry-runtime/src/buffer/mod.rs +++ b/crates/perry-runtime/src/buffer/mod.rs @@ -43,6 +43,8 @@ pub use header::{ pub(crate) use header::{ collect_dead_registered_buffers_post_trace, finalize_collected_dead_buffer, }; +#[cfg(test)] +pub(crate) use header::{test_data_view_registry_len, test_shared_array_buffer_registry_len}; // ---- Re-exports: ArrayBuffer detach / transfer (ES2024) ---- // `detach_array_buffer` dereferences the raw address it is given, so it stays diff --git a/crates/perry-runtime/src/gc/tests/buffer_side_tables.rs b/crates/perry-runtime/src/gc/tests/buffer_side_tables.rs new file mode 100644 index 0000000000..e04670bb20 --- /dev/null +++ b/crates/perry-runtime/src/gc/tests/buffer_side_tables.rs @@ -0,0 +1,231 @@ +//! Death pruning for the buffer-identity side tables (#6337). +//! +//! `finalize_collected_dead_buffer` exists to drop "every registry/side-table +//! entry keyed by a dead buffer's address" — its own doc comment cites +//! preventing the #6080 ABA class. But `DATA_VIEW_REGISTRY` and +//! `SHARED_ARRAY_BUFFER_REGISTRY` were never pruned anywhere in the tree: they +//! had zero `.remove`/`.retain` sites. That leaked one permanent entry per +//! `DataView` / SAB-flagged buffer ever created, and let a recycled address +//! (`arena_reset_empty_blocks` resets a fully-empty block's offset to 0 while +//! KEEPING its base pointer) inherit a dead view's identity. +//! +//! The SharedArrayBuffer side has a wrinkle the DataView side does not: a +//! *process-global* SAB backing (`shared_sab::alloc_shared_sab`) is not a GC +//! allocation at all and is never freed, so it must be vetoed as a dead +//! candidate rather than pruned — see +//! `test_seeded_shared_sab_is_never_a_dead_buffer_candidate`. + +use super::super::*; +use super::support::*; + +fn full_gc() { + let _ = + gc_collect_full_mark_sweep_with_trigger(GcTriggerSnapshot::capture(GcTriggerKind::Direct)); +} + +/// Drops a test-seeded `shared_sab` entry even if the test panics — the +/// registry is process-global and never cleared, so a leaked seed would make +/// every later test in this binary treat that address as a live SAB backing. +struct SeededSharedSabGuard(usize); + +impl Drop for SeededSharedSabGuard { + fn drop(&mut self) { + crate::shared_sab::test_unseed_shared_sab(self.0); + } +} + +/// A dead `DataView`'s `DATA_VIEW_REGISTRY` entry must go. `is_data_view` gates +/// `util.types.isDataView`, `ArrayBuffer.isView`, the `[object DataView]` tag, +/// and the structuredClone / `.slice()` re-marking paths — a fresh Buffer +/// landing on the recycled address would answer to all of them. +#[test] +fn test_dead_data_view_registry_entry_pruned_on_full_gc() { + let _guard = GcTestIsolationGuard::new(); + + let addr = crate::buffer::buffer_alloc(32) as usize; + crate::buffer::mark_as_data_view(addr); + assert!( + crate::buffer::is_data_view(addr), + "test premise: the view is registered" + ); + + // No roots: dead at the full trace. (Buffers are TENURED old-gen residents, + // so only a FULL trace can prove them dead.) + full_gc(); + + assert!( + !crate::buffer::is_data_view(addr), + "dead buffer must not keep its DataView identity — a recycled address \ + would answer to util.types.isDataView / ArrayBuffer.isView" + ); +} + +/// The SAB-flagged buffers that actually die are the arena-allocated copies: +/// `SharedArrayBuffer.prototype.slice` (`object/buffer_dispatch.rs`) and +/// structuredClone (`builtins/globals.rs`) both `buffer_alloc` a fresh +/// `BufferHeader` and re-`mark_as_shared_array_buffer` it. Those are ordinary +/// GC-heap objects whose addresses get recycled. +#[test] +fn test_dead_shared_array_buffer_flag_pruned_on_full_gc() { + let _guard = GcTestIsolationGuard::new(); + + let addr = crate::buffer::buffer_alloc(32) as usize; + crate::buffer::mark_as_shared_array_buffer(addr); + assert!( + crate::buffer::is_shared_array_buffer(addr), + "test premise: the SAB flag is registered" + ); + + full_gc(); + + assert!( + !crate::buffer::is_shared_array_buffer(addr), + "dead SAB-flagged buffer must not keep its identity — a recycled \ + address would answer to util.types.isSharedArrayBuffer" + ); +} + +/// The safety inverse: a LIVE (rooted) view keeps its flag. Full mark-sweep is +/// non-moving, so the rooted buffers keep their addresses. +#[test] +fn test_live_data_view_and_shared_array_buffer_flags_survive_full_gc() { + let _guard = GcTestIsolationGuard::new(); + + let view = crate::buffer::buffer_alloc(32) as usize; + crate::buffer::mark_as_data_view(view); + let sab = crate::buffer::buffer_alloc(32) as usize; + crate::buffer::mark_as_shared_array_buffer(sab); + + js_shadow_slot_set(0, ptr_bits(view)); + js_shadow_slot_set(1, ptr_bits(sab)); + + full_gc(); + + assert!( + crate::buffer::is_data_view(view), + "a live (rooted) DataView must keep its registry entry" + ); + assert!( + crate::buffer::is_shared_array_buffer(sab), + "a live (rooted) SAB-flagged buffer must keep its registry entry" + ); +} + +/// The leak regression: N views, all references dropped, one full collection, +/// and both registries must DRAIN. A per-address `is_*` probe cannot show this +/// — before the fix the tables grew monotonically for the life of the process, +/// one permanent entry per `DataView` / SAB-flagged buffer ever created. +#[test] +fn test_data_view_and_sab_registries_drain_after_owners_die() { + let _guard = GcTestIsolationGuard::new(); + + const N: usize = 4096; + let base_views = crate::buffer::test_data_view_registry_len(); + let base_sabs = crate::buffer::test_shared_array_buffer_registry_len(); + + for _ in 0..N { + let view = crate::buffer::buffer_alloc(16) as usize; + crate::buffer::mark_as_data_view(view); + let sab = crate::buffer::buffer_alloc(16) as usize; + crate::buffer::mark_as_shared_array_buffer(sab); + } + + assert_eq!( + crate::buffer::test_data_view_registry_len(), + base_views + N, + "test premise: every DataView registered" + ); + assert_eq!( + crate::buffer::test_shared_array_buffer_registry_len(), + base_sabs + N, + "test premise: every SAB-flagged buffer registered" + ); + + // Every one of them is unreachable — no shadow slot, no global root. + full_gc(); + + assert_eq!( + crate::buffer::test_data_view_registry_len(), + base_views, + "DATA_VIEW_REGISTRY must drain when its buffers are swept (it had no \ + .remove site at all before #6337)" + ); + assert_eq!( + crate::buffer::test_shared_array_buffer_registry_len(), + base_sabs, + "SHARED_ARRAY_BUFFER_REGISTRY must drain when its buffers are swept" + ); +} + +/// A process-global `SharedArrayBuffer` backing must NEVER be treated as a +/// collectable GC object. +/// +/// `alloc_shared_sab` takes the block straight from `alloc_zeroed`: no +/// `GcHeader`, never freed (that is what lets the bytes alias across +/// `perry/thread` agents, #4913). But `js_shared_array_buffer_new` DOES +/// `register_buffer` it, so it reaches the dead-buffer scan on every full +/// trace, where `try_read_gc_header` would sniff the 8 bytes BEFORE the malloc +/// block and read the allocator's own metadata as a header — one arbitrary byte +/// against `GC_TYPE_BUFFER` (10), the next against the mark/pin/forward bits. +/// +/// A chance match declares a LIVE SAB dead, and `finalize_collected_dead_buffer` +/// then runs `view::remove_entries_for_dead_buffer`, which retains on +/// `info.backing != addr` — silently unregistering EVERY live typed-array view +/// over that SAB, i.e. exactly the views cross-agent `Atomics` wait/notify +/// resolve their absolute slot addresses through. +/// +/// That coincidence cannot be forced from a test without writing outside the +/// allocation, so seed an ordinary GC buffer — whose real header genuinely says +/// "dead" — into the SAB registry. It reproduces the same decision +/// deterministically: without the veto this buffer is reported dead. +#[test] +fn test_seeded_shared_sab_is_never_a_dead_buffer_candidate() { + let _guard = GcTestIsolationGuard::new(); + + let addr = crate::buffer::buffer_alloc(32) as usize; + crate::buffer::mark_as_shared_array_buffer(addr); + crate::shared_sab::test_seed_shared_sab(addr); + let _seeded = SeededSharedSabGuard(addr); + + // Unrooted, and its real GcHeader is unmarked — the scan would list it. + let dead = crate::buffer::collect_dead_registered_buffers_post_trace(true); + + assert!( + !dead.contains(&addr), + "a process-global SAB backing must never be reported dead: it has no \ + GcHeader and is never freed, and finalizing it would unregister every \ + live typed-array view over it (breaking cross-agent Atomics)" + ); +} + +/// End-to-end companion to the veto test: a real `new SharedArrayBuffer(n)` +/// backing survives a full collection with no roots at all, and stays +/// recognisable through every predicate — the thread-local flag, the buffer +/// registry, and the process-global registry the cross-thread serializer and +/// `Atomics` futex keying depend on. +#[test] +fn test_process_global_sab_backing_survives_full_gc_unrooted() { + let _guard = GcTestIsolationGuard::new(); + + let buf = crate::buffer::js_shared_array_buffer_new(64); + let addr = buf as usize; + assert!(crate::shared_sab::is_shared_sab(addr)); + + // Deliberately unrooted. The backing is never freed, so this must be a + // no-op for it — a SAB is reachable from other agents, not from this + // thread's roots. + full_gc(); + + assert!( + crate::shared_sab::is_shared_sab(addr), + "the process-global SAB registry must still recognise the backing" + ); + assert!( + crate::buffer::is_shared_array_buffer(addr), + "the SAB must still answer util.types.isSharedArrayBuffer" + ); + assert!( + crate::buffer::is_registered_buffer(addr), + "the SAB must still answer as a registered buffer" + ); +} diff --git a/crates/perry-runtime/src/gc/tests/mod.rs b/crates/perry-runtime/src/gc/tests/mod.rs index 3941487d19..5190313adf 100644 --- a/crates/perry-runtime/src/gc/tests/mod.rs +++ b/crates/perry-runtime/src/gc/tests/mod.rs @@ -1,6 +1,7 @@ mod alloc; mod barrier; mod budgeted_step_api; +mod buffer_side_tables; mod contract; mod copying; mod copying_side_tables; diff --git a/crates/perry-runtime/src/shared_sab.rs b/crates/perry-runtime/src/shared_sab.rs index 2fa5af8406..438e93eae7 100644 --- a/crates/perry-runtime/src/shared_sab.rs +++ b/crates/perry-runtime/src/shared_sab.rs @@ -22,6 +22,7 @@ use std::alloc::{alloc_zeroed, handle_alloc_error, Layout}; use std::collections::HashSet; +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Mutex, OnceLock}; use crate::buffer::BufferHeader; @@ -29,6 +30,17 @@ use crate::buffer::BufferHeader; /// Set of `BufferHeader` addresses that back a `SharedArrayBuffer`. static SHARED_SAB_REGISTRY: OnceLock>> = OnceLock::new(); +/// Latched true by the first SAB allocation. Mirrors `EXTERNAL_BUFFERS_NONEMPTY` +/// in `buffer::header` and exists for the same reason: `is_shared_sab` sits on +/// two hot paths that run for *every* pointer-shaped value — +/// `buffer::is_registered_buffer`'s final fallback (which JSON.stringify runs +/// per serialized pointer, #6009) and the GC's dead-buffer scan, which probes +/// every registered buffer on every full trace. Without the latch both take the +/// registry mutex on each miss. The overwhelming majority of processes never +/// allocate a `SharedArrayBuffer` at all, so they can answer `false` from a +/// single relaxed atomic load and never touch the lock. +static SHARED_SAB_NONEMPTY: AtomicBool = AtomicBool::new(false); + fn registry() -> &'static Mutex> { SHARED_SAB_REGISTRY.get_or_init(|| Mutex::new(HashSet::new())) } @@ -63,16 +75,52 @@ pub fn alloc_shared_sab(size: u32) -> *mut BufferHeader { .lock() .unwrap_or_else(|e| e.into_inner()) .insert(buf as usize); + // Release-store AFTER the insert is visible, so a thread that observes the + // latch also observes the entry it was latched for. + SHARED_SAB_NONEMPTY.store(true, Ordering::Release); buf } /// True if `addr` is a process-global `SharedArrayBuffer` backing store. Unlike /// the thread-local `buffer::is_shared_array_buffer`, this answers correctly on /// every thread — used by the cross-thread serializer to recognise a SAB -/// pointer that has no `GcHeader`. +/// pointer that has no `GcHeader`, and by the GC's dead-buffer scan to refuse +/// to treat one as a collectable GC allocation (see +/// `buffer::header::registered_buffer_is_dead_post_trace`). pub fn is_shared_sab(addr: usize) -> bool { + if !SHARED_SAB_NONEMPTY.load(Ordering::Acquire) { + return false; + } registry() .lock() .map(|r| r.contains(&addr)) .unwrap_or(false) } + +/// Test-only: pretend `addr` is a process-global SAB backing. +/// +/// A real backing has no `GcHeader`, so the GC's dead-buffer scan can only +/// mistake it for a collectable object when the malloc metadata preceding the +/// block happens to read as a dead `GC_TYPE_BUFFER` header — a coincidence a +/// test cannot force without writing outside the allocation. Seeding an +/// ordinary GC buffer (whose real header genuinely says "dead") into this +/// registry reproduces the same decision deterministically, so the veto in +/// `buffer::header::registered_buffer_is_dead_post_trace` can be proven rather +/// than assumed. Callers MUST pair this with [`test_unseed_shared_sab`] — the +/// registry is process-global and never cleared. +#[cfg(test)] +pub(crate) fn test_seed_shared_sab(addr: usize) { + registry() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .insert(addr); + SHARED_SAB_NONEMPTY.store(true, Ordering::Release); +} + +#[cfg(test)] +pub(crate) fn test_unseed_shared_sab(addr: usize) { + registry() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .remove(&addr); +} From be1a483aadf37d0ac55a3140e27ede79a36d2155 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Mon, 13 Jul 2026 07:40:30 +0200 Subject: [PATCH 2/3] test(gc): root the live-survival buffers in a real shadow frame (#6337) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GcTestIsolationGuard does not push a shadow frame, so js_shadow_slot_set under it roots nothing — the buffers stayed unreachable and the safety inverse would have passed for the wrong reason. Use CopyingNurseryTestGuard, which pushes the frame (same as the dead_owner_side_tables precedent). --- crates/perry-runtime/src/gc/tests/buffer_side_tables.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/perry-runtime/src/gc/tests/buffer_side_tables.rs b/crates/perry-runtime/src/gc/tests/buffer_side_tables.rs index e04670bb20..b5a50b6118 100644 --- a/crates/perry-runtime/src/gc/tests/buffer_side_tables.rs +++ b/crates/perry-runtime/src/gc/tests/buffer_side_tables.rs @@ -87,9 +87,14 @@ fn test_dead_shared_array_buffer_flag_pruned_on_full_gc() { /// The safety inverse: a LIVE (rooted) view keeps its flag. Full mark-sweep is /// non-moving, so the rooted buffers keep their addresses. +/// +/// `CopyingNurseryTestGuard::new(2)` — not `GcTestIsolationGuard` — because +/// only it pushes the shadow frame that makes `js_shadow_slot_set` an actual +/// root. Under the plain isolation guard the slot writes land in no frame, the +/// buffers stay unreachable, and the test would "pass" for the wrong reason. #[test] fn test_live_data_view_and_shared_array_buffer_flags_survive_full_gc() { - let _guard = GcTestIsolationGuard::new(); + let _guard = CopyingNurseryTestGuard::new(2); let view = crate::buffer::buffer_alloc(32) as usize; crate::buffer::mark_as_data_view(view); From 2d69880b7d2481ad7ba62470a2168ce65524fce6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Mon, 13 Jul 2026 07:51:10 +0200 Subject: [PATCH 3/3] perf(gc): snapshot the SAB registry once per dead-buffer scan (#6337) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The veto in registered_buffer_is_dead_post_trace called is_shared_sab per registered buffer, taking the process-global registry mutex once per buffer per full trace in any program that allocates a SharedArrayBuffer. The set is tiny and the GC is stop-the-world here, so lock it once per scan and hand the filter a plain set. Returns None — and skips the check entirely, allocating nothing — for the processes that never allocate a SAB. --- crates/perry-runtime/src/buffer/header.rs | 21 +++++++++++++++------ crates/perry-runtime/src/shared_sab.rs | 15 +++++++++++++++ 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/crates/perry-runtime/src/buffer/header.rs b/crates/perry-runtime/src/buffer/header.rs index 407c547cc5..167f9acd09 100644 --- a/crates/perry-runtime/src/buffer/header.rs +++ b/crates/perry-runtime/src/buffer/header.rs @@ -468,16 +468,25 @@ pub(crate) fn collect_dead_registered_buffers_post_trace(full_trace: bool) -> Ve if !full_trace { return Vec::new(); } + // Lock the process-global SAB registry ONCE for the whole scan rather than + // once per registered buffer (see `registered_buffer_is_dead_post_trace`). + // `None` — nearly every process — means no SAB was ever allocated. + let shared_sabs = crate::shared_sab::snapshot_shared_sabs(); BUFFER_REGISTRY.with(|r| { r.borrow() .iter() .copied() - .filter(|&addr| unsafe { registered_buffer_is_dead_post_trace(addr) }) + .filter(|&addr| unsafe { + registered_buffer_is_dead_post_trace(addr, shared_sabs.as_ref()) + }) .collect() }) } -unsafe fn registered_buffer_is_dead_post_trace(addr: usize) -> bool { +unsafe fn registered_buffer_is_dead_post_trace( + addr: usize, + shared_sabs: Option<&std::collections::HashSet>, +) -> bool { // A process-global `SharedArrayBuffer` backing is NOT a GC allocation: // `shared_sab::alloc_shared_sab` takes it straight from `alloc_zeroed`, it // carries no `GcHeader`, and it is never freed (#4913 — that is what lets @@ -496,10 +505,10 @@ unsafe fn registered_buffer_is_dead_post_trace(addr: usize) -> bool { // wait/notify resolve their absolute slot addresses. // // So: veto first, and never sniff a header the object does not have. The - // registry is behind a latch (`SHARED_SAB_NONEMPTY`), so the processes that - // never allocate a SAB — nearly all of them — pay one relaxed atomic load - // here and never take the lock. - if crate::shared_sab::is_shared_sab(addr) { + // set is snapshotted once per scan by the caller and is `None` for the + // processes that never allocate a SAB — nearly all of them — so the common + // path here is a single null check. + if shared_sabs.is_some_and(|sabs| sabs.contains(&addr)) { return false; } let Some(header) = crate::value::addr_class::try_read_gc_header(addr) else { diff --git a/crates/perry-runtime/src/shared_sab.rs b/crates/perry-runtime/src/shared_sab.rs index 438e93eae7..f918067984 100644 --- a/crates/perry-runtime/src/shared_sab.rs +++ b/crates/perry-runtime/src/shared_sab.rs @@ -97,6 +97,21 @@ pub fn is_shared_sab(addr: usize) -> bool { .unwrap_or(false) } +/// Snapshot the SAB backing set for one GC dead-buffer scan. +/// +/// The scan tests every registered buffer, and calling [`is_shared_sab`] per +/// buffer would take the registry mutex once per buffer per full trace. The set +/// is tiny (a handful of entries even in heavy `Atomics` users) and the GC is +/// stop-the-world here, so lock it once and hand the scan a plain set. `None` — +/// the case for nearly every process — means no SAB was ever allocated and the +/// scan can skip the check entirely without allocating anything. +pub(crate) fn snapshot_shared_sabs() -> Option> { + if !SHARED_SAB_NONEMPTY.load(Ordering::Acquire) { + return None; + } + registry().lock().ok().map(|r| r.clone()) +} + /// Test-only: pretend `addr` is a process-global SAB backing. /// /// A real backing has no `GcHeader`, so the GC's dead-buffer scan can only