From 8b92b93302df355696468c8291f9a10960014e36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 15 Aug 2026 00:25:27 +0200 Subject: [PATCH 1/2] perf(codegen): fix a root-reload allowlist symbol that has never matched anything MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `root_reload.rs`'s `NON_COLLECTING` listed `js_gc_layout_note_slot`. The runtime exports `js_gc_note_slot_layout` and `js_gc_note_slot_layout_aware`; no symbol by the old spelling exists in the tree. The list is matched against LLVM callee names by exact string, so the entry never fired, and the fallback for an unrecognised helper is safe-direction — treat it as collecting, i.e. insert a reload. The result was a silent pessimisation: every emitted slot-layout note forced a root reload, including the one per guarded array element store, which is #5094's hot path. Six further names in the same list (`js_runtime_write_barrier_slot`, `js_value_is_object`, `js_value_is_string`, `js_typeof_tag`, `js_typed_feedback_shape_guard`, `js_typed_feedback_note`) are also symbols this tree does not export. They cost nothing on their own, but they made a real transposition indistinguishable from an aspirational entry, so they are removed from both this list and its twin in `scripts/gc_root_dominance_check.py`. `_aware` is added to the checker's `NONCOLLECTING` alongside the entry point: 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 name the set already admits — the same "differs only by doing less" argument the file already records for `declare` vs `init`. The file's one-way containment invariant is preserved. Two regression tests, both sabotage-verified to fail with the parent spelling: every `NON_COLLECTING` entry must have an `extern "C" fn` definition in perry-runtime/perry-stdlib, and the two note helpers are pinned by name because the bug was a missing entry and "no phantoms" is satisfied by an empty set. Validation: `cargo test --release -p perry-codegen --lib root_reload` 25 passed; with the parent spelling restored, 23 passed / 2 failed naming `js_gc_layout_note_slot` exactly. `gc_root_dominance_check.py` over the full 144-source corpus: 0 violations across 2959 functions / 172 modules / 13214 root stores. `cargo fmt --all -- --check`, `git diff --check`, `check_file_size.sh`, `gc_store_site_inventory.py`, `raw_handle_debt.py`, `addr_class_inventory.py`, `gc_runtime_root_holders.py`, and the checker's `--self-test` all pass. Refs #5094. --- crates/perry-codegen/src/root_reload.rs | 33 ++++-- crates/perry-codegen/src/root_reload_tests.rs | 100 ++++++++++++++++++ scripts/gc_root_dominance_check.py | 17 +-- 3 files changed, 136 insertions(+), 14 deletions(-) diff --git a/crates/perry-codegen/src/root_reload.rs b/crates/perry-codegen/src/root_reload.rs index ccccc1672c..9a329979e7 100644 --- a/crates/perry-codegen/src/root_reload.rs +++ b/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", diff --git a/crates/perry-codegen/src/root_reload_tests.rs b/crates/perry-codegen/src/root_reload_tests.rs index 82f600ab43..b9ec74e65d 100644 --- a/crates/perry-codegen/src/root_reload_tests.rs +++ b/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 ` 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" + ); +} diff --git a/scripts/gc_root_dominance_check.py b/scripts/gc_root_dominance_check.py index 91b6abfcc2..557b6b4cca 100755 --- a/scripts/gc_root_dominance_check.py +++ b/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 From f1ec083f1ae27ec2e82339911cd9355324972fa1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 15 Aug 2026 00:26:08 +0200 Subject: [PATCH 2/2] docs(changelog): changeset for the root-reload allowlist symbol fix --- .../8106-root-reload-note-slot-symbols.md | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 changelog.d/8106-root-reload-note-slot-symbols.md diff --git a/changelog.d/8106-root-reload-note-slot-symbols.md b/changelog.d/8106-root-reload-note-slot-symbols.md new file mode 100644 index 0000000000..b27e073607 --- /dev/null +++ b/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.