perf(codegen): fix a root-reload allowlist symbol that has never matched anything

changelog.d/8106-root-reload-note-slot-symbols.md

@@ -0,0 +1,30 @@
+**Fixed a symbol name in codegen's root-reload allowlist that has never matched
+anything (#5094).** `root_reload.rs`'s `NON_COLLECTING` listed
+`js_gc_layout_note_slot`; the runtime exports `js_gc_note_slot_layout`
+(`gc/layout.rs:814`) and `js_gc_note_slot_layout_aware` (`:833`). No symbol by
+the old spelling exists anywhere in the tree — `gc_call_effects.rs` and all
+twelve tests that reference these helpers use the real names.
+
+The failure mode is why it survived. The file's own contract says a helper
+missing from the list "is treated as collecting, which inserts a reload the
+checker would not have demanded — a load, not a bug". So the typo was
+safe-direction and silent: every emitted slot-layout note forced a root reload
+instead of none, including the one call per guarded array element store
+(`expr/index_set_guarded.rs`), which is the hot path #5094 exists for.
+
+Both real names are now listed, and the phantom is removed from
+`scripts/gc_root_dominance_check.py` as well — that file carried it too,
+harmlessly, because it also carries the correct spelling. `_aware` is added
+there alongside: it is `js_gc_note_slot_layout` behind an early return taken
+when neither the new nor the old bits are pointer-bearing, so it does strictly
+less than the entry point the set already admits — the same "differ only by
+doing less" argument the file already accepts for `declare` vs `init`. The
+one-way containment invariant (`root_reload.rs`'s list is a subset of the
+checker's) is preserved in both directions of the edit.
+
+Two regression tests in `root_reload_tests.rs`, both failing on the parent
+commit: every `NON_COLLECTING` entry must be a name
+`gc_call_effects::classify_direct_callee` answers `CannotCollect` for — which is
+the "the two lists must agree" rule the checker's own comment states and nothing
+enforced — and the two note helpers are pinned by name, because the bug was a
+*missing* entry and a containment test alone is satisfied by an empty set.

crates/perry-codegen/src/root_reload.rs

@@ -183,22 +183,39 @@ const NON_COLLECTING: &[&str] = &[
     "js_gc_init_typed_shape_layout",
     "js_gc_declare_typed_shape_layout",
     "js_gc_forget_object_layout",
-    "js_gc_layout_note_slot",
+    // The two real slot-layout note exports. `js_gc_layout_note_slot` used to
+    // stand here, and no such symbol has ever existed in the tree: the runtime
+    // spells them `js_gc_note_slot_layout` (`gc/layout.rs:814`) and
+    // `js_gc_note_slot_layout_aware` (`:833`), which is how
+    // `gc_call_effects.rs` and every test names them. The typo was
+    // safe-direction — an absent helper is treated as collecting, which costs
+    // a reload rather than losing one — but it meant EVERY emitted note forced
+    // a reload, including the one per guarded array element store
+    // (`expr/index_set_guarded.rs`), which is #5094's hot path.
+    "js_gc_note_slot_layout",
+    // `_aware` is `js_gc_note_slot_layout` plus an early return when neither
+    // the new nor the old bits are pointer-bearing, so it does strictly LESS
+    // than the entry point above. Same reasoning the checker already accepts
+    // for `declare` vs `init`.
+    "js_gc_note_slot_layout_aware",
     "js_write_barrier",
     "js_write_barrier_root_nanbox",
     "js_write_barrier_slot",
-    "js_runtime_write_barrier_slot",
     "js_gc_register_global_root",
-    // pure value predicates / bit twiddling
+    // pure value predicates / bit twiddling.
+    //
+    // `js_value_is_object`, `js_value_is_string` and `js_typeof_tag` used to sit
+    // here, and — like `js_gc_layout_note_slot` above them — none is a symbol
+    // this tree exports. Six such names had accumulated (the three here plus
+    // `js_runtime_write_barrier_slot`, `js_typed_feedback_shape_guard` and
+    // `js_typed_feedback_note`). They cost nothing on their own, because a name
+    // that matches no callee never fires; what they cost is camouflage — they
+    // made a REAL transposition indistinguishable from an aspirational entry.
+    // `every_non_collecting_entry_is_a_real_runtime_export` now rejects both.
     "js_is_truthy",
     "js_nanbox_get_pointer",
-    "js_value_is_object",
-    "js_value_is_string",
-    "js_typeof_tag",
     // inline-cache guards: pure reads
     "js_typed_feedback_closure_direct_call_guard",
-    "js_typed_feedback_shape_guard",
-    "js_typed_feedback_note",
     // verified non-allocating bookkeeping stores/reads
     "js_closure_set_capture_bits",
     "js_closure_get_capture_bits",

crates/perry-codegen/src/root_reload_tests.rs

@@ -909,3 +909,103 @@ fn a_capture_set_to_a_different_index_does_not_suppress_the_reload() {
         "a set to a DIFFERENT capture index must not suppress this reload"
     );
 }
+
+/// Every name in [`NON_COLLECTING`] must be a symbol the runtime actually
+/// exports.
+///
+/// The list is consulted by exact string match against an LLVM callee, so a
+/// name that matches nothing is inert — which is precisely why seven of them
+/// accumulated undetected. Six were aspirational (`js_value_is_object`,
+/// `js_typeof_tag`, …); the seventh, `js_gc_layout_note_slot`, was a
+/// transposition of the real, hot `js_gc_note_slot_layout`, and it cost a root
+/// reload at every emitted slot-layout note — including the one per guarded
+/// array element store.
+///
+/// Nothing could distinguish the two cases, because the fallback for an
+/// unrecognised helper is safe-direction (treat as collecting ⇒ insert a
+/// reload), so the only symptom was a permanent, quiet pessimisation. This
+/// makes a misspelling a test failure.
+#[test]
+fn every_non_collecting_entry_is_a_real_runtime_export() {
+    let mut sources = String::new();
+    for crate_dir in ["perry-runtime", "perry-stdlib"] {
+        let root = std::path::Path::new(env!("CARGO_MANIFEST_DIR"))
+            .join("..")
+            .join(crate_dir)
+            .join("src");
+        collect_rust_sources(&root, &mut sources);
+    }
+    assert!(
+        sources.len() > 1_000_000,
+        "runtime sources did not load (read {} bytes); the test would pass \
+         vacuously",
+        sources.len()
+    );
+
+    let phantom: Vec<&str> = NON_COLLECTING
+        .iter()
+        .copied()
+        .filter(|name| !declares_extern_c_fn(&sources, name))
+        .collect();
+    assert!(
+        phantom.is_empty(),
+        "NON_COLLECTING names with no `extern \"C\" fn` definition in \
+         perry-runtime/perry-stdlib — a name that matches no callee is inert, \
+         so a typo here is a silent pessimisation rather than a failure: \
+         {phantom:?}"
+    );
+}
+
+/// Append every `.rs` file under `dir` to `out`.
+fn collect_rust_sources(dir: &std::path::Path, out: &mut String) {
+    let Ok(entries) = std::fs::read_dir(dir) else {
+        return;
+    };
+    for entry in entries.flatten() {
+        let path = entry.path();
+        if path.is_dir() {
+            collect_rust_sources(&path, out);
+        } else if path.extension().is_some_and(|e| e == "rs") {
+            if let Ok(text) = std::fs::read_to_string(&path) {
+                out.push_str(&text);
+                out.push('\n');
+            }
+        }
+    }
+}
+
+/// Is there an `extern "C" fn <name>` definition in `sources`?
+///
+/// Deliberately matches the definition, not a mention: every phantom this
+/// catches was *mentioned* — in this list and in the checker's twin of it.
+fn declares_extern_c_fn(sources: &str, name: &str) -> bool {
+    sources
+        .match_indices("extern \"C\" fn ")
+        .any(|(at, marker)| {
+            let rest = &sources[at + marker.len()..];
+            rest.strip_prefix(name)
+                .is_some_and(|tail| !tail.starts_with(|c: char| c.is_alphanumeric() || c == '_'))
+        })
+}
+
+/// The two real slot-layout note exports must be present, spelled the way the
+/// runtime exports them (`gc/layout.rs`) and `gc_call_effects.rs` matches them.
+///
+/// Pinned by name rather than left to the existence test above, because the bug
+/// this replaces was a *missing* entry, and "no phantom members" is satisfied
+/// by an empty set.
+#[test]
+fn the_slot_layout_note_helpers_are_non_collecting() {
+    for name in ["js_gc_note_slot_layout", "js_gc_note_slot_layout_aware"] {
+        assert!(
+            NON_COLLECTING.contains(&name),
+            "{name} is emitted per guarded element/field store; leaving it out \
+             forces a root reload at every one of them"
+        );
+    }
+    assert!(
+        !NON_COLLECTING.contains(&"js_gc_layout_note_slot"),
+        "js_gc_layout_note_slot is not a symbol in this tree — it was a \
+         transposition of js_gc_note_slot_layout"
+    );
+}

scripts/gc_root_dominance_check.py

@@ -468,15 +468,12 @@ def build_cfg(f):
     # cannot -- side-table metadata writes through the system allocator, which
     # arms no Perry GC trigger.
     "js_gc_init_typed_shape_layout", "js_gc_declare_typed_shape_layout",
-    "js_gc_layout_note_slot",
     "js_write_barrier_root_nanbox", "js_write_barrier_slot",
-    "js_runtime_write_barrier_slot", "js_gc_register_global_root",
+    "js_gc_register_global_root",
     # pure value predicates / bit twiddling
-    "js_is_truthy", "js_nanbox_get_pointer", "js_value_is_object",
-    "js_value_is_string", "js_typeof_tag",
+    "js_is_truthy", "js_nanbox_get_pointer",
     # inline-cache guards: pure reads
     "js_typed_feedback_closure_direct_call_guard",
-    "js_typed_feedback_shape_guard", "js_typed_feedback_note",
     # ctor identity selection
     "js_ctor_return_override",
     "llvm.lifetime.start.p0", "llvm.lifetime.end.p0",
@@ -505,7 +502,15 @@ def build_cfg(f):
     "js_closure_unbox_callee_checked",
     # object/this_binding.rs:160 -- a thread-local cell swap
     "js_implicit_this_set", "js_implicit_this_get",
-    "js_gc_note_slot_layout", "js_string_addref_if_heap_string",
+    # `js_gc_note_slot_layout` (gc/layout.rs:814) and its `_aware` sibling
+    # (:833). `_aware` is the same body behind an early return taken when
+    # neither the new nor the old bits are pointer-bearing, so it does strictly
+    # LESS than the entry point beside it -- the same "differ only by doing
+    # less" argument this file already accepts for `declare` vs `init` above.
+    # A phantom third spelling, `js_gc_layout_note_slot`, sat in this set (and
+    # in `root_reload.rs`) and matched no symbol in the tree.
+    "js_gc_note_slot_layout", "js_gc_note_slot_layout_aware",
+    "js_string_addref_if_heap_string",
     # `js_get_string_pointer_unified` is deliberately NOT here. Its SSO branch
     # calls `js_string_materialize_to_heap`, which allocates (value/nanbox.rs:268),
     # so it is a collection point by this file's one-sided rule even though the