perf(runtime): symbol probe stops taking the global mutex to say "no" (third latch instance)

changelog.d/9177-symbol-registry-range-filter.md

@@ -0,0 +1,31 @@
+`is_registered_symbol` sits on property/method dispatch, so it is asked about
+almost every pointer a program touches. `SYMBOL_EVER_REGISTERED` answered the
+idle case in one atomic load, but stopped discriminating the moment a program
+created its first symbol — and every program that touches a well-known symbol
+creates one. After that each probe took the process-global registry mutex,
+overwhelmingly to say "no".
+
+The registry now carries the smallest and largest pointer ever registered, and
+a pointer outside that range is rejected without the lock.
+`PERRY_SYMBOL_RANGE_FILTER=0` restores the unconditional acquisition.
+
+Unlike the buffer registries this pattern was lifted from, `SYMBOL_POINTERS` is
+a GC ROOT registry: the collector re-keys an entry to the symbol's new address
+every time it moves one. A range filter is sound only while every path that
+puts a pointer in the set widens the bounds first, and there are three — the
+registration, a per-slot forwarding rewrite, and the bulk rewrite that the
+copying minor actually drives. Only the first widened, so a symbol evacuated
+past the bounds its own allocation established became a live, registered symbol
+that the probe reported as "not a symbol", losing `typeof`, symbol-keyed
+property lookup and `Symbol.iterator` dispatch for it.
+
+Widening and inserting are now a single operation (`insert_symbol_pointer_in_set`),
+which is what makes the three sites uniform rather than three chances to forget.
+
+`test_copying_minor_keeps_moved_symbol_visible_to_the_range_filter` covers it
+through the public probe after a real copying minor. The existing sibling test
+asserts membership with `test_symbol_pointer_root_contains`, which reads the set
+directly and so could not see this — the entry IS in the set. The new test also
+resets the range around a single registration, because a wide range left by
+earlier tests in the same binary would admit the moved address by luck and make
+the assertion vacuous.

crates/perry-runtime/src/gc/tests/copying_side_tables.rs

@@ -155,6 +155,62 @@ fn test_copying_minor_rewrites_symbol_side_table_roots_and_lookups() {
     }
 }
 
+/// The range filter in front of `SYMBOL_POINTERS` must admit a symbol's NEW
+/// address after the collector moves it.
+///
+/// `is_registered_symbol` rejects an out-of-range pointer WITHOUT consulting
+/// the set, so a moved symbol whose new address fell outside the bounds its
+/// allocation established would be a live, registered symbol that the probe
+/// reports as "not a symbol" — losing `typeof`, symbol-keyed property lookup
+/// and `Symbol.iterator` dispatch.
+///
+/// The sibling test above asserts membership with
+/// `test_symbol_pointer_root_contains`, which reads the set directly and so
+/// cannot see this: the entry IS in the set. This one goes through the public
+/// probe, and resets the range first so exactly one registration defines it —
+/// otherwise a wide range left by earlier tests in this binary would admit the
+/// moved address by luck and the assertion would be vacuous.
+#[test]
+fn test_copying_minor_keeps_moved_symbol_visible_to_the_range_filter() {
+    let _guard = CopyingNurseryTestGuard::new(1);
+    crate::symbol::test_clear_symbol_side_table_roots();
+    gc_register_mutable_root_scanner(crate::symbol::scan_symbol_side_table_roots_mut);
+
+    // Reset AFTER the clear above, and register exactly one symbol, so the
+    // range is the single point that symbol sits at.
+    let _range = crate::symbol::SymbolAddrRangeGuard::reset();
+
+    let sym_key = unsafe { alloc_nursery_test_symbol() };
+    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 _ = gc_collect_minor();
+
+    let sym_key_after = (js_shadow_slot_get(0) & POINTER_MASK) as usize;
+    assert_ne!(
+        sym_key_after, sym_key,
+        "fixture must actually MOVE the symbol, or nothing is under test"
+    );
+    assert!(crate::symbol::test_symbol_pointer_root_contains(
+        sym_key_after
+    ));
+    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"
+    );
+}
+
 #[test]
 fn test_copying_minor_rewrites_old_overflow_object_child_without_reentrant_borrow() {
     struct OverflowFieldsRootGuard;

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, Ordering};
+use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering};
 use std::sync::Mutex;
 
 // NaN-boxing tags (must match value.rs)
@@ -448,15 +448,103 @@ 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
+/// filter in front of the process-global registry mutex.
+///
+/// `SYMBOL_EVER_REGISTERED` answers "has any symbol EVER been registered?",
+/// which stops discriminating the moment a program creates its first symbol —
+/// and every program that touches a well-known symbol creates one. After that
+/// `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
+/// 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
+/// 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`.
+///
+/// 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".
+///
+/// That is not a theoretical second inserter: `SYMBOL_POINTERS` is a GC root
+/// registry, and `rewrite_symbol_pointer_metadata_if_forwarded` re-keys an
+/// entry to the symbol's new address every time the collector moves it. A
+/// 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);
+}
+
+/// The ONLY way to put a pointer into `SYMBOL_POINTERS`. Widening 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.
+pub(crate) fn insert_symbol_pointer_in_set(set: &mut PtrHashSet<usize>, ptr: usize) {
+    widen_symbol_addr_range(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.
+#[cfg(test)]
+pub(crate) struct SymbolAddrRangeGuard(usize, usize);
+
+#[cfg(test)]
+impl SymbolAddrRangeGuard {
+    /// Reset to the empty range, 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
+    }
+}
+
+#[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);
+    }
+}
+
+#[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 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);
     let mut guard = crate::gc::lock_gc_root_registry(&SYMBOL_POINTERS);
     if guard.is_none() {
         *guard = Some(new_ptr_hash_set());
     }
-    guard.as_mut().unwrap().insert(ptr);
+    insert_symbol_pointer_in_set(guard.as_mut().unwrap(), ptr);
 }
 
 #[cfg(test)]
@@ -527,11 +615,30 @@ pub fn is_registered_symbol(ptr: usize) -> bool {
     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<bool> = 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))
 }

crates/perry-runtime/src/symbol/gc_roots.rs

@@ -126,7 +126,7 @@ fn scan_symbol_pointer_metadata_roots_mut(visitor: &mut crate::gc::RuntimeRootVi
     for (old_ptr, new_ptr) in rewrites {
         set.remove(&old_ptr);
         if new_ptr != 0 {
-            set.insert(new_ptr);
+            insert_symbol_pointer_in_set(set, new_ptr);
         }
     }
 }
@@ -346,7 +346,7 @@ fn rewrite_symbol_pointer_metadata_if_forwarded(
     if let Some(set) = guard.as_mut() {
         set.remove(&ptr);
         if new_ptr != 0 {
-            set.insert(new_ptr);
+            insert_symbol_pointer_in_set(set, new_ptr);
         }
     }
 }
@@ -376,7 +376,11 @@ pub(crate) fn test_clear_symbol_side_table_roots() {
     if persistent.is_empty() {
         *guard = None;
     } else {
-        *guard = Some(persistent.into_iter().collect());
+        let mut set = new_ptr_hash_set();
+        for ptr in persistent {
+            insert_symbol_pointer_in_set(&mut set, ptr);
+        }
+        *guard = Some(set);
     }
 }