diff --git a/changelog.d/registry-probe-address-window.md b/changelog.d/registry-probe-address-window.md new file mode 100644 index 0000000000..09dac486c6 --- /dev/null +++ b/changelog.d/registry-probe-address-window.md @@ -0,0 +1,119 @@ +### Performance + +- **The two hottest side-table probes answer "no" from an inlined address + compare instead of an out-of-line registry lookup, and `is_closure_ptr` tests + the tag before the arena.** + + `is_registered_buffer` and `lookup_typed_array_kind` are asked "is this + pointer special?" from ~239 and ~200 generic call sites — every property + get/set, every prototype walk, every element read, every `[[HasProperty]]`. + #9176 gave both a monotone "has anything ever been registered?" latch, which + makes the answer free for a program that never allocates a `Buffer` or a typed + array. `claude-code` allocates both, so the latch is armed and stops + discriminating. + + How badly it stops discriminating was the finding. Measured with uprobes and + uretprobes on a symbolized `claude --help`, exact counts from one run — not + inferred from the profile: + + | probe | registrations | calls | answered "yes" | + |---|---|---|---| + | `is_registered_buffer_slow` | 10 | 4,650,058 | **4** | + | `lookup_registered_typed_array_kind` | 42 | 3,566,956 | **0** | + + 1.16 million probes per "yes" for buffers, and not one "yes" in three and a + half million for typed arrays — every one of them going out of line to a + thread-local resolution, a `RefCell` borrow and a hash. The typed-array probe + additionally consults a direct-mapped negative cache whose cold miss *writes + back*, dirtying a shared cache line to record an answer nothing asked twice. + + `RegistryAddrWindow` is the same monotone idea applied to the address rather + than to the fact of registration: a process-global `[lo, hi]` that every + registration widens *before* it publishes, checked inline at the call site. An + address outside it cannot be in any table the window covers, so rejecting is + sound; accepting falls through to the exact lookup that was already there. It + is strictly stronger than a latch — an unregistered process has the empty + window `[usize::MAX, 0]`, which contains nothing. + + It removes 98.0% of the buffer probe's calls and 97.2% of the typed-array + probe's (4,650,058 → 92,965 and 3,566,956 → 100,926, measured the same way). + + It is deliberately **not** a `GcHeader` tag test. A registered typed array is + not required to have a readable `ptr - GC_HEADER_SIZE` (see the `mprotect`ed + guard-page fixture in `promise::combinators`), and `native_arena`'s + `native_memory_copy_rejects_buffer_registry_forged_to_old_non_buffer` pins + that a registry entry may legitimately disagree with the header type. The + window never dereferences the candidate, so neither case can be misclassified. + + `buffer::header` already had this filter as the thread-local + `BUFFER_ADDR_RANGE` — but *behind* the call, where it still paid the call, the + prologue, two thread-local resolutions and a tail call into `is_shared_sab` + for every rejection. The window is the same test hoisted in front of the call + and widened to cover the external and `SharedArrayBuffer` registries too, so + those routes keep their existing behaviour. + + `is_closure_ptr` consults no registry at all; its cost was ordering. It is a + conjunction of a handle-band check, a heap-floor check, an alignment check, + arena ownership plus a GC-header read, and an exact `CLOSURE_MAGIC` tag — and + the tag ran last. Measured over 2,240,934 calls, the tag alone partitions them + 231,704 / 2,009,230, which is bit for bit the partition the whole function + produces: in that run it decided every answer, while `classify_heap_generation` + and the header read ran on 100% of calls to change none of them. The tag now + runs first. Arena ownership still runs, below, where it does its actual job of + refusing a coincidental "CLOS" left in recycled arena storage — pinned by + `managed_error_with_closure_magic_in_padding_is_not_a_closure`, which writes + the magic into an `ErrorHeader`'s padding and demands `false`. + + Measured on `claude-code` (`cli_2.1.112.js`, `--help`), both arms built from + `b3f14e9cde` in one session on the same host, `PERRY_DEBUG_SYMBOLS=1`, + 11 interleaved reps: + + | | instructions (min / median) | cycles (min / median) | + |---|---|---| + | before | 7,160,271,524 / 7,163,283,415 | 3,446,625,309 / 3,479,536,202 | + | after | 6,791,077,579 / 6,793,829,953 | 3,229,414,851 / 3,267,521,023 | + | | **−5.16% / −5.16%** | **−6.30% / −6.09%** | + + Cycles fall by *more* than instructions, and IPC rises (2.077 → 2.102), so + none of this is work that was riding free in superscalar slack — the check + that a previous change in this campaign failed, having removed 24% of its + instructions to move cycles by +1%. The probe family's share of a symbolized + profile goes 6.86% → 2.65%: `is_registered_buffer_slow` 2.33% → 0.21%, + `lookup_registered_typed_array_kind` 1.30% → 0.11%, `is_closure_ptr` + 1.23% → 0.42%. + + Output stays byte-identical to `node cli_2.1.112.js --help` (9,175 bytes, + rc=0) on both arms; every number above is gated on that. Re-measuring the + answer distribution on the shipped binary confirms the window kept all four + of the run's genuine "yes" answers while removing 98.03% of the calls. + + The same shape still fits four more probes that this change does not touch, + now the largest remaining members of the family: `is_registered_symbol_slow` + (0.60%), `is_registered_class_prototype_object` (0.47%), `is_registered_box_ptr` + (0.33%) and `is_uint8array_buffer_slow` (0.21%). + +### Fixed + +- **`RegistryAddrWindow::admit` cannot drop a registration.** It is two + unconditional `AcqRel` `fetch_min`/`fetch_max` calls, with no "already + covered?" pre-check, because two earlier drafts of that one function were + wrong in two different ways and a dropped registration here is a misclassified + pointer, not a slow path. + + A `load` then `store` is a read-modify-write with a hole in it: two threads + registering at once both read the old bound and the *narrower* of the two + stores can land last, evicting the other thread's live registration from the + window. A `Relaxed` "skip if already covered" pre-check is the same bug moved + up into the memory model: a thread that skips the RMW because it *observed* + another thread's widening performs no acquire, so that widening never joins + its happens-before graph, and a reader synchronising with only this thread's + subsequent publish is not guaranteed to see the bound that covers the address. + Registration runs 52 times across a 6.9-billion-instruction `claude --help`, + so neither fast path bought anything measurable. + + Both probes now re-derive every window rejection from the authoritative tables + under `debug_assertions`. The window is sound only if *every* route into the + guarded tables admits first; an enumeration of those routes is a snapshot a + later commit can invalidate in silence, so the enumeration is machine-checked + instead — a registration route added without `admit` panics in the first test + that exercises it. Compiled out entirely in release. diff --git a/crates/perry-runtime/src/buffer/header.rs b/crates/perry-runtime/src/buffer/header.rs index 32612f5424..8ab8823385 100644 --- a/crates/perry-runtime/src/buffer/header.rs +++ b/crates/perry-runtime/src/buffer/header.rs @@ -173,7 +173,7 @@ crate::perry_thread_local! { RefCell::new(new_ptr_hash_map()); } -use crate::registry_latch::RegistryLatch; +use crate::registry_latch::{RegistryAddrWindow, RegistryLatch}; /// Monotone "at least one `Buffer`-shaped allocation exists" latch. /// @@ -191,6 +191,55 @@ use crate::registry_latch::RegistryLatch; /// load rather than one per registry — hence [`note_buffer_like_registered`], /// which `shared_sab::alloc_shared_sab` calls before publishing a backing. static BUFFER_LIKE_EVER_REGISTERED: RegistryLatch = RegistryLatch::new(); + +/// Smallest and largest address ever registered as buffer-like, process-wide. +/// +/// The latch above answers "has ANY buffer ever been registered?", which +/// `claude-code --help` arms with one of its **10** buffer allocations and then +/// consults 4,650,058 times — every one of them going out of line to a +/// thread-local resolution, a `RefCell` borrow and a hash, to answer "no" +/// 4,650,054 times out of 4,650,058 (uretprobe count, one run). This window +/// answers the same question about the *address*, from two adjacent static +/// loads that inline into all ~239 call sites, and removes 98.0% of those +/// calls — 4,650,058 down to 92,965, with all four genuine "yes" answers +/// preserved. +/// +/// It covers every table `is_registered_buffer_slow` consults: +/// * `BUFFER_REGISTRY` — only `register_buffer` inserts, and it admits first; +/// * `EXTERNAL_BUFFER_REGISTRY` — both writers (`js_buffer_register_external` +/// and `js_buffer_mark_as_crypto_key_external`) route through +/// `register_buffer` with the same address first; +/// * `shared_sab`'s process-global SAB registry — `alloc_shared_sab` calls +/// [`note_buffer_like_registered`] with the backing address before it +/// publishes. +/// +/// Rejecting an address outside the window is therefore sound; see +/// [`RegistryAddrWindow`] for the ordering rule that makes it so. +static BUFFER_LIKE_ADDR_WINDOW: RegistryAddrWindow = RegistryAddrWindow::new(); + +#[cfg(test)] +thread_local! { +/// Test-only count of `is_registered_buffer` calls that got past the address +/// window and reached the registries. The window is a fast path, and a fast +/// path nobody can prove ran is not a fast path (same contract as +/// `typedarray::TEST_TA_REGISTRY_PROBES`, #7765). +/// +/// Per THREAD, not per process, exactly like `TEST_TA_REGISTRY_PROBES`: the +/// registry it guards is thread-local and `cargo test` gives each case its own +/// thread inside one process. + static TEST_BUFFER_REGISTRY_PROBES: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +#[cfg(test)] +pub(crate) fn test_buffer_registry_probe_count() -> u64 { + TEST_BUFFER_REGISTRY_PROBES.with(|c| c.get()) +} + +#[cfg(test)] +pub(crate) fn test_buffer_addr_window_bounds() -> Option<(usize, usize)> { + BUFFER_LIKE_ADDR_WINDOW.bounds_for_tests() +} + /// Avoid a thread-local map probe in `buffer_data{,_mut}` until the first /// foreign-backed buffer is created. The latch is deliberately monotone; /// these accessors are among the hottest paths in the runtime. @@ -202,7 +251,11 @@ static FOREIGN_BACKING_EVER_REGISTERED: RegistryLatch = RegistryLatch::new(); /// reports as buffers without them ever entering `BUFFER_REGISTRY`, so it must /// arm the same latch — and, per the [`crate::registry_latch`] rule, must do so /// *before* the backing becomes reachable. -pub(crate) fn note_buffer_like_registered() { +pub(crate) fn note_buffer_like_registered(addr: usize) { + // Widen before arming, and arm before the caller publishes: the probe + // checks the latch and then the window, so both must already cover this + // address by the time it becomes findable. + BUFFER_LIKE_ADDR_WINDOW.admit(addr); BUFFER_LIKE_EVER_REGISTERED.arm(); } @@ -304,8 +357,9 @@ pub fn register_buffer(ptr: *const BufferHeader) { // Arm BEFORE the insert: an arm placed afterwards leaves a window in which // this buffer is in the registry while `is_registered_buffer` still takes // the idle fast path and denies it. See `crate::registry_latch`. - BUFFER_LIKE_EVER_REGISTERED.arm(); let addr = ptr as usize; + BUFFER_LIKE_ADDR_WINDOW.admit(addr); + BUFFER_LIKE_EVER_REGISTERED.arm(); BUFFER_ADDR_RANGE.with(|r| { let (lo, hi) = r.get(); r.set((lo.min(addr), hi.max(addr))); @@ -337,6 +391,37 @@ pub fn is_registered_buffer(addr: usize) -> bool { if BUFFER_LIKE_EVER_REGISTERED.is_idle() { return false; } + // An address outside the registered window cannot be in any of the three + // tables the slow path consults, so reject it here — inline, without the + // call, the thread-local resolution, the `RefCell` borrow or the hash. + // Every writer widens the window before it publishes, which is what makes + // rejecting sound; see `BUFFER_LIKE_ADDR_WINDOW`. + if !BUFFER_LIKE_ADDR_WINDOW.may_contain(addr) { + // Machine-check the completeness of the writer set instead of trusting + // an enumeration of it. The window is only sound if EVERY route into + // the three tables below calls `admit` first; an enumeration of those + // routes is a snapshot that a later commit can invalidate silently, and + // the failure it would cause is a misclassified pointer, not a slow + // path. In debug builds every rejection is therefore re-derived from + // the authoritative tables, which turns "someone added a registration + // route without admitting" into a panic in the first test that + // exercises that route. Compiled out entirely in release. + #[cfg(debug_assertions)] + { + assert!( + !is_registered_buffer_slow(addr), + "BUFFER_LIKE_ADDR_WINDOW rejected {addr:#x}, but it IS a \ + registered buffer. Some registration route reached \ + BUFFER_REGISTRY, the external-buffer registry or the \ + shared-SAB registry without calling \ + `BUFFER_LIKE_ADDR_WINDOW.admit()` (via `register_buffer` or \ + `note_buffer_like_registered`) first." + ); + } + return false; + } + #[cfg(test)] + TEST_BUFFER_REGISTRY_PROBES.with(|c| c.set(c.get().wrapping_add(1))); is_registered_buffer_slow(addr) } diff --git a/crates/perry-runtime/src/buffer/mod.rs b/crates/perry-runtime/src/buffer/mod.rs index 3a737aba62..fc7b1477ee 100644 --- a/crates/perry-runtime/src/buffer/mod.rs +++ b/crates/perry-runtime/src/buffer/mod.rs @@ -63,7 +63,10 @@ pub(crate) use header::{ finalize_collected_dead_buffer, is_foreign_backed_buffer, }; #[cfg(test)] -pub(crate) use header::{test_data_view_registry_len, test_shared_array_buffer_registry_len}; +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, +}; // ---- 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/closure/dynamic_props.rs b/crates/perry-runtime/src/closure/dynamic_props.rs index 98959acb29..40292d97a8 100644 --- a/crates/perry-runtime/src/closure/dynamic_props.rs +++ b/crates/perry-runtime/src/closure/dynamic_props.rs @@ -505,6 +505,27 @@ pub fn is_closure_ptr(ptr: usize) -> bool { if !ptr.is_multiple_of(std::mem::align_of::()) { return false; } + // Read the type tag BEFORE consulting the arena. The answer is the same + // conjunction either way — arena ownership AND the exact magic — but this + // is the selective, cheap term and it used to run last. + // + // Measured on `claude-code --help` (uretprobe + a tag read at the uprobe, + // 2,240,934 calls): the tag test partitions the calls 231,704 true / + // 2,009,230 false, which is bit for bit the partition the whole function + // produces. In that entire run it alone decided every answer, while + // `classify_heap_generation` and the GC-header read ran on 100% of calls to + // change none of them. + // + // The load is safe exactly where it is now, because the three checks above + // are the only guard it has ever had: the `Unknown` arm below performed + // this same read with nothing else in front of it, and `Unknown` means "in + // no arena this process knows about" — the LEAST known case, not the most. + // Arena ownership was never what made the load safe; it is what + // disambiguates a coincidental "CLOS", which is why it stays below. + let type_tag = unsafe { *((ptr as *const u8).add(CLOSURE_TYPE_TAG_OFFSET) as *const u32) }; + if type_tag != CLOSURE_MAGIC { + return false; + } // Arena ownership gives us an authoritative discriminator. Do not let a // coincidental CLOSURE_MAGIC in another managed cell's payload win: in // particular, ErrorHeader has padding at the closure tag offset and an @@ -523,10 +544,7 @@ pub fn is_closure_ptr(ptr: usize) -> bool { return false; } } - unsafe { - let type_tag = *((ptr as *const u8).add(CLOSURE_TYPE_TAG_OFFSET) as *const u32); - type_tag == CLOSURE_MAGIC - } + true } /// C-ABI predicate: returns 1 when `value_bits` (a NaN-boxed JSValue passed as diff --git a/crates/perry-runtime/src/registry_latch.rs b/crates/perry-runtime/src/registry_latch.rs index 5f8f6a2e6d..68e1385d2b 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, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; /// A one-way "this feature has been registered at least once" flag. /// @@ -112,6 +112,128 @@ impl RegistryLatch { } } +/// A monotone "smallest and largest address ever registered" window. +/// +/// [`RegistryLatch`] answers "has this feature EVER been used?". That question +/// stops discriminating the moment a program registers its first entry — and +/// for the two hottest probes in the runtime it stops discriminating almost +/// immediately: a `claude-code --help` run registers **10** buffers and **42** +/// typed arrays, then probes `is_registered_buffer` 4,650,058 times and +/// `lookup_typed_array_kind` 3,566,956 times. Counted with uretprobes on that +/// binary, the buffer probe answered `true` **4** times and the typed-array +/// probe answered `Some` **zero** times. The latch was armed for every one of +/// those calls, so all 8.2 M paid the out-of-line call, a thread-local +/// resolution, a `RefCell` borrow and a hash to say "no". +/// +/// This window is the same monotone idea applied to the address instead of to +/// the fact of registration: every registration widens `[lo, hi]` *before* it +/// publishes, so an address outside the window cannot be in any table the +/// window covers. Rejecting is therefore sound; accepting merely falls through +/// to the exact lookup that was already there. +/// +/// It is strictly stronger than a latch — an unregistered process has the empty +/// window `[usize::MAX, 0]`, which contains nothing — and it costs two adjacent +/// static loads and two compares, which inline into the probe's call sites +/// instead of being paid behind a call. +/// +/// # The ordering rule (binding, and identical to [`RegistryLatch`]'s) +/// +/// **[`admit`](Self::admit) must run BEFORE the registry mutation it +/// advertises.** Widening after the insert opens a window in which an address +/// is registered but outside the published range, so a concurrent probe would +/// answer `false` for an address that is genuinely registered. +/// +/// `lo` and `hi` are separate atomics, so a racing reader can observe a mix of +/// old and new values. That is harmless: each moves in one direction only, so +/// once `lo <= a <= hi` holds for an address it holds forever, and a reader +/// that observes a partially-updated pair observes a window that is only ever +/// wider than the one it replaced — never narrower. +/// +/// Cross-thread visibility rests on [`admit`](Self::admit)'s `AcqRel` +/// read-modify-writes. An RMW reads the latest value in its location's +/// modification order, so its acquire half joins every earlier widening — by +/// any thread — into the admitting thread's happens-before graph before that +/// thread publishes the address. A reader can only ask about an address it has +/// obtained, which requires the publishing hand-off, and `may_contain`'s +/// `Acquire` loads complete the chain. This is why `admit` may not skip the +/// RMW: see its own documentation. +#[derive(Debug)] +pub struct RegistryAddrWindow { + lo: AtomicUsize, + hi: AtomicUsize, +} + +impl Default for RegistryAddrWindow { + fn default() -> Self { + Self::new() + } +} + +impl RegistryAddrWindow { + /// An empty window: contains no address at all. + pub const fn new() -> Self { + Self { + lo: AtomicUsize::new(usize::MAX), + hi: AtomicUsize::new(0), + } + } + + /// `false` ⟹ `addr` is definitively absent from every table this window + /// covers, so the caller can answer "not found" without touching one. + /// + /// This is the hot side. It is deliberately `inline(always)`: the whole + /// point is that the common negative answer costs a couple of loads at the + /// call site rather than a call into the registry probe. + #[inline(always)] + pub fn may_contain(&self, addr: usize) -> bool { + addr >= self.lo.load(Ordering::Acquire) && addr <= self.hi.load(Ordering::Acquire) + } + + /// Widen the window to include `addr`. + /// + /// MUST run **before** the guarded table is mutated — see the type docs. + /// + /// Two unconditional atomic read-modify-writes, deliberately with no + /// "already covered?" pre-check in front of them. Registration is rare — + /// `claude-code --help` calls this 10 times for buffers and 42 times for + /// typed arrays across a 6.9-billion-instruction run — so a fast path here + /// buys nothing measurable and costs the one thing this type cannot spend: + /// certainty. Two earlier drafts of exactly this function were wrong. + /// + /// 1. `load` then `store` is a read-modify-write with a hole in it. Two + /// threads registering at once both read the old bound, and the + /// *narrower* of the two stores can land last — dropping the other + /// thread's address out of the window while its entry is live in that + /// thread's registry. `fetch_min`/`fetch_max` are single RMWs, so no + /// update can be lost no matter how the two interleave. + /// + /// 2. A `Relaxed` "skip if already covered" pre-check reintroduces the same + /// bug one level up, in the memory model rather than in the interleaving. + /// If this thread skips the RMW because it *observed* another thread's + /// widening, it performs no acquire, so that other thread's widening + /// never enters this thread's happens-before graph — and a reader that + /// synchronises only with THIS thread's subsequent publish is not + /// guaranteed to see the bound that actually covers the address. It + /// would answer "not registered" for a registered pointer. + /// + /// `AcqRel` closes that: an RMW reads the latest value in the location's + /// modification order, and the acquire half joins every prior widening into + /// this thread's happens-before graph before the caller publishes. + #[inline] + pub fn admit(&self, addr: usize) { + self.lo.fetch_min(addr, Ordering::AcqRel); + self.hi.fetch_max(addr, Ordering::AcqRel); + } + + /// Test hook: the current `[lo, hi]` pair, or `None` while empty. + #[cfg(test)] + pub(crate) fn bounds_for_tests(&self) -> Option<(usize, usize)> { + let lo = self.lo.load(Ordering::Acquire); + let hi = self.hi.load(Ordering::Acquire); + (lo <= hi).then_some((lo, hi)) + } +} + #[cfg(test)] mod tests { use super::*; @@ -136,4 +258,85 @@ mod tests { std::thread::spawn(|| LATCH.arm()).join().unwrap(); assert!(LATCH.is_armed()); } + + #[test] + fn empty_window_contains_nothing() { + let w = RegistryAddrWindow::new(); + assert_eq!(w.bounds_for_tests(), None); + for addr in [0usize, 1, 0x1000, usize::MAX / 2, usize::MAX] { + assert!( + !w.may_contain(addr), + "an empty window must reject {addr:#x} — it stands in for an idle latch" + ); + } + } + + #[test] + fn window_only_ever_widens_and_never_rejects_an_admitted_address() { + let w = RegistryAddrWindow::new(); + w.admit(0x3000); + assert_eq!(w.bounds_for_tests(), Some((0x3000, 0x3000))); + assert!(w.may_contain(0x3000)); + assert!(!w.may_contain(0x2fff)); + assert!(!w.may_contain(0x3001)); + + w.admit(0x9000); + assert_eq!(w.bounds_for_tests(), Some((0x3000, 0x9000))); + // Both admitted addresses stay inside, and so does everything between. + assert!(w.may_contain(0x3000)); + assert!(w.may_contain(0x6000)); + assert!(w.may_contain(0x9000)); + assert!(!w.may_contain(0x2fff)); + assert!(!w.may_contain(0x9001)); + + // Re-admitting an interior address must not narrow anything. + w.admit(0x6000); + assert_eq!(w.bounds_for_tests(), Some((0x3000, 0x9000))); + } + + /// Concurrent registration must not lose an address. With a load-then-store + /// `admit` the two threads' stores race and the narrower bound can land + /// last, evicting the other thread's live registration from the window — + /// a false negative, which is a misclassification rather than a slowdown. + /// `fetch_min`/`fetch_max` make that impossible, so this passes + /// deterministically here and fails with high probability on the racy form. + #[test] + fn concurrent_admits_never_drop_an_address() { + const THREADS: usize = 8; + const PER_THREAD: usize = 512; + static WINDOW: RegistryAddrWindow = RegistryAddrWindow::new(); + let handles: Vec<_> = (0..THREADS) + .map(|t| { + std::thread::spawn(move || { + // Interleave the ranges so every thread admits both very + // low and very high addresses, maximising the number of + // genuine bound updates that can race. + for i in 0..PER_THREAD { + WINDOW.admit(0x1_0000 + i * THREADS + t); + } + }) + }) + .collect(); + for h in handles { + h.join().unwrap(); + } + for t in 0..THREADS { + for i in 0..PER_THREAD { + let addr = 0x1_0000 + i * THREADS + t; + assert!( + WINDOW.may_contain(addr), + "{addr:#x} was admitted but the window lost it: {:?}", + WINDOW.bounds_for_tests() + ); + } + } + } + + #[test] + fn window_admit_is_visible_to_another_thread() { + static WINDOW: RegistryAddrWindow = RegistryAddrWindow::new(); + assert!(!WINDOW.may_contain(0x4000)); + std::thread::spawn(|| WINDOW.admit(0x4000)).join().unwrap(); + assert!(WINDOW.may_contain(0x4000)); + } } diff --git a/crates/perry-runtime/src/registry_latch_probes.rs b/crates/perry-runtime/src/registry_latch_probes.rs index 3c450a6130..bdf94772fa 100644 --- a/crates/perry-runtime/src/registry_latch_probes.rs +++ b/crates/perry-runtime/src/registry_latch_probes.rs @@ -216,6 +216,102 @@ fn detached_buffer_mark_is_found_after_the_idle_fast_path_ran() { assert!(!crate::buffer::is_detached_buffer(scratch)); } +/// An address no allocator on any supported platform can return: above +/// `addr_class::is_valid_obj_ptr`'s heap ceiling, so no concurrently running +/// test can widen a registry window to cover it. +const FAR_OUTSIDE_ANY_WINDOW: usize = 0x7000_0000_0000_0000; + +/// The window is a fast path, so it must be shown to *run*, not merely to give +/// the right answer — a probe that reached the registries and missed returns +/// `false` too. The probe counter distinguishes the two, and the second half of +/// this test is what makes the first half able to fail: an always-`false` +/// `may_contain` would pass the rejection assertion and fail here. +#[test] +fn buffer_probe_rejects_an_out_of_window_address_without_touching_the_registries() { + // Two real registrations, so the window has an interior rather than a + // single point. + let first = crate::buffer::buffer_alloc(32) as usize; + let second = crate::buffer::buffer_alloc(32) as usize; + let (lo, hi) = crate::buffer::test_buffer_addr_window_bounds() + .expect("registering a buffer must open the address window"); + assert!( + lo <= first.min(second) && hi >= first.max(second), + "the window must cover every registered buffer: \ + [{lo:#x}, {hi:#x}] vs {first:#x} / {second:#x}" + ); + + let before = crate::buffer::test_buffer_registry_probe_count(); + assert!( + !crate::buffer::is_registered_buffer(FAR_OUTSIDE_ANY_WINDOW), + "an address outside the window is not a registered buffer" + ); + assert_eq!( + crate::buffer::test_buffer_registry_probe_count(), + before, + "the address window must answer without reaching the registries" + ); + + // A registered address must still be admitted AND still resolve — this is + // the direction in which a wrong window is a type confusion, not a slowdown. + assert!( + crate::buffer::is_registered_buffer(first), + "the window must not hide a registered buffer" + ); + assert!( + crate::buffer::test_buffer_registry_probe_count() > before, + "an in-window address must reach the registries" + ); +} + +#[test] +fn typed_array_probe_rejects_an_out_of_window_address_without_touching_the_registry() { + let first = crate::typedarray::typed_array_alloc(crate::typedarray::KIND_UINT8, 4) as usize; + let second = crate::typedarray::typed_array_alloc(crate::typedarray::KIND_FLOAT64, 4) as usize; + let (lo, hi) = crate::typedarray::test_typed_array_addr_window_bounds() + .expect("registering a typed array must open the address window"); + assert!(lo <= first.min(second) && hi >= first.max(second)); + + let before = crate::typedarray::test_typed_array_window_admitted_probe_count(); + assert_eq!( + crate::typedarray::lookup_typed_array_kind(FAR_OUTSIDE_ANY_WINDOW), + None + ); + assert_eq!( + crate::typedarray::test_typed_array_window_admitted_probe_count(), + before, + "the address window must answer without reaching the registry \ + or writing a negative cache entry" + ); + + assert_eq!( + crate::typedarray::lookup_typed_array_kind(first), + Some(crate::typedarray::KIND_UINT8), + "the window must not hide a registered typed array" + ); + assert!(crate::typedarray::test_typed_array_window_admitted_probe_count() > before); +} + +/// `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 +/// `js_shared_array_buffer_new`, which also calls `register_buffer`) is what +/// makes this test able to fail: it exercises the `note_buffer_like_registered` +/// path alone. +#[test] +fn shared_sab_backing_is_inside_the_buffer_address_window() { + let sab = crate::shared_sab::alloc_shared_sab(64) as usize; + assert!( + crate::buffer::is_registered_buffer(sab), + "a SharedArrayBuffer backing must stay visible to `is_registered_buffer`" + ); + let (lo, hi) = crate::buffer::test_buffer_addr_window_bounds() + .expect("a SAB allocation must open the address window"); + assert!( + lo <= sab && sab <= hi, + "the SAB route must widen the window: {sab:#x} outside [{lo:#x}, {hi:#x}]" + ); +} + /// The ordering rule itself, modelled on a private latch + table pair so both /// orderings can be run. This is the "prove the gate can fail" half: if /// arm-after-insert were harmless the wrong-order case would be indistinguishable diff --git a/crates/perry-runtime/src/shared_sab.rs b/crates/perry-runtime/src/shared_sab.rs index 4421c7f359..60a4ac6c64 100644 --- a/crates/perry-runtime/src/shared_sab.rs +++ b/crates/perry-runtime/src/shared_sab.rs @@ -79,7 +79,7 @@ pub fn alloc_shared_sab(size: u32) -> *mut BufferHeader { // the idle fast path. (This is the ordering `js_buffer_register_external` // already documents; see also `crate::registry_latch`.) SHARED_SAB_NONEMPTY.store(true, Ordering::Release); - crate::buffer::note_buffer_like_registered(); + crate::buffer::note_buffer_like_registered(buf as usize); registry() .lock() .unwrap_or_else(|e| e.into_inner()) @@ -135,7 +135,7 @@ pub(crate) fn test_seed_shared_sab(addr: usize) { // fixture exercises the real fast/slow-path split rather than a state the // production path never produces. SHARED_SAB_NONEMPTY.store(true, Ordering::Release); - crate::buffer::note_buffer_like_registered(); + crate::buffer::note_buffer_like_registered(addr); registry() .lock() .unwrap_or_else(|e| e.into_inner()) diff --git a/crates/perry-runtime/src/typedarray/mod.rs b/crates/perry-runtime/src/typedarray/mod.rs index f74cfe5f5b..6933a30369 100644 --- a/crates/perry-runtime/src/typedarray/mod.rs +++ b/crates/perry-runtime/src/typedarray/mod.rs @@ -356,10 +356,52 @@ fn ta_kind_cache_get(addr: usize) -> Option> { static TYPED_ARRAY_EVER_REGISTERED: crate::registry_latch::RegistryLatch = crate::registry_latch::RegistryLatch::new(); +/// Smallest and largest address ever entered into `TYPED_ARRAY_REGISTRY`. +/// +/// The latch above stops discriminating at the first typed array. On +/// `claude-code --help` there are 42 registrations against 3,566,956 probes — +/// and every one of those probes answered `None` (uretprobe count, one run: +/// not a single `Some` in the whole run). Each still paid the +/// out-of-line call, the direct-mapped cache probe and, on the (usual) cold +/// miss, a thread-local resolution, a hash **and a negative-entry write-back +/// that dirties a shared cache line**. +/// +/// `register_typed_array` is the only writer of `TYPED_ARRAY_REGISTRY` and of +/// the positive entries in `PERRY_TA_KIND_CACHE`, and it widens this window +/// before it touches either, so an address outside the window is definitively +/// not a registered typed array. Unlike a `GcHeader` tag test this never +/// dereferences the candidate — which matters, because a registered typed array +/// is not required to have a readable `ptr - GC_HEADER_SIZE` (see the +/// guard-page fixture in `promise::combinators`). +static TYPED_ARRAY_ADDR_WINDOW: crate::registry_latch::RegistryAddrWindow = + crate::registry_latch::RegistryAddrWindow::new(); + +/// Test hook: the registered-address window's current bounds. +#[cfg(test)] +pub(crate) fn test_typed_array_addr_window_bounds() -> Option<(usize, usize)> { + TYPED_ARRAY_ADDR_WINDOW.bounds_for_tests() +} + +#[cfg(test)] +thread_local! { +/// Test-only count of probes that got past `TYPED_ARRAY_ADDR_WINDOW` and +/// reached [`lookup_registered_typed_array_kind`]. `TEST_TA_REGISTRY_PROBES` +/// counts ENTRIES into `lookup_typed_array_kind` and so cannot see the window +/// working; this counts the calls the window was added to remove. + static TEST_TA_WINDOW_ADMITTED_PROBES: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +#[cfg(test)] +pub(crate) fn test_typed_array_window_admitted_probe_count() -> u64 { + TEST_TA_WINDOW_ADMITTED_PROBES.with(|c| c.get()) +} + pub fn register_typed_array(ptr: *const TypedArrayHeader, kind: u8) { // Arm BEFORE either table becomes readable — an arm placed after the stores // would leave a window in which `lookup_typed_array_kind` answers `None` for - // this very array. See `crate::registry_latch`. + // this very array. See `crate::registry_latch`. The address window carries + // the same obligation and is widened first for the same reason. + TYPED_ARRAY_ADDR_WINDOW.admit(ptr as usize); TYPED_ARRAY_EVER_REGISTERED.arm(); // Keep the cache authoritative: overwrite any colliding/stale slot so a // freed-then-reused address never reads back its previous kind. @@ -426,6 +468,36 @@ pub fn lookup_typed_array_kind(addr: usize) -> Option { if TYPED_ARRAY_EVER_REGISTERED.is_idle() { return None; } + // Armed says only that SOME typed array exists. An address outside the + // registered window is not this one, and rejecting it here keeps the whole + // negative answer inline: no call, no cache-slot load, and — the part that + // costs the most on a cold address — no negative-entry write-back into the + // shared `PERRY_TA_KIND_CACHE`. See `TYPED_ARRAY_ADDR_WINDOW`. + if !TYPED_ARRAY_ADDR_WINDOW.may_contain(addr) { + // Completeness audit — see the twin in `buffer::header:: + // is_registered_buffer` for why the writer set is machine-checked + // rather than enumerated. Read the registry DIRECTLY rather than + // through `lookup_registered_typed_array_kind`: that function writes a + // negative entry into `PERRY_TA_KIND_CACHE` on a miss, and an audit + // that mutates the state it audits changes what the next probe does. + // `TYPED_ARRAY_REGISTRY` is authoritative anyway — every positive cache + // entry is derived from it. Compiled out entirely in release. + #[cfg(debug_assertions)] + { + assert!( + TYPED_ARRAY_REGISTRY + .with(|r| r.borrow().get(&addr).copied()) + .is_none(), + "TYPED_ARRAY_ADDR_WINDOW rejected {addr:#x}, but it IS in \ + TYPED_ARRAY_REGISTRY. Some route reached the registry without \ + calling `TYPED_ARRAY_ADDR_WINDOW.admit()` (via \ + `register_typed_array`) first." + ); + } + return None; + } + #[cfg(test)] + TEST_TA_WINDOW_ADMITTED_PROBES.with(|c| c.set(c.get().wrapping_add(1))); lookup_registered_typed_array_kind(addr) }