diff --git a/changelog.d/registry-probe-address-filter.md b/changelog.d/registry-probe-address-filter.md new file mode 100644 index 0000000000..923ff47854 --- /dev/null +++ b/changelog.d/registry-probe-address-filter.md @@ -0,0 +1,121 @@ +### Performance + +- **The symbol and class-prototype probes answer "no" from an inlined Bloom + test instead of a mutex acquisition and a linear scan; the Uint8Array probe + gets #9272's address window.** + + #9272 put an inline `[lo, hi]` address window in front of `is_registered_buffer` + and `lookup_typed_array_kind`, and named four probes as the next-largest + members of the same family. Measured on a symbolized `claude --help` + (`is_registered_symbol_slow` 0.65%, `is_registered_class_prototype_object` + 0.46%, `is_registered_box_ptr` 0.22%, `is_uint8array_buffer_slow` 0.22%), a + window is the right fix for exactly one of them. + + Exact uprobe/uretprobe counts from one run — not inferred from the profile: + + | probe | calls | answered "yes" | a window rejects | a filter rejects | + |---|---|---|---|---| + | `is_registered_symbol` | 378,163 | 622 | 38.3% | **99.58%** | + | `is_uint8array_buffer` | 537,921 | **0** | **100%** | — | + | `is_registered_class_prototype_object` | 26,290 | 122 | 54.0% | **99.05%** | + | `is_registered_box_ptr` | 211,148 | **205,640 (97.4%)** | — | — | + + The window column is not a guess: it replays each probe's real argument stream + against the window its own registrations would have built, widening it exactly + as `RegistryAddrWindow::admit` does. The filter column does the same against a + bit-for-bit simulation of the filter that ships here. + + **Why a window fails on two of them.** Buffers and typed arrays sit in their + own allocations; symbols and class prototypes are ordinary `gc_malloc`'d heap + objects, interleaved with everything else the program allocates, so `[lo, hi]` + grows to span ~280 MB of heap and stops discriminating. `RegistryAddrFilter` + is the same monotone contract — admit before you publish, bits only ever + set, `false` means "definitively absent" — over a 1,024-bit Bloom filter + instead of a range. Both shapes are in the tree on purpose; pick by + measurement. + + For symbols this replaces #9177's `[lo, hi]` range, which sat *inside* + `is_registered_symbol_slow`: a rejected probe still paid the out-of-line call + and a `OnceLock` load for an env kill switch before reaching the two bound + loads. `PERRY_SYMBOL_RANGE_FILTER` is gone with it — it guarded an + enumeration of the inserters, and the debug-build audit below is strictly + stronger than an env var nobody sets. + + For class prototypes the filter sits in front of a `map.values().any(…)` + **linear scan** (#9225) reached through a thread-local and an `RwLock`, whose + one caller — `descriptor_state::disable_inline_guards_for_descriptor_target`, + 100% of the 26,290 calls — runs on every `Object.defineProperty`, and a bundle's + `__export(exports, { … })` init runs thousands of those. This does **not** + close #9225: a false positive still pays the scan, so the table's O(n) slope + survives at ~1% of its strength, and the O(1) inverse index that issue asks + for is still the right structural fix. + +- **`is_registered_box_ptr` is not a member of this family, and the measurement + is what says so.** It answers **"yes" 205,640 times out of 211,148** (97.4%) + — the opposite regime from every other probe here, because its four callers + (`js_closure_set_box_capture_ptr`, `js_box_get_bits`, `js_box_set_bits`, + `js_box_capture_cell_ptr`, 100% of calls between them) ask it about *actual + box pointers* on the async-locals read/write path. A filter in front of it + could remove at most 2.6% of its 0.22%. It is left alone deliberately; if it + is attacked again, the target is the cost of the **hit** — the direct-mapped + positive cache in `tls_hot` — not a rejection filter. + + The filter is 1,024 bits with three probes per address, sized for the ~160 + entries this corpus registers. A much larger bundle can saturate it — + `CLASS_PROTOTYPE_OBJECTS` grows by one entry per ES5-transpiled constructor — + and that is a *win* cliff, not a correctness one: a saturated filter is + exactly the code that ran before it existed. Raising the constant is a + one-line change; it is deliberately not raised on speculation, because 1,024 + bits is the size every number above was measured at. + + Bits accrue per *admission*, not per live entry: both tables are re-keyed by + the collector, so an evacuated symbol or prototype is admitted again at its + new address and the old address's bits stay set. The number that matters is + therefore the false-positive rate at END of run, and it was measured on the + shipped binary rather than assumed — see the census below. + +#### The census on the shipped binary + +Re-running the uretprobe answer census on the built `claude --help` binary, +after the change: + +| probe | calls | "yes" | +|---|---|---| +| `is_registered_symbol` | 378,163 → **1,492** | 622 → **622** | +| `is_uint8array_buffer` | 537,921 → **0** | 0 → 0 | +| `is_registered_class_prototype_object` | 26,290 → 26,290 | 122 → **122** | + +Every genuine "yes" survives. The symbol filter's 1,492 admissions are 622 real +answers plus 870 false positives — 0.23% of the 377,541 negatives, at the end +of a run in which the population was evacuated and re-admitted throughout. The +class-prototype probe is still *entered* the same number of times (the filter +is inside the function, which has three call sites and is not `#[inline]`); +what it no longer does is the scan, which is where its 0.46% lived. + +`--help` output stayed byte-identical to node — 9,175 bytes, rc=0 — in every +arm, before and after. + +#### Why it cannot misclassify + +These probes classify pointers, so a wrong answer is type confusion rather than +a slow path. A Bloom filter has false positives and no false negatives, which +is the asymmetry the probes need: a false positive costs the ordinary lookup +that was already there. + +That leaves one obligation — every registration must admit before it publishes +— and it is not left as an enumeration of writers. Enumerations of writer sets +have produced silent wrong answers in this codebase twice recently, so under +`debug_assertions` **every rejection is re-derived from the authoritative +table**: `SYMBOL_POINTERS` for the symbol filter, `CLASS_PROTOTYPE_OBJECTS` for +the prototype filter, `is_uint8array_buffer_slow` for the Uint8Array window. A +registration route added without admitting panics in the first test that +touches it. The class-prototype table is the harder case — the two GC root +scanners and the per-slot GC step all *rewrite* stored addresses through +`visit_usize_slot`, so each admits what the visitor left behind, under the same +write guard that keeps the new address unfindable until it drops. + +`admit` performs its bit-set RMWs unconditionally, with no "already set?" +pre-check, for the reason #9272 gives for the window's: a thread that skips the +RMW performs no acquire, so another thread's admission never joins its +happens-before graph and a reader synchronising only with this thread could +miss it. diff --git a/crates/perry-codegen/src/stmt/loops.rs b/crates/perry-codegen/src/stmt/loops.rs index 07ae4c79f2..fc271d3eb9 100644 --- a/crates/perry-codegen/src/stmt/loops.rs +++ b/crates/perry-codegen/src/stmt/loops.rs @@ -872,11 +872,7 @@ fn emit_packed_numeric_accumulator_admission( offset_reads_inlined: bool, ) -> PackedAccumulatorScope { let accumulators = super::stable_packed_accumulator::collect_numeric_accumulators( - ctx, - body, - array_id, - counter_id, - offset_reads_inlined, + ctx, body, array_id, counter_id, offset_reads_inlined, ); // Integer (`c++`) accumulators admit independently of the float set — // a pure count loop has no float accumulator at all. diff --git a/crates/perry-codegen/src/stmt/stable_packed_accumulator.rs b/crates/perry-codegen/src/stmt/stable_packed_accumulator.rs index 2edce30b26..0bc7abdec6 100644 --- a/crates/perry-codegen/src/stmt/stable_packed_accumulator.rs +++ b/crates/perry-codegen/src/stmt/stable_packed_accumulator.rs @@ -67,8 +67,10 @@ fn accumulator_rhs_is_numeric( // expression lowers to a tag-test diamond over // `js_dynamic_string_or_number_add` — the same cost #9060 and // #9091 removed for the bare-counter form. - _ if offset_reads_inlined => crate::expr::packed_f64_loop_index_parts(index) - .is_some_and(|(i, _)| i == counter_id), + _ if offset_reads_inlined => { + crate::expr::packed_f64_loop_index_parts(index) + .is_some_and(|(i, _)| i == counter_id) + } _ => false, } } @@ -76,42 +78,17 @@ fn accumulator_rhs_is_numeric( candidates.contains(id) || crate::type_analysis::is_numeric_expr(ctx, expr) } Expr::Binary { left, right, .. } => { - accumulator_rhs_is_numeric( - ctx, - left, - array_id, - counter_id, - offset_reads_inlined, - candidates, - ) && accumulator_rhs_is_numeric( - ctx, - right, - array_id, - counter_id, - offset_reads_inlined, - candidates, - ) + accumulator_rhs_is_numeric(ctx, left, array_id, counter_id, offset_reads_inlined, candidates) + && accumulator_rhs_is_numeric(ctx, right, array_id, counter_id, offset_reads_inlined, candidates) + } + Expr::NumberCoerce(operand) => { + accumulator_rhs_is_numeric(ctx, operand, array_id, counter_id, offset_reads_inlined, candidates) } - Expr::NumberCoerce(operand) => accumulator_rhs_is_numeric( - ctx, - operand, - array_id, - counter_id, - offset_reads_inlined, - candidates, - ), Expr::Unary { op, operand } => { matches!( op, perry_hir::UnaryOp::Neg | perry_hir::UnaryOp::Pos | perry_hir::UnaryOp::BitNot - ) && accumulator_rhs_is_numeric( - ctx, - operand, - array_id, - counter_id, - offset_reads_inlined, - candidates, - ) + ) && accumulator_rhs_is_numeric(ctx, operand, array_id, counter_id, offset_reads_inlined, candidates) } Expr::MathAbs(v) | Expr::MathSqrt(v) @@ -120,41 +97,16 @@ fn accumulator_rhs_is_numeric( | Expr::MathRound(v) | Expr::MathTrunc(v) | Expr::MathSign(v) - | Expr::MathFround(v) => accumulator_rhs_is_numeric( - ctx, - v, - array_id, - counter_id, - offset_reads_inlined, - candidates, - ), + | Expr::MathFround(v) => { + accumulator_rhs_is_numeric(ctx, v, array_id, counter_id, offset_reads_inlined, candidates) + } Expr::MathImul(l, r) | Expr::MathPow(l, r) => { - accumulator_rhs_is_numeric( - ctx, - l, - array_id, - counter_id, - offset_reads_inlined, - candidates, - ) && accumulator_rhs_is_numeric( - ctx, - r, - array_id, - counter_id, - offset_reads_inlined, - candidates, - ) + accumulator_rhs_is_numeric(ctx, l, array_id, counter_id, offset_reads_inlined, candidates) + && accumulator_rhs_is_numeric(ctx, r, array_id, counter_id, offset_reads_inlined, candidates) } - Expr::MathMin(values) | Expr::MathMax(values) => values.iter().all(|v| { - accumulator_rhs_is_numeric( - ctx, - v, - array_id, - counter_id, - offset_reads_inlined, - candidates, - ) - }), + Expr::MathMin(values) | Expr::MathMax(values) => values + .iter() + .all(|v| accumulator_rhs_is_numeric(ctx, v, array_id, counter_id, offset_reads_inlined, candidates)), _ => false, } } @@ -331,14 +283,16 @@ pub(super) fn collect_numeric_accumulators( .copied() .filter(|id| { !writes[id].iter().all(|write| match write { - Some(rhs) => accumulator_rhs_is_numeric( - ctx, - rhs, - array_id, - counter_id, - offset_reads_inlined, - &candidates, - ), + Some(rhs) => { + accumulator_rhs_is_numeric( + ctx, + rhs, + array_id, + counter_id, + offset_reads_inlined, + &candidates, + ) + } // `Update` (++/--): ToNumeric(Number) ± 1 is a Number. None => true, }) diff --git a/crates/perry-runtime/src/buffer/header.rs b/crates/perry-runtime/src/buffer/header.rs index 8ab8823385..c6079eddc5 100644 --- a/crates/perry-runtime/src/buffer/header.rs +++ b/crates/perry-runtime/src/buffer/header.rs @@ -268,6 +268,48 @@ static ARRAY_BUFFER_EVER_MARKED: RegistryLatch = RegistryLatch::new(); static SHARED_ARRAY_BUFFER_EVER_MARKED: RegistryLatch = RegistryLatch::new(); static DATA_VIEW_EVER_MARKED: RegistryLatch = RegistryLatch::new(); static UINT8ARRAY_EVER_MARKED: RegistryLatch = RegistryLatch::new(); + +/// Smallest and largest address ever marked as a `new Uint8Array(...)` +/// backing, process-wide. +/// +/// `UINT8ARRAY_EVER_MARKED` stops discriminating at the first Uint8Array, and +/// `typedarray_props::typed_array_owner_kind` asks this question about the +/// receiver of **every untyped element access** — so on `claude-code --help` +/// the latch is armed for essentially the whole run and the probe is a +/// permanent out-of-line call, a `OnceLock` load, a thread-local resolution and +/// a `RefCell` borrow to say "no". +/// +/// It covers both tables `is_uint8array_buffer_slow` consults, each of which +/// has exactly one insert funnel: +/// * `UINT8ARRAY_FROM_CTOR` — only [`mark_as_uint8array`] inserts; +/// * the process-global external registry — only +/// [`register_external_uint8array`] inserts, and both of ITS callers reach +/// [`mark_as_uint8array`] with the same address anyway. +/// +/// Both widen before they publish, so an address outside the window is in +/// neither table and rejecting it is sound. Removal (the GC's dead-buffer +/// sweep) never narrows the window, which only makes it a weaker filter, never +/// a wrong one. See [`RegistryAddrWindow`] for the ordering rule. +static UINT8ARRAY_ADDR_WINDOW: RegistryAddrWindow = RegistryAddrWindow::new(); + +#[cfg(test)] +thread_local! { +/// Test-only count of `is_uint8array_buffer` calls that got past the address +/// window and reached the registries — the twin of +/// `TEST_BUFFER_REGISTRY_PROBES`, for the same reason: a fast path nobody can +/// prove ran is not a fast path. + static TEST_UINT8ARRAY_REGISTRY_PROBES: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +#[cfg(test)] +pub(crate) fn test_uint8array_registry_probe_count() -> u64 { + TEST_UINT8ARRAY_REGISTRY_PROBES.with(|c| c.get()) +} + +#[cfg(test)] +pub(crate) fn test_uint8array_addr_window_bounds() -> Option<(usize, usize)> { + UINT8ARRAY_ADDR_WINDOW.bounds_for_tests() +} static SECRET_KEY_EVER_MARKED: RegistryLatch = RegistryLatch::new(); static CRYPTO_KEY_EVER_MARKED: RegistryLatch = RegistryLatch::new(); static ASYMMETRIC_KEY_EVER_MARKED: RegistryLatch = RegistryLatch::new(); @@ -468,6 +510,10 @@ fn is_registered_buffer_slow(addr: usize) -> bool { /// Mark this buffer as one that came from `new Uint8Array(...)` so it /// formats as `Uint8Array(N) [ ... ]` rather than ``. pub fn mark_as_uint8array(addr: usize) { + // Widen before arming, and arm before the insert: the probe consults the + // latch and then the window, so both must already cover this address by the + // time it becomes findable. See `crate::registry_latch`. + UINT8ARRAY_ADDR_WINDOW.admit(addr); UINT8ARRAY_EVER_MARKED.arm(); UINT8ARRAY_ADDR_RANGE.with(|r| { let (lo, hi) = r.get(); @@ -510,6 +556,13 @@ pub extern "C" fn js_buffer_mark_as_uint8array_external(addr: usize) { /// precisely so an address registered on one thread is visible from another, /// and the thread-local set that would otherwise cover it is not. fn register_external_uint8array(addr: usize) { + // Both of this function's callers also call `mark_as_uint8array(addr)`, + // which admits the same address — but that is an enumeration of callers, + // and this is the funnel the doc comment above promises is authoritative. + // Admitting here too costs two RMWs on a path that runs a handful of times + // per process and makes the funnel self-sufficient. + UINT8ARRAY_ADDR_WINDOW.admit(addr); + UINT8ARRAY_EVER_MARKED.arm(); EXTERNAL_UINT8ARRAYS_NONEMPTY.store(true, std::sync::atomic::Ordering::Release); if let Ok(mut r) = external_uint8arrays().lock() { r.insert(addr); @@ -659,6 +712,30 @@ pub fn is_uint8array_buffer(addr: usize) -> bool { if UINT8ARRAY_EVER_MARKED.is_idle() { return false; } + // An address outside the marked window is in neither table the slow path + // consults, so reject it inline — no call, no `OnceLock`, no thread-local + // resolution, no `RefCell` borrow. See `UINT8ARRAY_ADDR_WINDOW`. + if !UINT8ARRAY_ADDR_WINDOW.may_contain(addr) { + // Completeness audit, machine-checked rather than enumerated — see the + // twin in `is_registered_buffer` for why. `is_uint8array_buffer_slow` + // is the authoritative reader of both tables and mutates nothing, so + // calling it here changes no state the next probe would observe. + // Compiled out entirely in release. + #[cfg(debug_assertions)] + { + assert!( + !is_uint8array_buffer_slow(addr), + "UINT8ARRAY_ADDR_WINDOW rejected {addr:#x}, but it IS a marked \ + Uint8Array backing. Some route reached UINT8ARRAY_FROM_CTOR or \ + the external-Uint8Array registry without calling \ + `UINT8ARRAY_ADDR_WINDOW.admit()` (via `mark_as_uint8array` or \ + `register_external_uint8array`) first." + ); + } + return false; + } + #[cfg(test)] + TEST_UINT8ARRAY_REGISTRY_PROBES.with(|c| c.set(c.get().wrapping_add(1))); is_uint8array_buffer_slow(addr) } diff --git a/crates/perry-runtime/src/buffer/mod.rs b/crates/perry-runtime/src/buffer/mod.rs index fc7b1477ee..3dc3e0c76e 100644 --- a/crates/perry-runtime/src/buffer/mod.rs +++ b/crates/perry-runtime/src/buffer/mod.rs @@ -65,7 +65,8 @@ pub(crate) use header::{ #[cfg(test)] pub(crate) use header::{ test_buffer_addr_window_bounds, test_buffer_registry_probe_count, test_data_view_registry_len, - test_shared_array_buffer_registry_len, + test_shared_array_buffer_registry_len, test_uint8array_addr_window_bounds, + test_uint8array_registry_probe_count, }; // ---- Re-exports: ArrayBuffer detach / transfer (ES2024) ---- diff --git a/crates/perry-runtime/src/gc/tests/copying_side_tables.rs b/crates/perry-runtime/src/gc/tests/copying_side_tables.rs index d1465e23c6..3f41818c80 100644 --- a/crates/perry-runtime/src/gc/tests/copying_side_tables.rs +++ b/crates/perry-runtime/src/gc/tests/copying_side_tables.rs @@ -184,12 +184,11 @@ fn test_copying_minor_keeps_moved_symbol_visible_to_the_range_filter() { crate::symbol::test_seed_symbol_pointer_root(sym_key); js_shadow_slot_set(0, ptr_bits(sym_key)); - let (lo_before, hi_before) = crate::symbol::test_symbol_addr_range(); - assert_eq!( - (lo_before, hi_before), - (sym_key, sym_key), - "fixture must start with a single-point range, or the move below \ - cannot land outside it and the assertion is vacuous" + let filter_before = crate::symbol::test_symbol_filter_snapshot(); + assert!( + crate::symbol::test_symbol_filter_bits_set() <= 3, + "fixture must start from a filter holding exactly this one symbol \ + (at most its three bits), or the assertion below is vacuous" ); let _ = gc_collect_minor(); @@ -202,12 +201,17 @@ fn test_copying_minor_keeps_moved_symbol_visible_to_the_range_filter() { assert!(crate::symbol::test_symbol_pointer_root_contains( sym_key_after )); + assert!( + !crate::symbol::test_symbol_filter_snapshot_may_contain(&filter_before, sym_key_after), + "fixture must move the symbol to an address the filter its own \ + allocation built does NOT already accept ({sym_key_after:#x}), or the \ + assertion below passes without the forwarding rewrite doing anything" + ); assert!( crate::symbol::is_registered_symbol(sym_key_after), - "a symbol evacuated to {sym_key_after:#x}, outside the \ - [{lo_before:#x}, {hi_before:#x}] range its allocation established, is \ - still live and registered — the range filter must have been widened \ - by the forwarding rewrite" + "a symbol evacuated to {sym_key_after:#x}, which the filter its \ + allocation established rejects, is still live and registered — the \ + forwarding rewrite must have admitted the new address" ); } diff --git a/crates/perry-runtime/src/object/class_gc_roots.rs b/crates/perry-runtime/src/object/class_gc_roots.rs index b0a221ac95..4e2d21e856 100644 --- a/crates/perry-runtime/src/object/class_gc_roots.rs +++ b/crates/perry-runtime/src/object/class_gc_roots.rs @@ -47,6 +47,11 @@ pub fn scan_class_inheritance_roots_mut(visitor: &mut crate::gc::RuntimeRootVisi if let Some(map) = guard.as_mut() { for ptr in map.values_mut() { visitor.visit_usize_slot(ptr); + // Evacuation moves a prototype to an address the filter has + // never seen. Admit what the visitor left behind, under the + // same write guard, so the address is admitted before any + // reader can find it. + crate::object::class_registry::note_class_prototype_object_registered(*ptr); } } } @@ -74,6 +79,7 @@ pub fn scan_class_inheritance_roots_mut(visitor: &mut crate::gc::RuntimeRootVisi #[cfg(test)] pub(crate) fn test_seed_class_inheritance_roots(proto_cid: u32, proto_ptr: usize) { // GC_STORE_AUDIT(ROOT): test seed mirrors CLASS_PROTOTYPE_OBJECTS values scanned by scan_class_inheritance_roots_mut. + crate::object::class_registry::note_class_prototype_object_registered(proto_ptr); CLASS_PROTOTYPE_OBJECTS.with(|table| { let mut guard = table.write().unwrap(); guard diff --git a/crates/perry-runtime/src/object/class_registry.rs b/crates/perry-runtime/src/object/class_registry.rs index 7e8c199f04..9ac3cbcf4b 100644 --- a/crates/perry-runtime/src/object/class_registry.rs +++ b/crates/perry-runtime/src/object/class_registry.rs @@ -72,8 +72,8 @@ pub(crate) use state::{ class_static_prototype_is_nulled, class_static_prototype_root_clear, class_static_prototype_root_store, class_static_set_defined_attrs, class_unmark_key_deleted, global_object_prototype_bits, is_bound_native_constructor_closure_value, - is_non_constructable_builtin_function_value, parent_closure_in_chain, - throw_non_constructable_builtin_function, + is_non_constructable_builtin_function_value, note_class_prototype_object_registered, + parent_closure_in_chain, throw_non_constructable_builtin_function, CLASS_PROTOTYPE_ADDR_FILTER, }; pub use state::{ ClassVTable, VTableMethodEntry, CLASS_DECL_PROTOTYPE_OBJECTS, CLASS_DYNAMIC_PARENT_VALUE, @@ -184,6 +184,8 @@ pub(crate) use dispatch::{ }; // ── parent_static.rs ──────────────────────────────────────────────────────── +#[cfg(test)] +pub(crate) use parent_static::test_class_prototype_scan_count; pub(crate) use parent_static::{ call_private_static_method_for_owner, call_registered_static_method, call_static_method, class_chain_has_instance_accessor, class_dynamic_static_accessor_descriptor, diff --git a/crates/perry-runtime/src/object/class_registry/gc_roots.rs b/crates/perry-runtime/src/object/class_registry/gc_roots.rs index 941d7bcefa..40882a0cb7 100644 --- a/crates/perry-runtime/src/object/class_registry/gc_roots.rs +++ b/crates/perry-runtime/src/object/class_registry/gc_roots.rs @@ -114,6 +114,8 @@ pub fn scan_class_side_table_roots_mut(visitor: &mut crate::gc::RuntimeRootVisit if let Some(map) = guard.as_mut() { for proto_addr in map.values_mut() { visitor.visit_usize_slot(proto_addr); + // See the twin in `class_gc_roots::scan_class_inheritance_roots_mut`. + super::note_class_prototype_object_registered(*proto_addr); } } } @@ -413,6 +415,9 @@ fn scan_class_side_table_root_slot( if let Ok(mut guard) = table.write() { if let Some(proto_addr) = guard.as_mut().and_then(|map| map.get_mut(class_id)) { visitor.visit_usize_slot(proto_addr); + // The per-slot GC step moves one prototype at a time; + // it carries the same obligation as the bulk scanner. + super::note_class_prototype_object_registered(*proto_addr); } } }); diff --git a/crates/perry-runtime/src/object/class_registry/parent_static.rs b/crates/perry-runtime/src/object/class_registry/parent_static.rs index 308986447b..db98d25382 100644 --- a/crates/perry-runtime/src/object/class_registry/parent_static.rs +++ b/crates/perry-runtime/src/object/class_registry/parent_static.rs @@ -1776,6 +1776,44 @@ pub fn is_registered_class_prototype_object(ptr: usize) -> bool { if crate::value::addr_class::is_handle_band(ptr) { return false; } + // An address no registration ever admitted cannot be in the map, so reject + // it here — inline, and in particular before the `map.values().any(…)` + // linear scan below, which is what this probe actually costs (#9225). + // 99.05% of the calls on `claude-code --help` end here. + if !crate::object::class_registry::CLASS_PROTOTYPE_ADDR_FILTER.may_contain(ptr) { + // Machine-check the writer set rather than enumerate it. The filter is + // sound only if EVERY route that stores an address into + // `CLASS_PROTOTYPE_OBJECTS` admits it first — the insert, both GC root + // scanners, the per-slot GC step and the test seeds — and a route added + // without admitting would not crash: it would silently report a live + // prototype as "not a prototype". In a debug build every rejection is + // therefore re-derived from the map itself, which turns that into a + // panic in the first test that exercises the route. Compiled out + // entirely in release. + #[cfg(debug_assertions)] + { + // `try_read`, not `read`, for the reason the symbol twin gives: + // the rejection path never took this lock before, so a blocking + // audit could hang on a caller the audited code would not have. + let present = CLASS_PROTOTYPE_OBJECTS.with(|table| { + table.try_read().is_ok_and(|guard| { + guard + .as_ref() + .is_some_and(|map| map.values().any(|&p| p == ptr)) + }) + }); + assert!( + !present, + "CLASS_PROTOTYPE_ADDR_FILTER rejected {ptr:#x}, but it IS a \ + registered class prototype. Some route stored it into \ + CLASS_PROTOTYPE_OBJECTS without calling \ + `note_class_prototype_object_registered` first." + ); + } + return false; + } + #[cfg(test)] + TEST_CLASS_PROTOTYPE_SCANS.with(|c| c.set(c.get().wrapping_add(1))); CLASS_PROTOTYPE_OBJECTS.with(|table| { if let Ok(guard) = table.read() { if let Some(map) = guard.as_ref() { @@ -1786,6 +1824,20 @@ pub fn is_registered_class_prototype_object(ptr: usize) -> bool { }) } +#[cfg(test)] +thread_local! { +/// Test-only count of `is_registered_class_prototype_object` calls that got +/// past `CLASS_PROTOTYPE_ADDR_FILTER` and reached the linear scan. The filter +/// is a fast path, and a fast path nobody can prove ran is not a fast path +/// (same contract as `buffer::header::TEST_BUFFER_REGISTRY_PROBES`). + static TEST_CLASS_PROTOTYPE_SCANS: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +#[cfg(test)] +pub(crate) fn test_class_prototype_scan_count() -> u64 { + TEST_CLASS_PROTOTYPE_SCANS.with(|c| c.get()) +} + /// Walk the prototype chain of `class_id` and return the id of the class that /// actually OWNS the method `name` (the prototype where it is defined). Used to /// make method-as-value identity stable: a class method is a single shared @@ -1816,177 +1868,5 @@ pub fn method_owner_class_id(class_id: u32, name: &str) -> Option { mod unstamped_tests; #[cfg(test)] -mod shape_authority_tests_8067 { - fn key<'scope>( - scope: &'scope crate::gc::RuntimeHandleScope, - name: &str, - ) -> crate::gc::RuntimeHandle<'scope> { - scope.root_string_ptr(crate::string::js_string_from_bytes( - name.as_ptr(), - name.len() as u32, - )) - } - - #[test] - fn mark_class_rejects_non_heap_addresses() { - // Representative ids from the native-handle and proxy bands. The - // extern entry point must validate before reading a preceding header. - super::js_object_mark_class(0x40000); - super::js_object_mark_class(1); - } - - #[test] - fn class_kind_survives_static_field_installation_and_deletion() { - let _lock = crate::gc::global_side_table_test_lock(); - unsafe { - const CID: u32 = 0x8067; - let scope = crate::gc::RuntimeHandleScope::new(); - let obj_handle = scope.root_raw_mut_ptr(crate::object::js_object_alloc(CID, 8)); - let before = obj_handle.with_mut_ptr::(|obj| { - let before = crate::object::shapes::object_shape_id(obj); - assert!(crate::object::object_is_regular(obj)); - before - }); - - let ((), obj) = obj_handle.across_mut::(|| { - obj_handle.with_mut_ptr::(|obj| { - super::js_object_mark_class(obj as i64) - }) - }); - let after = crate::object::shapes::object_shape_id(obj); - assert_ne!(before, after, "becoming a class object is semantic"); - assert_eq!( - crate::object::shapes::object_shape_descriptor(obj) - .expect("class descriptor") - .object_kind, - crate::object::shapes::ShapeObjectKind::Class - ); - - // #8113 removed the `object_type` compatibility mirror this used to - // sabotage. Classification is driven by the ShapeId descriptor - // transition above and by nothing else, so assert that directly. - assert!(super::is_class_object_ptr(obj.cast())); - assert!(!crate::object::object_is_regular(obj)); - - // Numeric layout installation historically set/cleared bits in - // GcHeader::_reserved, where the old class marker collided with - // GC_LAYOUT_ALL_POINTERS. Shape kind must be unaffected. - let numeric_key = key(&scope, "numericStatic"); - let ((), obj) = obj_handle.across_mut::(|| { - obj_handle.with_mut_ptr::(|obj| { - numeric_key.with_const_ptr::(|key| { - crate::object::js_object_set_field_by_name(obj, key, 42.0) - }) - }) - }); - assert!( - super::is_class_object_ptr(obj.cast()), - "numeric static write changed class descriptor: {:?}", - crate::object::shapes::object_shape_descriptor(obj) - ); - assert!(!crate::object::object_is_regular(obj)); - - // Repeat with a pointer-bearing static value, which drives the - // opposite GC layout state and used to erase the aliased bit. - let pointer_key = key(&scope, "pointerStatic"); - let payload = key(&scope, "rootedStaticValue"); - let ((), obj) = obj_handle.across_mut::(|| { - obj_handle.with_mut_ptr::(|obj| { - pointer_key.with_const_ptr::(|key| { - payload.with_mut_ptr::(|payload| { - let value = - f64::from_bits(crate::value::JSValue::string_ptr(payload).bits()); - crate::object::js_object_set_field_by_name(obj, key, value) - }) - }) - }) - }); - assert!(super::is_class_object_ptr(obj.cast())); - assert!(!crate::object::object_is_regular(obj)); - assert_eq!( - crate::object::shapes::object_shape_descriptor(obj) - .expect("post-write class descriptor") - .object_kind, - crate::object::shapes::ShapeObjectKind::Class, - "class kind must never share storage with GC layout flags" - ); - - // The pointer-bearing write above leaves a typed/side-mask layout. - // Growing the keys array from that state invalidates typed - // feedback. The invalidation asks for the receiver's shape, so a - // keys transition must keep the old class stamp visible until the - // invalidation finishes and must prefer its saved predecessor over - // any defensive self-heal in the temporary cleared-stamp window. - let after_pointer_key = key(&scope, "afterPointerStatic"); - let ((), obj) = obj_handle.across_mut::(|| { - obj_handle.with_mut_ptr::(|obj| { - after_pointer_key.with_const_ptr::(|key| { - crate::object::js_object_set_field_by_name(obj, key, 7.0) - }) - }) - }); - assert!( - super::is_class_object_ptr(obj.cast()), - "typed-layout invalidation erased class descriptor lineage: {:?}", - crate::object::shapes::object_shape_descriptor(obj) - ); - assert_eq!( - crate::object::shapes::object_shape_descriptor(obj) - .expect("post-invalidation class descriptor") - .object_kind, - crate::object::shapes::ShapeObjectKind::Class - ); - - // Deletion installs a cloned keys array, which clears the current - // stamp. The replacement descriptor must inherit class kind from - // the predecessor captured before that clear. - let (deleted, obj) = obj_handle.across_mut::(|| { - obj_handle.with_mut_ptr::(|obj| { - numeric_key.with_const_ptr::(|key| { - crate::object::js_object_delete_field(obj, key) - }) - }) - }); - assert_eq!(deleted, 1); - assert!( - super::is_class_object_ptr(obj.cast()), - "deleting a static field erased class descriptor lineage: {:?}", - crate::object::shapes::object_shape_descriptor(obj) - ); - assert_eq!( - crate::object::shapes::object_shape_descriptor(obj) - .expect("post-delete class descriptor") - .object_kind, - crate::object::shapes::ShapeObjectKind::Class - ); - - let class_value = crate::value::js_nanbox_pointer(obj as i64); - let class_value_handle = scope.root_nanbox_f64(class_value); - let typeof_ptr = crate::builtins::js_value_typeof(class_value); - assert_eq!(crate::regex::string_as_str(typeof_ptr), "function"); - - let instance = crate::object::js_new_function_construct( - class_value_handle.get_nanbox_f64(), - std::ptr::null(), - 0, - ); - let instance_handle = scope.root_nanbox_f64(instance); - let instance_value = crate::value::JSValue::from_bits(instance.to_bits()); - assert!( - instance_value.is_pointer(), - "construction must return an object" - ); - let instance_ptr = instance_value.as_pointer::(); - assert_eq!((*instance_ptr).class_id, CID); - assert_eq!( - crate::object::js_instanceof_dynamic( - instance_handle.get_nanbox_f64(), - class_value_handle.get_nanbox_f64(), - ) - .to_bits(), - crate::value::TAG_TRUE, - "instanceof must still recognize the class after static writes" - ); - } - } -} +#[path = "parent_static/shape_authority_tests_8067.rs"] +mod shape_authority_tests_8067; diff --git a/crates/perry-runtime/src/object/class_registry/parent_static/shape_authority_tests_8067.rs b/crates/perry-runtime/src/object/class_registry/parent_static/shape_authority_tests_8067.rs new file mode 100644 index 0000000000..3fe0319d3a --- /dev/null +++ b/crates/perry-runtime/src/object/class_registry/parent_static/shape_authority_tests_8067.rs @@ -0,0 +1,175 @@ +//! #8067 shape-authority tests, split out of `parent_static.rs` to keep it +//! under the 2000-line cap. Body unchanged; only its home file moved. + +fn key<'scope>( + scope: &'scope crate::gc::RuntimeHandleScope, + name: &str, +) -> crate::gc::RuntimeHandle<'scope> { + scope.root_string_ptr(crate::string::js_string_from_bytes( + name.as_ptr(), + name.len() as u32, + )) +} + +#[test] +fn mark_class_rejects_non_heap_addresses() { + // Representative ids from the native-handle and proxy bands. The + // extern entry point must validate before reading a preceding header. + super::js_object_mark_class(0x40000); + super::js_object_mark_class(1); +} + +#[test] +fn class_kind_survives_static_field_installation_and_deletion() { + let _lock = crate::gc::global_side_table_test_lock(); + unsafe { + const CID: u32 = 0x8067; + let scope = crate::gc::RuntimeHandleScope::new(); + let obj_handle = scope.root_raw_mut_ptr(crate::object::js_object_alloc(CID, 8)); + let before = obj_handle.with_mut_ptr::(|obj| { + let before = crate::object::shapes::object_shape_id(obj); + assert!(crate::object::object_is_regular(obj)); + before + }); + + let ((), obj) = obj_handle.across_mut::(|| { + obj_handle.with_mut_ptr::(|obj| { + super::js_object_mark_class(obj as i64) + }) + }); + let after = crate::object::shapes::object_shape_id(obj); + assert_ne!(before, after, "becoming a class object is semantic"); + assert_eq!( + crate::object::shapes::object_shape_descriptor(obj) + .expect("class descriptor") + .object_kind, + crate::object::shapes::ShapeObjectKind::Class + ); + + // #8113 removed the `object_type` compatibility mirror this used to + // sabotage. Classification is driven by the ShapeId descriptor + // transition above and by nothing else, so assert that directly. + assert!(super::is_class_object_ptr(obj.cast())); + assert!(!crate::object::object_is_regular(obj)); + + // Numeric layout installation historically set/cleared bits in + // GcHeader::_reserved, where the old class marker collided with + // GC_LAYOUT_ALL_POINTERS. Shape kind must be unaffected. + let numeric_key = key(&scope, "numericStatic"); + let ((), obj) = obj_handle.across_mut::(|| { + obj_handle.with_mut_ptr::(|obj| { + numeric_key.with_const_ptr::(|key| { + crate::object::js_object_set_field_by_name(obj, key, 42.0) + }) + }) + }); + assert!( + super::is_class_object_ptr(obj.cast()), + "numeric static write changed class descriptor: {:?}", + crate::object::shapes::object_shape_descriptor(obj) + ); + assert!(!crate::object::object_is_regular(obj)); + + // Repeat with a pointer-bearing static value, which drives the + // opposite GC layout state and used to erase the aliased bit. + let pointer_key = key(&scope, "pointerStatic"); + let payload = key(&scope, "rootedStaticValue"); + let ((), obj) = obj_handle.across_mut::(|| { + obj_handle.with_mut_ptr::(|obj| { + pointer_key.with_const_ptr::(|key| { + payload.with_mut_ptr::(|payload| { + let value = + f64::from_bits(crate::value::JSValue::string_ptr(payload).bits()); + crate::object::js_object_set_field_by_name(obj, key, value) + }) + }) + }) + }); + assert!(super::is_class_object_ptr(obj.cast())); + assert!(!crate::object::object_is_regular(obj)); + assert_eq!( + crate::object::shapes::object_shape_descriptor(obj) + .expect("post-write class descriptor") + .object_kind, + crate::object::shapes::ShapeObjectKind::Class, + "class kind must never share storage with GC layout flags" + ); + + // The pointer-bearing write above leaves a typed/side-mask layout. + // Growing the keys array from that state invalidates typed + // feedback. The invalidation asks for the receiver's shape, so a + // keys transition must keep the old class stamp visible until the + // invalidation finishes and must prefer its saved predecessor over + // any defensive self-heal in the temporary cleared-stamp window. + let after_pointer_key = key(&scope, "afterPointerStatic"); + let ((), obj) = obj_handle.across_mut::(|| { + obj_handle.with_mut_ptr::(|obj| { + after_pointer_key.with_const_ptr::(|key| { + crate::object::js_object_set_field_by_name(obj, key, 7.0) + }) + }) + }); + assert!( + super::is_class_object_ptr(obj.cast()), + "typed-layout invalidation erased class descriptor lineage: {:?}", + crate::object::shapes::object_shape_descriptor(obj) + ); + assert_eq!( + crate::object::shapes::object_shape_descriptor(obj) + .expect("post-invalidation class descriptor") + .object_kind, + crate::object::shapes::ShapeObjectKind::Class + ); + + // Deletion installs a cloned keys array, which clears the current + // stamp. The replacement descriptor must inherit class kind from + // the predecessor captured before that clear. + let (deleted, obj) = obj_handle.across_mut::(|| { + obj_handle.with_mut_ptr::(|obj| { + numeric_key.with_const_ptr::(|key| { + crate::object::js_object_delete_field(obj, key) + }) + }) + }); + assert_eq!(deleted, 1); + assert!( + super::is_class_object_ptr(obj.cast()), + "deleting a static field erased class descriptor lineage: {:?}", + crate::object::shapes::object_shape_descriptor(obj) + ); + assert_eq!( + crate::object::shapes::object_shape_descriptor(obj) + .expect("post-delete class descriptor") + .object_kind, + crate::object::shapes::ShapeObjectKind::Class + ); + + let class_value = crate::value::js_nanbox_pointer(obj as i64); + let class_value_handle = scope.root_nanbox_f64(class_value); + let typeof_ptr = crate::builtins::js_value_typeof(class_value); + assert_eq!(crate::regex::string_as_str(typeof_ptr), "function"); + + let instance = crate::object::js_new_function_construct( + class_value_handle.get_nanbox_f64(), + std::ptr::null(), + 0, + ); + let instance_handle = scope.root_nanbox_f64(instance); + let instance_value = crate::value::JSValue::from_bits(instance.to_bits()); + assert!( + instance_value.is_pointer(), + "construction must return an object" + ); + let instance_ptr = instance_value.as_pointer::(); + assert_eq!((*instance_ptr).class_id, CID); + assert_eq!( + crate::object::js_instanceof_dynamic( + instance_handle.get_nanbox_f64(), + class_value_handle.get_nanbox_f64(), + ) + .to_bits(), + crate::value::TAG_TRUE, + "instanceof must still recognize the class after static writes" + ); + } +} diff --git a/crates/perry-runtime/src/object/class_registry/state.rs b/crates/perry-runtime/src/object/class_registry/state.rs index 7adfb8092b..73516022de 100644 --- a/crates/perry-runtime/src/object/class_registry/state.rs +++ b/crates/perry-runtime/src/object/class_registry/state.rs @@ -320,6 +320,56 @@ crate::perry_thread_local! { pub static CLASS_PROTOTYPE_OBJECTS: RwLock>> = RwLock::new(None); } +/// Monotone address filter over the values of [`CLASS_PROTOTYPE_OBJECTS`]. +/// +/// `is_registered_class_prototype_object` answers "is this heap object some +/// class's registered prototype?" with `map.values().any(…)` — a LINEAR SCAN, +/// #9225 — and the caller that asks it is +/// `descriptor_state::disable_inline_guards_for_descriptor_target`, which runs +/// on every `Object.defineProperty`. esbuild's `__export(exports, { … })` makes +/// that thousands of calls per bundle: on `claude-code --help` the probe is +/// called 26,290 times, answers `true` **122** times, and costs **0.46%** of +/// the run — roughly 1,200 instructions per call, which is the scan. +/// +/// A monotone `[lo, hi]` window cannot help here: prototypes are ordinary +/// GC-heap objects interleaved with everything else, and replaying the real +/// argument stream against the window the registrations build rejects only +/// 54.0%. The same replay against this filter rejects **99.05%** (26,041 of +/// 26,290; 122 genuine `true` answers preserved, 127 false positives), and it +/// rejects them before the thread-local resolution, the `RwLock` and the scan. +/// +/// Rejecting is sound because every route that puts an address into the map +/// admits it here first — see [`note_class_prototype_object_registered`] — and +/// removals never clear bits, which only makes the filter weaker, never wrong. +/// The completeness of that writer set is machine-checked rather than +/// enumerated: see the probe. +/// +/// This does not close #9225. A false positive still pays the scan, so the +/// table's O(n) slope survives at ~1% of its strength; the O(1) inverse index +/// #9225 asks for is still the right structural fix, and this filter sits in +/// front of it either way. +pub(crate) static CLASS_PROTOTYPE_ADDR_FILTER: crate::registry_latch::RegistryAddrFilter = + crate::registry_latch::RegistryAddrFilter::new(); + +/// Admit `addr` into [`CLASS_PROTOTYPE_ADDR_FILTER`]. +/// +/// EVERY route that stores an address into [`CLASS_PROTOTYPE_OBJECTS`] must +/// call this **before** the address becomes findable — the insert below, the +/// two GC root scanners and the per-slot GC step (all of which rewrite stored +/// addresses through `visit_usize_slot`), and the test seeds. A route that +/// forgets does not crash: the probe reports a live prototype as "not a +/// prototype", so `getOwnPropertyDescriptor(C.prototype, …)` and the +/// descriptor-guard disable silently change behaviour. The debug-build audit in +/// the probe exists to turn that into a test failure. +/// +/// The GC scanners admit AFTER the visitor rewrites the slot, which is still +/// "before it is findable": they hold the table's write guard across both, so +/// no reader can observe the new address until the guard drops. +#[inline] +pub(crate) fn note_class_prototype_object_registered(addr: usize) { + CLASS_PROTOTYPE_ADDR_FILTER.admit(addr); +} + crate::perry_thread_local! { /// The CONSTRUCTOR's `[[Prototype]]`, set by `Object.setPrototypeOf(Ctor, obj)` /// on a declared class (perry represents those as INT32 ClassRefs, not heap @@ -492,6 +542,9 @@ pub(crate) fn class_prototype_object_root_store(class_id: u32, proto_ptr: *mut O if class_id == 0 || proto_ptr.is_null() { return; } + // Admit before the insert, so the address is never in the map while the + // filter still rejects it. See `note_class_prototype_object_registered`. + note_class_prototype_object_registered(proto_ptr as usize); CLASS_PROTOTYPE_OBJECTS.with(|table| { let mut guard = table.write().unwrap(); if guard.is_none() { diff --git a/crates/perry-runtime/src/object/descriptor_state.rs b/crates/perry-runtime/src/object/descriptor_state.rs index 9b49cf8e2a..1ec8bd19dd 100644 --- a/crates/perry-runtime/src/object/descriptor_state.rs +++ b/crates/perry-runtime/src/object/descriptor_state.rs @@ -257,9 +257,15 @@ pub(crate) fn test_reset_class_field_inline_guard() { /// setup that runs during every program's startup, `Object.freeze` on a config /// object, …) no longer disable the #5093 fast path process-wide. /// -/// The prototype-registry probes scan by value (O(#classes)); descriptor -/// installs are rare and never on the hot property path, so the scan cost is -/// acceptable. +/// The prototype-registry probes scan by value (O(#classes)). This comment +/// used to add "descriptor installs are rare and never on the hot property +/// path, so the scan cost is acceptable", and that is false for every bundle: +/// esbuild's `__export(exports, { … })` makes `Object.defineProperty` a +/// module-init primitive — claude-code's bundle contains 1,526 of them — so +/// this function runs 26,290 times on `claude --help` and +/// `is_registered_class_prototype_object`'s scan alone was 0.46% of the run. +/// It is now fronted by `CLASS_PROTOTYPE_ADDR_FILTER`, which rejects 99.05% of +/// those calls before the scan; the O(#classes) slope itself is #9225. /// /// #6759 C5a — per-KEY refinement (the follow-up the paragraph above used to /// promise): the inline fast path only ever compiles accesses to DECLARED diff --git a/crates/perry-runtime/src/registry_latch.rs b/crates/perry-runtime/src/registry_latch.rs index 68e1385d2b..dd6adcadcc 100644 --- a/crates/perry-runtime/src/registry_latch.rs +++ b/crates/perry-runtime/src/registry_latch.rs @@ -61,7 +61,7 @@ //! publishes a heap address through a lock-free path, so the stronger ordering //! is what ships. -use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicU64, AtomicUsize, Ordering}; /// A one-way "this feature has been registered at least once" flag. /// @@ -234,6 +234,234 @@ impl RegistryAddrWindow { } } +/// A monotone "which addresses have ever been registered?" **set filter** — +/// a Bloom filter over the registered addresses, for the probes a +/// [`RegistryAddrWindow`] cannot discriminate. +/// +/// # Why a second shape was needed +/// +/// The window works when the registered addresses sit in a narrow band. Two of +/// the probes measured after #9272 do not: their entries are ordinary GC-heap +/// objects, interleaved with everything else the program allocates, so `[lo, +/// hi]` grows to cover most of the heap and stops rejecting. Measured on +/// `claude-code --help` by replaying each probe's real argument stream against +/// the window the registrations would have built: +/// +/// | probe | calls | a window rejects | this filter rejects | +/// |---|---|---|---| +/// | `is_registered_symbol` | 378,163 | 38.3% | **99.58%** | +/// | `is_registered_class_prototype_object` | 26,290 | 54.0% | **99.05%** | +/// +/// (`is_uint8array_buffer`, whose entries are `BufferHeader`s, is the opposite +/// case: a window rejects **100%** of its 540,328 calls, so it keeps the +/// cheaper shape. Both are in the tree on purpose; pick by measurement.) +/// +/// # The contract, which is the window's contract +/// +/// `may_contain` returning `false` means "definitively not registered". A Bloom +/// filter has false positives and no false negatives, which is exactly the +/// asymmetry the probes need: a false positive costs the ordinary lookup that +/// was already there, and a false negative — the one dangerous answer — cannot +/// occur while every registration sets its bits before it publishes. +/// +/// Bits are only ever SET, never cleared, so unregistration (death pruning, the +/// GC's dead-buffer sweep) leaves the filter a weaker approximation, never a +/// wrong one. There is deliberately no `clear`. +/// +/// # Sizing +/// +/// 1,024 bits (16 `AtomicU64`, two cache lines) and three probes per address. +/// `claude-code --help` registers **100** symbols and **160** class prototypes, +/// so at 160 entries the theoretical false-positive rate is +/// `(1 - e^(-3·160/1024))³ ≈ 1.6%`; the measured rates on the real address +/// streams were 0.26% and 0.49%. +/// +/// **The saturation regime is deliberate, and it is the reason `WORDS` is the +/// only knob.** `CLASS_PROTOTYPE_OBJECTS` grows by one entry per ES5-transpiled +/// constructor (#9225), so a bundle much larger than claude-code's can push it +/// past ~700 entries, where 1,024 bits saturate and `may_contain` starts +/// answering `true` for almost everything. That is not a correctness cliff and +/// not a regression: a saturated filter is exactly the code that ran before it +/// existed — the probe falls through to the lookup it always did. It is a +/// *win* cliff. If a corpus is found sitting on the wrong side of it, raise +/// `WORDS`: 64 words (4,096 bits, 512 B) holds ~640 entries at the same +/// false-positive rate and is still trivially L1-resident. The value shipped +/// here is the one the measurements above were taken at, and is deliberately +/// not raised on speculation. +/// +/// **Bits accrue per ADMISSION, not per live entry.** Both tables this guards +/// are re-keyed by the collector — a symbol or a prototype that is evacuated is +/// admitted again at its new address, and the bits its old address set are +/// never cleared — so a long-running program with a moving nursery walks +/// towards saturation over time rather than sitting at a fixed occupancy. That +/// is measurable rather than theoretical, and it was measured: re-running the +/// answer census on the shipped `claude --help` binary, the symbol filter ended +/// the run admitting 1,492 of 378,163 probes, of which 622 were genuine — 870 +/// false positives, a 0.23% rate against a population that had been evacuated +/// and re-admitted throughout. The bound to watch is the false-positive rate at +/// END of run, not the registration count. +/// +/// # The ordering rule (binding, and identical to [`RegistryAddrWindow`]'s) +/// +/// **[`admit`](Self::admit) must run BEFORE the registry mutation it +/// advertises.** Setting the bits after the insert opens a window in which an +/// address is registered but the filter denies it. +/// +/// The three words are separate atomics, so a racing reader can observe a mix +/// of old and new. That is harmless for the same reason it is harmless for the +/// window: each word only ever gains bits, so once `may_contain` holds for an +/// address it holds forever, and a partially-observed filter is only ever a +/// weaker filter — never one that rejects something it used to accept. +/// +/// Cross-thread visibility rests on `admit`'s `AcqRel` read-modify-writes, and +/// on `may_contain`'s `Acquire` loads, exactly as for the window. As there, +/// `admit` performs its RMWs unconditionally: a `Relaxed` "already set?" +/// pre-check would let a thread publish an address without ever acquiring +/// another thread's bit-setting, so a reader synchronising only with this +/// thread could miss it. +pub struct RegistryAddrFilter { + words: [AtomicU64; Self::WORDS], +} + +impl Default for RegistryAddrFilter { + fn default() -> Self { + Self::new() + } +} + +impl std::fmt::Debug for RegistryAddrFilter { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("RegistryAddrFilter") + .field("bits_set", &self.bits_set()) + .finish() + } +} + +impl RegistryAddrFilter { + const WORDS: usize = 16; + const BITS: u64 = (Self::WORDS as u64) * 64; + + /// An empty filter: contains no address at all. + pub const fn new() -> Self { + Self { + words: [const { AtomicU64::new(0) }; Self::WORDS], + } + } + + /// The three bit positions for `addr`. + /// + /// Registered addresses are allocator results, so the low three bits carry + /// no information and are shifted out; the multiply then spreads what is + /// left across the whole word, and the three slices are taken from the top, + /// where a 64-bit multiply mixes best. Deliberately branch-free and + /// division-free — this runs inline at every call site of the probes it + /// guards. + #[inline(always)] + const fn bit_positions(addr: usize) -> (u64, u64, u64) { + let h = ((addr as u64) >> 3).wrapping_mul(0x9E37_79B9_7F4A_7C15); + ( + (h >> 54) % Self::BITS, + (h >> 44) % Self::BITS, + (h >> 34) % Self::BITS, + ) + } + + #[inline(always)] + fn bit_is_set(&self, bit: u64) -> bool { + let word = self.words[(bit / 64) as usize].load(Ordering::Acquire); + word & (1u64 << (bit % 64)) != 0 + } + + /// `false` ⟹ `addr` is definitively absent from every table this filter + /// covers, so the caller can answer "not found" without touching one. + /// + /// This is the hot side, and `inline(always)` for the same reason the + /// window's is: the point is that the common negative answer costs a load + /// and a test at the call site rather than a call into the registry probe. + /// The `&&` chain short-circuits, so most rejections read one word. + #[inline(always)] + pub fn may_contain(&self, addr: usize) -> bool { + let (a, b, c) = Self::bit_positions(addr); + self.bit_is_set(a) && self.bit_is_set(b) && self.bit_is_set(c) + } + + /// Add `addr` to the filter. + /// + /// MUST run **before** the guarded table is mutated — see the type docs. + #[inline] + pub fn admit(&self, addr: usize) { + let (a, b, c) = Self::bit_positions(addr); + for bit in [a, b, c] { + self.words[(bit / 64) as usize].fetch_or(1u64 << (bit % 64), Ordering::AcqRel); + } + } + + /// How many bits are set. Diagnostics and tests only: a filter whose bits + /// are nearly all set has stopped discriminating, and a test that wants to + /// prove the fast path RAN needs to know the filter is not saturated. + pub fn bits_set(&self) -> u32 { + self.words + .iter() + .map(|w| w.load(Ordering::Acquire).count_ones()) + .sum() + } + + /// Test hook: empty the filter, so a test can establish a state in which + /// only what it registers is admitted. + /// + /// Only a test may have this: the soundness argument is that bits are never + /// cleared, so a production clear would make live registered addresses read + /// as unregistered. A test that calls this must restore what it cleared — + /// see [`Self::restore_for_tests`]. + #[cfg(test)] + pub(crate) fn take_for_tests(&self) -> [u64; Self::WORDS] { + let mut previous = [0u64; Self::WORDS]; + for (slot, word) in previous.iter_mut().zip(self.words.iter()) { + *slot = word.swap(0, Ordering::AcqRel); + } + previous + } + + /// Test hook: OR the saved bits back in. + #[cfg(test)] + pub(crate) fn restore_for_tests(&self, saved: [u64; Self::WORDS]) { + for (word, bits) in self.words.iter().zip(saved.iter()) { + word.fetch_or(*bits, Ordering::AcqRel); + } + } + + /// Test hook: the current bit words, as a plain value. + #[cfg(test)] + pub(crate) fn snapshot_for_tests(&self) -> [u64; Self::WORDS] { + let mut words = [0u64; Self::WORDS]; + for (slot, word) in words.iter_mut().zip(self.words.iter()) { + *slot = word.load(Ordering::Acquire); + } + words + } + + /// Test hook: what [`may_contain`](Self::may_contain) WOULD have answered + /// for `addr` against a snapshot taken earlier. + /// + /// This is what lets a test prove a widening was load-bearing rather than + /// lucky: "the address the collector moved this symbol to is not one the + /// filter already happened to accept" is only checkable against the filter + /// as it stood BEFORE the move. + #[cfg(test)] + pub(crate) fn snapshot_may_contain(words: &[u64; Self::WORDS], addr: usize) -> bool { + let (a, b, c) = Self::bit_positions(addr); + [a, b, c] + .into_iter() + .all(|bit| words[(bit / 64) as usize] & (1u64 << (bit % 64)) != 0) + } + + /// The word count, so a caller can name the snapshot type. + #[cfg(test)] + pub(crate) const fn words() -> usize { + Self::WORDS + } +} + #[cfg(test)] mod tests { use super::*; @@ -332,6 +560,102 @@ mod tests { } } + #[test] + fn empty_filter_contains_nothing() { + let f = RegistryAddrFilter::new(); + assert_eq!(f.bits_set(), 0); + for addr in [0usize, 8, 0x1000, 0x7fff_ffff_f000, usize::MAX] { + assert!( + !f.may_contain(addr), + "an empty filter must contain nothing, but accepted {addr:#x}" + ); + } + } + + /// The one direction that must never fail: an admitted address is always + /// accepted afterwards. A Bloom filter is allowed to accept addresses it + /// was never given; it is never allowed to reject one it was. + #[test] + fn filter_never_rejects_an_admitted_address() { + let f = RegistryAddrFilter::new(); + let mut admitted = Vec::new(); + // A realistic spread: 8-byte-aligned addresses across a wide heap. + for i in 0..256usize { + let addr = 0x1_0000_0000usize + i * 0x2a8; + f.admit(addr); + admitted.push(addr); + for &a in &admitted { + assert!( + f.may_contain(a), + "{a:#x} was admitted and must stay accepted after {i} \ + further admissions" + ); + } + } + assert!(f.bits_set() > 0); + } + + /// A filter that accepted everything would satisfy the test above without + /// doing anything, so this is the half that makes it able to fail: with a + /// realistic population the filter must still reject the overwhelming + /// majority of addresses it was never given. + #[test] + fn filter_rejects_almost_everything_it_was_never_given() { + let f = RegistryAddrFilter::new(); + // 160 entries — the number of class prototypes `claude-code --help` + // registers, i.e. the population this size was chosen for. + for i in 0..160usize { + f.admit(0x2_0000_0000usize + i * 0x330); + } + let probes = 20_000usize; + let accepted = (0..probes) + .filter(|i| f.may_contain(0x3_0000_0000usize + i * 8)) + .count(); + assert!( + accepted * 20 < probes, + "at 160 entries the filter must reject far more than 95% of \ + unregistered addresses; it accepted {accepted} of {probes}" + ); + } + + #[test] + fn concurrent_filter_admits_never_drop_an_address() { + static FILTER: RegistryAddrFilter = RegistryAddrFilter::new(); + const PER_THREAD: usize = 512; + let handles: Vec<_> = (0..4usize) + .map(|t| { + std::thread::spawn(move || { + for i in 0..PER_THREAD { + FILTER.admit(0x5_0000_0000usize + (t * PER_THREAD + i) * 8); + } + }) + }) + .collect(); + for h in handles { + h.join().unwrap(); + } + for t in 0..4usize { + for i in 0..PER_THREAD { + let addr = 0x5_0000_0000usize + (t * PER_THREAD + i) * 8; + assert!( + FILTER.may_contain(addr), + "{addr:#x} was admitted by thread {t} and must be visible \ + here — a lost `fetch_or` is a misclassified pointer" + ); + } + } + } + + #[test] + fn filter_admit_is_visible_to_another_thread() { + static FILTER: RegistryAddrFilter = RegistryAddrFilter::new(); + assert!(!FILTER.may_contain(0x9_0000_1000)); + std::thread::spawn(|| FILTER.admit(0x9_0000_1000)) + .join() + .unwrap(); + assert!(FILTER.may_contain(0x9_0000_1000)); + } + #[test] fn window_admit_is_visible_to_another_thread() { static WINDOW: RegistryAddrWindow = RegistryAddrWindow::new(); diff --git a/crates/perry-runtime/src/registry_latch_probes.rs b/crates/perry-runtime/src/registry_latch_probes.rs index bdf94772fa..fb74e2a760 100644 --- a/crates/perry-runtime/src/registry_latch_probes.rs +++ b/crates/perry-runtime/src/registry_latch_probes.rs @@ -49,6 +49,7 @@ fn unregistered_address_misses_every_probe() { assert!(!crate::regex::is_registered_regex(addr)); assert!(!crate::map::is_registered_map(addr)); assert!(!crate::set::is_registered_set(addr)); + assert!(!crate::object::is_registered_class_prototype_object(addr)); } /// #7474-shape regression: constructing a typed array AFTER the idle fast path @@ -291,6 +292,115 @@ fn typed_array_probe_rejects_an_out_of_window_address_without_touching_the_regis assert!(crate::typedarray::test_typed_array_window_admitted_probe_count() > before); } +/// The Uint8Array-mark window, on the same terms as the buffer one above: it +/// must be shown to RUN, not merely to answer correctly, and it must not hide a +/// marked backing. `typed_array_owner_kind` asks this question on every untyped +/// element access, so the rejection is the common case by a wide margin. +#[test] +fn uint8array_probe_rejects_an_out_of_window_address_without_touching_the_registries() { + let first = crate::buffer::buffer_alloc(32) as usize; + crate::buffer::mark_as_uint8array(first); + let second = crate::buffer::buffer_alloc(32) as usize; + crate::buffer::mark_as_uint8array(second); + let (lo, hi) = crate::buffer::test_uint8array_addr_window_bounds() + .expect("marking a Uint8Array must open the address window"); + assert!( + lo <= first.min(second) && hi >= first.max(second), + "the window must cover every marked backing: \ + [{lo:#x}, {hi:#x}] vs {first:#x} / {second:#x}" + ); + + let before = crate::buffer::test_uint8array_registry_probe_count(); + assert!( + !crate::buffer::is_uint8array_buffer(FAR_OUTSIDE_ANY_WINDOW), + "an address outside the window is not a marked Uint8Array backing" + ); + assert_eq!( + crate::buffer::test_uint8array_registry_probe_count(), + before, + "the address window must answer without reaching the registries" + ); + + assert!( + crate::buffer::is_uint8array_buffer(first), + "the window must not hide a marked Uint8Array backing" + ); + assert!( + crate::buffer::test_uint8array_registry_probe_count() > before, + "an in-window address must reach the registries" + ); +} + +/// The symbol address FILTER. `is_registered_symbol` is asked about arbitrary +/// pointer-shaped values on the generic property, coercion and iteration paths, +/// and the answer is essentially always "no" — but a symbol the collector has +/// EVACUATED must keep answering "yes" from its new address, which is what the +/// forwarding rewrite's admission is for (see +/// `gc::tests::copying_side_tables::test_copying_minor_keeps_moved_symbol_visible_to_the_range_filter`). +/// +/// A Bloom filter has false positives, so "the filter rejected this particular +/// address" is not by itself a proof that it can reject: the probe counter is, +/// and the second half — an address the filter must ADMIT — is what makes the +/// first half able to fail. +#[test] +fn symbol_probe_rejects_a_filtered_address_without_touching_the_registry() { + let sym = unsafe { crate::symbol::alloc_symbol(std::ptr::null_mut(), false) } as usize; + assert!(sym != 0, "test premise: the symbol allocated"); + + let before = crate::symbol::test_symbol_filter_admitted_probe_count(); + assert!(!crate::symbol::is_registered_symbol(FAR_OUTSIDE_ANY_WINDOW)); + assert_eq!( + crate::symbol::test_symbol_filter_admitted_probe_count(), + before, + "the address filter must answer without reaching SYMBOL_POINTERS" + ); + + assert!( + crate::symbol::is_registered_symbol(sym), + "the filter must not hide a registered symbol" + ); + assert!( + crate::symbol::test_symbol_filter_admitted_probe_count() > before, + "a filter-admitted address must reach SYMBOL_POINTERS" + ); +} + +/// The class-prototype address filter. The probe behind it is a LINEAR SCAN +/// (#9225) reached through a thread-local and an `RwLock`, and its one caller — +/// `descriptor_state::disable_inline_guards_for_descriptor_target` — runs on +/// every `Object.defineProperty`, so the rejection is what keeps a bundle's +/// `__export(exports, { … })` init off the scan entirely. +#[test] +fn class_prototype_probe_rejects_a_filtered_address_without_scanning() { + use crate::object as class_registry; + + // A registered prototype, seeded through the real store so the filter is + // admitted exactly as production admits it. + let proto = crate::object::js_object_alloc(0, 2) as usize; + assert!(proto != 0, "test premise: the prototype object allocated"); + class_registry::test_seed_class_prototype_object_root(0x7f00_0001, proto); + + let before = class_registry::test_class_prototype_scan_count(); + assert!( + !class_registry::is_registered_class_prototype_object(FAR_OUTSIDE_ANY_WINDOW), + "an address no registration admitted is not a class prototype" + ); + assert_eq!( + class_registry::test_class_prototype_scan_count(), + before, + "the address filter must answer without reaching the scan" + ); + + assert!( + class_registry::is_registered_class_prototype_object(proto), + "the filter must not hide a registered class prototype" + ); + assert!( + class_registry::test_class_prototype_scan_count() > before, + "a filter-admitted address must reach the scan" + ); +} + /// `alloc_shared_sab` publishes a backing that `is_registered_buffer` reports /// as a buffer without it ever entering `BUFFER_REGISTRY`, so the window has to /// be widened on that route too. Calling the allocator directly (rather than diff --git a/crates/perry-runtime/src/symbol.rs b/crates/perry-runtime/src/symbol.rs index ed7e11061c..087ca886d2 100644 --- a/crates/perry-runtime/src/symbol.rs +++ b/crates/perry-runtime/src/symbol.rs @@ -89,7 +89,7 @@ use crate::fast_hash::{ use crate::string::StringHeader; use std::cell::RefCell; use std::collections::{HashMap, HashSet}; -use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering}; +use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::Mutex; // NaN-boxing tags (must match value.rs) @@ -448,7 +448,7 @@ pub(crate) fn test_disable_symbol_magic_screen(disabled: bool) -> bool { TEST_DISABLE_SYMBOL_MAGIC_SCREEN.with(|c| c.replace(disabled)) } -/// Smallest and largest pointer ever registered as a Symbol, as a conservative +/// Which pointers have ever been registered as a Symbol — a conservative /// filter in front of the process-global registry mutex. /// /// `SYMBOL_EVER_REGISTERED` answers "has any symbol EVER been registered?", @@ -457,21 +457,43 @@ pub(crate) fn test_disable_symbol_magic_screen(disabled: bool) -> bool { /// `is_registered_symbol_slow` took the global mutex on EVERY probe, including /// the overwhelming majority asking about pointers that are not symbols at all. /// -/// The range only ever widens, and registration extends it before taking the +/// The filter only ever gains bits, and registration admits before taking the /// lock — the same ordering, and for the same reason, as the latch arm above -/// it: a pointer outside the range cannot be in the set, so rejecting is sound, -/// while accepting merely falls through to the lookup that was already there. -/// Death pruning removes entries without narrowing the range, which is +/// it: a pointer the filter rejects cannot be in the set, so rejecting is +/// sound, while accepting merely falls through to the lookup that was already +/// there. Death pruning removes entries without clearing bits, which is /// harmless: those pointers reach the lookup, which correctly says no. -static SYMBOL_ADDR_MIN: AtomicUsize = AtomicUsize::new(usize::MAX); -static SYMBOL_ADDR_MAX: AtomicUsize = AtomicUsize::new(0); - -/// Widen the address range to admit `ptr`. +/// +/// # Why this is a filter and not the `[lo, hi]` range #9177 shipped +/// +/// #9177 put a monotone address RANGE in front of the registry mutex, inside +/// `is_registered_symbol_slow`. Symbols are `gc_malloc`'d, so they are spread +/// through the object heap rather than clustered: replaying this probe's real +/// argument stream from a `claude-code --help` run against the range those +/// registrations build, **only 38.3% of 378,163 calls fall outside it** — the +/// other 61.7% were admitted and paid the process-global mutex and the hash +/// anyway. That is where the probe's 0.65% of the run lives. +/// +/// A Bloom filter over the same addresses rejects **99.58%** of them (measured +/// the same way, by simulating the filter bit-for-bit against the real address +/// stream: 376,555 of 378,163, with all 622 genuine `true` answers preserved +/// and 985 false positives). It also inlines: the range check sat behind an +/// out-of-line call and a `OnceLock` load for an env kill switch, and the +/// filter replaces both with a load and a test at the call site. +/// +/// The env kill switch went with it. It guarded an enumeration ("every +/// inserter widens"), and this file no longer relies on one: `debug_assertions` +/// builds re-derive every rejection from `SYMBOL_POINTERS` itself, so an +/// inserter added without admitting panics in the first test that touches it. +static SYMBOL_ADDR_FILTER: crate::registry_latch::RegistryAddrFilter = + crate::registry_latch::RegistryAddrFilter::new(); + +/// Admit `ptr` into [`SYMBOL_ADDR_FILTER`]. /// /// EVERY path that puts a pointer into `SYMBOL_POINTERS` must call this -/// first, not just the registration one. `is_registered_symbol_slow` rejects -/// an out-of-range pointer WITHOUT consulting the set, so a member outside -/// the range is a live symbol the probe reports as "not a symbol". +/// first, not just the registration one. `is_registered_symbol` rejects a +/// filtered-out pointer WITHOUT consulting the set, so a member the filter +/// does not hold is a live symbol the probe reports as "not a symbol". /// /// That is not a theoretical second inserter: `SYMBOL_POINTERS` is a GC root /// registry, and `rewrite_symbol_pointer_metadata_if_forwarded` re-keys an @@ -479,67 +501,78 @@ static SYMBOL_ADDR_MAX: AtomicUsize = AtomicUsize::new(0); /// symbol evacuated out of the range established by its own allocation would /// otherwise stop answering to `typeof`, symbol-keyed property lookup and /// `Symbol.iterator` dispatch — while still being perfectly alive. -pub(crate) fn widen_symbol_addr_range(ptr: usize) { - SYMBOL_ADDR_MIN.fetch_min(ptr, Ordering::Release); - SYMBOL_ADDR_MAX.fetch_max(ptr, Ordering::Release); +pub(crate) fn admit_symbol_pointer(ptr: usize) { + SYMBOL_ADDR_FILTER.admit(ptr); } -/// The ONLY way to put a pointer into `SYMBOL_POINTERS`. Widening and +/// The ONLY way to put a pointer into `SYMBOL_POINTERS`. Admitting and /// inserting are one operation on purpose: there are three insert sites (the /// registration, and two forwarding rewrites — a per-slot one and the bulk -/// one the copying minor actually drives), and a range filter is only sound -/// while every one of them widens. +/// one the copying minor actually drives), and the filter is only sound while +/// every one of them admits. pub(crate) fn insert_symbol_pointer_in_set(set: &mut PtrHashSet, ptr: usize) { - widen_symbol_addr_range(ptr); + admit_symbol_pointer(ptr); set.insert(ptr); } -/// Save/restore the range filter's bounds around a test that needs to observe -/// a NARROW range. The bounds are process-global and only ever widen in -/// production, so a test that resets them must put back at least what it -/// found — otherwise a later test in the same binary gets a range too narrow -/// for symbols registered before it ran, and fails for no reason of its own. +/// Width of the symbol filter's bit array, for tests that snapshot it. +#[cfg(test)] +pub(crate) const SYMBOL_FILTER_WORDS: usize = crate::registry_latch::RegistryAddrFilter::words(); + +/// Save/restore the address filter around a test that needs to observe a +/// NEARLY-EMPTY filter. It is process-global and only ever gains bits in +/// production, so a test that clears it must put back at least what it found — +/// otherwise a later test in the same binary gets a filter that no longer holds +/// symbols registered before it ran, and fails for no reason of its own. #[cfg(test)] -pub(crate) struct SymbolAddrRangeGuard(usize, usize); +pub(crate) struct SymbolAddrRangeGuard([u64; SYMBOL_FILTER_WORDS]); #[cfg(test)] impl SymbolAddrRangeGuard { - /// Reset to the empty range, so only what the test registers is admitted. + /// Empty the filter, so only what the test registers is admitted. pub(crate) fn reset() -> Self { - let g = SymbolAddrRangeGuard( - SYMBOL_ADDR_MIN.load(Ordering::Acquire), - SYMBOL_ADDR_MAX.load(Ordering::Acquire), - ); - SYMBOL_ADDR_MIN.store(usize::MAX, Ordering::Release); - SYMBOL_ADDR_MAX.store(0, Ordering::Release); - g + SymbolAddrRangeGuard(SYMBOL_ADDR_FILTER.take_for_tests()) } } #[cfg(test)] impl Drop for SymbolAddrRangeGuard { fn drop(&mut self) { - // Union of what we saved and what the test widened to, so neither the - // pre-existing members nor the test's own survive outside the range. - SYMBOL_ADDR_MIN.fetch_min(self.0, Ordering::Release); - SYMBOL_ADDR_MAX.fetch_max(self.1, Ordering::Release); + // Union of what we saved and what the test admitted, so neither the + // pre-existing members nor the test's own fall out of the filter. + SYMBOL_ADDR_FILTER.restore_for_tests(self.0); } } #[cfg(test)] -pub(crate) fn test_symbol_addr_range() -> (usize, usize) { - ( - SYMBOL_ADDR_MIN.load(Ordering::Acquire), - SYMBOL_ADDR_MAX.load(Ordering::Acquire), - ) +pub(crate) fn test_symbol_filter_snapshot() -> [u64; SYMBOL_FILTER_WORDS] { + SYMBOL_ADDR_FILTER.snapshot_for_tests() +} + +/// What the filter WOULD have answered for `ptr` against a snapshot taken +/// earlier. A test that moves a symbol uses this to show the forwarding +/// rewrite's admission was load-bearing rather than a lucky collision. +#[cfg(test)] +pub(crate) fn test_symbol_filter_snapshot_may_contain( + words: &[u64; SYMBOL_FILTER_WORDS], + ptr: usize, +) -> bool { + crate::registry_latch::RegistryAddrFilter::snapshot_may_contain(words, ptr) +} + +/// How many bits the symbol filter has set — the discrimination it still has +/// left. Tests use it to assert a fixture starts from a nearly-empty filter. +#[cfg(test)] +pub(crate) fn test_symbol_filter_bits_set() -> u32 { + SYMBOL_ADDR_FILTER.bits_set() } pub(crate) fn register_symbol_pointer(ptr: usize) { // Arm before taking the lock, so the entry is never reachable while the // latch still reads idle. SYMBOL_EVER_REGISTERED.arm(); - // Widen before the insert, for the same reason. - widen_symbol_addr_range(ptr); + // Admit before the insert, for the same reason. + admit_symbol_pointer(ptr); let mut guard = crate::gc::lock_gc_root_registry(&SYMBOL_POINTERS); if guard.is_none() { *guard = Some(new_ptr_hash_set()); @@ -562,6 +595,22 @@ pub(crate) fn test_symbol_registry_probe_count() -> u64 { TEST_SYMBOL_REGISTRY_PROBES.with(|c| c.get()) } +#[cfg(test)] +thread_local! { +/// Every probe that got past `SYMBOL_ADDR_FILTER` and reached +/// `SYMBOL_POINTERS`. `TEST_SYMBOL_REGISTRY_PROBES` counts ENTRIES into +/// `is_registered_symbol` and so cannot see the filter working; this counts the +/// calls the filter was added to remove. (Twin of +/// `typedarray::TEST_TA_WINDOW_ADMITTED_PROBES`, for the same reason.) + static TEST_SYMBOL_FILTER_ADMITTED_PROBES: std::cell::Cell = + const { std::cell::Cell::new(0) }; +} + +#[cfg(test)] +pub(crate) fn test_symbol_filter_admitted_probe_count() -> u64 { + TEST_SYMBOL_FILTER_ADMITTED_PROBES.with(|c| c.get()) +} + #[cfg(test)] pub(crate) fn test_symbol_latch_is_idle() -> bool { SYMBOL_EVER_REGISTERED.is_idle() @@ -610,35 +659,62 @@ pub fn is_registered_symbol(ptr: usize) -> bool { if SYMBOL_EVER_REGISTERED.is_idle() { return false; } + // Counted BEFORE the filter on purpose: this counter's contract is "an + // entry that got past the latch", and two other suites + // (`get_field_by_name_probe_tests`, `native_call_method::probe_dispatch_tests`) + // assert against exactly that — including sabotage checks that DEFEAT a + // cheaper screen upstream and require this to move. Counting filter + // admissions is a different question; `TEST_SYMBOL_FILTER_ADMITTED_PROBES` + // below answers it. #[cfg(test)] TEST_SYMBOL_REGISTRY_PROBES.with(|c| c.set(c.get().wrapping_add(1))); + // Armed says only that SOME symbol exists — which is true of every program + // that touches a well-known symbol, i.e. essentially all of them. An + // address the filter rejects is not one of them, and rejecting it here + // keeps the negative answer inline: no call, no mutex, no hash. This is + // 99.58% of the calls on `claude-code --help`. See `SYMBOL_ADDR_FILTER`. + if !SYMBOL_ADDR_FILTER.may_contain(ptr) { + // Machine-check the writer set rather than enumerate it. Every route + // into `SYMBOL_POINTERS` goes through `insert_symbol_pointer_in_set`, + // which widens first — but that is a property of today's code, and a + // route added without it would not crash: it would silently report a + // live symbol as "not a symbol", so `typeof`, symbol-keyed lookup and + // `Symbol.iterator` dispatch would quietly stop working for it. In a + // debug build every rejection is therefore re-derived from the + // authoritative set, which turns that into a panic in the first test + // that exercises the route. Compiled out entirely in release. + #[cfg(debug_assertions)] + { + // `try_lock`, not `lock`. The rejection path never took this mutex + // before, so a blocking audit would introduce a deadlock that the + // code it audits cannot have: a caller that probes an unregistered + // address while holding `SYMBOL_POINTERS` is fine today and would + // hang here. A `try_lock` that loses the race simply does not audit + // that one rejection, and the suite performs millions of them. + let present = SYMBOL_POINTERS + .try_lock() + .ok() + .is_some_and(|g| g.as_ref().is_some_and(|s| s.contains(&ptr))); + assert!( + !present, + "SYMBOL_ADDR_FILTER rejected {ptr:#x}, but it IS in \ + SYMBOL_POINTERS. Some route reached the set without going \ + through `insert_symbol_pointer_in_set` (which admits into the \ + filter) first." + ); + } + return false; + } + #[cfg(test)] + TEST_SYMBOL_FILTER_ADMITTED_PROBES.with(|c| c.set(c.get().wrapping_add(1))); is_registered_symbol_slow(ptr) } -/// `PERRY_SYMBOL_RANGE_FILTER=0` restores the unconditional mutex acquisition. -fn symbol_range_filter_enabled() -> bool { - use std::sync::OnceLock; - static CACHED: OnceLock = OnceLock::new(); - *CACHED.get_or_init(|| { - !matches!( - std::env::var("PERRY_SYMBOL_RANGE_FILTER").as_deref(), - Ok("0") | Ok("off") | Ok("false") - ) - }) -} - #[inline(never)] fn is_registered_symbol_slow(ptr: usize) -> bool { if ptr < 0x10000 { return false; } - // Outside the registered range ⟹ not a symbol, without the global mutex. - if symbol_range_filter_enabled() - && (ptr < SYMBOL_ADDR_MIN.load(Ordering::Acquire) - || ptr > SYMBOL_ADDR_MAX.load(Ordering::Acquire)) - { - return false; - } let guard = SYMBOL_POINTERS.lock().unwrap(); guard.as_ref().is_some_and(|s| s.contains(&ptr)) }