perf(runtime): let the GC header pick the side-registry probe on dynamic dispatch (#7850) - #7868
Merged
Merged
Conversation
added 2 commits
August 11, 2026 18:56
📝 WalkthroughWalkthroughThe runtime now screens potential symbols before reading GC headers, then uses the header object type to select Set, Map, or RegExp probes. New instrumentation and tests validate probe avoidance, symbol handling, object classification, and receiver behavior. ChangesObject classification dispatch
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant native_call_method
participant symbol
participant GcHeader
participant registries
native_call_method->>symbol: Check may_be_symbol_header
alt Symbol header detected
native_call_method->>symbol: Check registered symbol
else Non-symbol pointer
native_call_method->>GcHeader: Read obj_type
GcHeader-->>native_call_method: Return object type
native_call_method->>registries: Probe matching Set, Map, or RegExp registry
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
changelog.d/7868-header-directed-probe-dispatch.md (1)
33-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving the abandoned address-window design out of the release fragment.
Lines 33-39 describe a design that was tried and rejected. It is not part of the shipped behavior. When the release notes are assembled, a reader sees a
(lo, hi)address window described in detail before learning it never shipped.The refutation is valuable engineering context. Keep it in the PR description or in the
may_be_symbol_headerdoc comment, where it already partly lives. Compress this section in the fragment to one sentence stating that an address-window screen was rejected because allocator placement defeats it.Based on learnings: "For PerryTS/perry changelog fragments in changelog.d/, describe the final shipped behavior as one coherent release-note entry. Do not include separate development-slice narratives that may contradict one another when the release notes are assembled."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@changelog.d/7868-header-directed-probe-dispatch.md` around lines 33 - 39, Condense the rejected address-window discussion in this changelog fragment to one sentence stating that allocator placement defeats the screen and it was not shipped. Remove the detailed invariant-test narrative, address range, and allocation examples from the release note; retain that engineering context in the PR description or may_be_symbol_header documentation.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-runtime/src/object/native_call_method.rs`:
- Around line 946-962: The fallback in the object-type match must only classify
pointers as regexes when the GC header reports GC_TYPE_OBJECT. Update the
is_regex_pointer handling in the obj_type match, or ensure stale REGEX_POINTERS
entries are removed, so reused addresses cannot be accepted as RegExp pointers
with another object type.
In `@crates/perry-runtime/src/object/native_call_method/probe_dispatch_tests.rs`:
- Around line 165-167: Update the test setup around js_regexp_new to create a
crate::gc::RuntimeHandleScope, allocate pattern and flags through rooted
handles, and reload pattern from its handle immediately before calling
js_regexp_new. Ensure pattern remains rooted across the flags allocation and no
stale raw pointer is reused.
- Around line 150-157: Add an inline comment immediately before the well-known
symbol assertion explaining that classify(wk) is intentionally not asserted
because WELL_KNOWN_SYMBOLS is process-global while SYMBOL_POINTERS is
thread-local; when the pointer is created on another thread, classify may not
recognize it and could inspect ptr - 8 unsafely. Keep the existing magic-byte
assertion unchanged.
- Around line 246-264: Replace the unsafe GC_HEADER_SIZE-based metadata read in
the leaked-symbol validation loop with a defined way to determine each symbol’s
relevant type or registration status. Preserve the assertion’s purpose of
proving these symbols are not independently excluded by set, map, or regex
checks, without dereferencing memory before the Box allocation returned by
js_symbol_for.
---
Nitpick comments:
In `@changelog.d/7868-header-directed-probe-dispatch.md`:
- Around line 33-39: Condense the rejected address-window discussion in this
changelog fragment to one sentence stating that allocator placement defeats the
screen and it was not shipped. Remove the detailed invariant-test narrative,
address range, and allocation examples from the release note; retain that
engineering context in the PR description or may_be_symbol_header documentation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a593d06-d361-44ca-80c8-01030b1f927d
📒 Files selected for processing (4)
changelog.d/7868-header-directed-probe-dispatch.mdcrates/perry-runtime/src/object/native_call_method.rscrates/perry-runtime/src/object/native_call_method/probe_dispatch_tests.rscrates/perry-runtime/src/symbol.rs
Comment on lines
+946
to
+962
| if crate::symbol::may_be_symbol_header(ptr as *const u8) | ||
| && crate::symbol::is_registered_symbol(addr) | ||
| { | ||
| return None; | ||
| } | ||
| let gc_header = (ptr as *const u8).sub(crate::gc::GC_HEADER_SIZE) as *const crate::gc::GcHeader; | ||
| Some((ptr, (*gc_header).obj_type)) | ||
| let obj_type = (*gc_header).obj_type; | ||
| let excluded = match obj_type { | ||
| crate::gc::GC_TYPE_SET => crate::set::is_registered_set(addr), | ||
| crate::gc::GC_TYPE_MAP => crate::map::is_registered_map(addr), | ||
| crate::gc::GC_TYPE_OBJECT => crate::regex::is_regex_pointer(ptr as *const u8), | ||
| _ => false, | ||
| }; | ||
| if excluded { | ||
| return None; | ||
| } | ||
| Some((ptr, obj_type)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm each side-registry probe ends in the matching GcHeader obj_type check.
set -euo pipefail
for fn in is_registered_set is_registered_map is_regex_pointer; do
echo "===== $fn ====="
ast-grep run --lang rust --pattern "fn $fn(\$\$\$) -> \$_ { \$\$\$ }" crates/perry-runtime/src
done
echo "===== every insert into the three registries ====="
rg -nP -C 6 '(SET_POINTERS|MAP_POINTERS|REGEX_POINTERS)\b' crates/perry-runtime/src --type rust
echo "===== gc_malloc type tags used by Set/Map/RegExp allocation ====="
rg -nP -C 4 'gc_malloc\s*\(' crates/perry-runtime/src/set.rs crates/perry-runtime/src/map.rs crates/perry-runtime/src/regex.rs 2>/dev/null || trueRepository: PerryTS/perry
Length of output: 181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== candidate definitions ====="
rg -n -C 12 'is_registered_(set|map)|is_regex_pointer' crates/perry-runtime/src --type rust
echo "===== registry declarations and writes ====="
rg -n -C 8 'SET_POINTERS|MAP_POINTERS|REGEX_POINTERS|registered_set|registered_map|regex_pointer' crates/perry-runtime/src --type rust
echo "===== relevant allocation/type-tag sites ====="
rg -n -C 10 'GC_TYPE_(SET|MAP|OBJECT)|gc_malloc' crates/perry-runtime/src/set.rs crates/perry-runtime/src/map.rs crates/perry-runtime/src/regex.rs --type rustRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== exact definitions ====="
rg -n 'pub .*fn (is_registered_set|is_registered_map|is_regex_pointer)|fn (is_registered_set|is_registered_map|is_regex_pointer)' \
crates/perry-runtime/src/set.rs crates/perry-runtime/src/map.rs crates/perry-runtime/src/regex.rs
echo "===== registry storage and mutation in set.rs/map.rs/regex.rs ====="
rg -n 'static|thread_local|Mutex|HashMap|HashSet|insert\(|remove\(|contains\(|GC_TYPE_(SET|MAP|OBJECT)|arena_alloc_gc' \
crates/perry-runtime/src/set.rs crates/perry-runtime/src/map.rs crates/perry-runtime/src/regex.rs
echo "===== tag assertions in focused tests ====="
rg -n -C 3 'registered (Set|Map)|is_registered_(set|map)|is_regex_pointer|GC_TYPE_(SET|MAP|OBJECT)' \
crates/perry-runtime/src/array/collection_tag_tests.rs --type rustRepository: PerryTS/perry
Length of output: 25072
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== set registration probe ====="
sed -n '228,270p' crates/perry-runtime/src/set.rs
echo "===== map registration probe ====="
sed -n '205,252p' crates/perry-runtime/src/map.rs
echo "===== regex registration probe and helper ====="
sed -n '105,190p' crates/perry-runtime/src/regex.rs
echo "===== allocation and registration sites ====="
sed -n '815,855p' crates/perry-runtime/src/set.rs
sed -n '1035,1090p' crates/perry-runtime/src/map.rs
sed -n '795,905p' crates/perry-runtime/src/regex.rsRepository: PerryTS/perry
Length of output: 19361
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== regex registry lifetime and cleanup ====="
rg -n -C 8 'REGEX_POINTERS|regex_pointers_contains|REGEX_SOURCE_TABLE|is_regex_pointer|regex_header_has_magic' \
crates/perry-runtime/src/regex.rs
echo "===== gc_malloc reuse and reclamation paths ====="
rg -n -C 8 'pub .*gc_malloc|fn gc_malloc|gc_malloc\(|free|dealloc|sweep|reuse|from.space|fromspace' \
crates/perry-runtime/src/gc crates/perry-runtime/src/arena --type rust | head -n 1200
echo "===== tests for stale regex addresses or type reuse ====="
rg -n -C 8 'stale.*regex|regex.*stale|REGEX_POINTERS|regex_header_has_magic|GC_TYPE_OBJECT' \
crates/perry-runtime/src --glob '*test*' --type rustRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== gc_malloc definition ====="
rg -n '(^|[[:space:]])(pub([[:space:]]*\([^)]*\))?[[:space:]]+)?(unsafe[[:space:]]+)?fn[[:space:]]+gc_malloc|gc_malloc' \
crates/perry-runtime/src/gc crates/perry-runtime/src/arena --type rust | head -n 120
echo "===== malloc-object sweep/free implementation ====="
rg -n -C 12 'malloc.*(sweep|free)|sweep.*malloc|dealloc.*malloc|malloc_registry|MALLOC_OBJECT|gc_malloc' \
crates/perry-runtime/src/gc --type rust | head -n 1000
echo "===== regex-specific tests ====="
rg -n -C 6 'stale|reuse|recycled|is_regex_pointer|is_valid_regex_ptr|is_registered_regex|REGEX_POINTERS' \
crates/perry-runtime/src/regex.rs crates/perry-runtime/src --glob '*regex*test*' --type rustRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
set_src = Path("crates/perry-runtime/src/set.rs").read_text()
map_src = Path("crates/perry-runtime/src/map.rs").read_text()
regex_src = Path("crates/perry-runtime/src/regex.rs").read_text()
def body(src, signature):
start = src.index(signature)
brace = src.index("{", start)
depth = 0
for i in range(brace, len(src)):
if src[i] == "{":
depth += 1
elif src[i] == "}":
depth -= 1
if depth == 0:
return src[brace:i + 1]
raise AssertionError("unterminated function")
set_body = body(set_src, "pub fn is_registered_set(addr: usize)")
map_body = body(map_src, "pub fn is_registered_map(addr: usize)")
regex_body = body(regex_src, "pub(crate) fn is_regex_pointer(ptr: *const u8)")
assert "header.obj_type == crate::gc::GC_TYPE_SET" in set_body
assert "header.obj_type == crate::gc::GC_TYPE_MAP" in map_body
assert "regex_header_has_magic" in regex_body
assert "regex_pointers_contains(ptr as usize)" in regex_body
# The fallback registry path has no header/type check after the magic probe.
fallback = regex_body.split("regex_header_has_magic", 1)[1]
assert "regex_pointers_contains(ptr as usize)" in fallback
assert "GC_TYPE_OBJECT" not in fallback
# The regex address registry is append-only in the source.
assert "REGEX_POINTERS.with" in regex_src
assert ".insert(ptr as usize)" in regex_src
assert not re.search(r"REGEX_POINTERS\.with\(.*?remove\(", regex_src, re.S)
print("set probe enforces GC_TYPE_SET: yes")
print("map probe enforces GC_TYPE_MAP: yes")
print("regex magic probe enforces GC_TYPE_OBJECT: yes")
print("regex fallback registry probe enforces GC_TYPE_OBJECT: no")
print("regex registry removal path found: no")
print("Conclusion: the regex implication is not established for stale/reused addresses.")
PYRepository: PerryTS/perry
Length of output: 447
Fix the is_regex_pointer fallback type check.
The fallback accepts append-only REGEX_POINTERS entries without checking GcHeader.obj_type. A reused RegExp address can therefore be treated as a RegExp and passed to object_ptr_from_value as the wrong pointer type.
Validate GC_TYPE_OBJECT on the fallback path or remove stale entries.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/object/native_call_method.rs` around lines 946 -
962, The fallback in the object-type match must only classify pointers as
regexes when the GC header reports GC_TYPE_OBJECT. Update the is_regex_pointer
handling in the obj_type match, or ensure stale REGEX_POINTERS entries are
removed, so reused addresses cannot be accepted as RegExp pointers with another
object type.
Comment on lines
+150
to
+157
| // The realistic leaked-symbol path — what a `for…of` mints. It carries no | ||
| // GcHeader, so only the magic screen can keep it out of the object arms. | ||
| let wk = crate::symbol::well_known_symbol("iterator") as usize; | ||
| assert!( | ||
| unsafe { crate::symbol::may_be_symbol_header(wk as *const u8) }, | ||
| "a well-known symbol must carry SYMBOL_MAGIC in its first word; if it \ | ||
| does not, `Symbol.iterator.toString()` reads `ptr - 8` as a GcHeader" | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Record why classify(wk) is deliberately not asserted.
Every other case in this test asserts classify(...).is_none(). The well-known symbol case asserts only the magic bytes. The reason is thread-scoped and important: the module doc at Lines 31-36 states WELL_KNOWN_SYMBOLS is a process-global cache while SYMBOL_POINTERS is per-thread. If another test thread creates Symbol.iterator first, this thread receives the cached pointer and is_registered_symbol returns false. classify(wk) would then read ptr - 8 on a Box allocation and return Some(...).
A future contributor may add the missing classify assertion and produce a test that fails only under thread interleaving. Add an inline comment stating that the classify assertion is omitted on purpose.
📝 Proposed comment addition
// The realistic leaked-symbol path — what a `for…of` mints. It carries no
// GcHeader, so only the magic screen can keep it out of the object arms.
+ //
+ // Deliberately NOT asserting `classify(wk).is_none()`: `WELL_KNOWN_SYMBOLS`
+ // is process-global while `SYMBOL_POINTERS` is per-thread, so under
+ // `cargo test` this thread may receive a pointer another thread registered
+ // and `is_registered_symbol` would answer `false`. Only the
+ // thread-independent property — the magic in the first word — is asserted.
let wk = crate::symbol::well_known_symbol("iterator") as usize;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // The realistic leaked-symbol path — what a `for…of` mints. It carries no | |
| // GcHeader, so only the magic screen can keep it out of the object arms. | |
| let wk = crate::symbol::well_known_symbol("iterator") as usize; | |
| assert!( | |
| unsafe { crate::symbol::may_be_symbol_header(wk as *const u8) }, | |
| "a well-known symbol must carry SYMBOL_MAGIC in its first word; if it \ | |
| does not, `Symbol.iterator.toString()` reads `ptr - 8` as a GcHeader" | |
| ); | |
| // The realistic leaked-symbol path — what a `for…of` mints. It carries no | |
| // GcHeader, so only the magic screen can keep it out of the object arms. | |
| // | |
| // Deliberately NOT asserting `classify(wk).is_none()`: `WELL_KNOWN_SYMBOLS` | |
| // is process-global while `SYMBOL_POINTERS` is per-thread, so under | |
| // `cargo test` this thread may receive a pointer another thread registered | |
| // and `is_registered_symbol` would answer `false`. Only the | |
| // thread-independent property — the magic in the first word — is asserted. | |
| let wk = crate::symbol::well_known_symbol("iterator") as usize; | |
| assert!( | |
| unsafe { crate::symbol::may_be_symbol_header(wk as *const u8) }, | |
| "a well-known symbol must carry SYMBOL_MAGIC in its first word; if it \ | |
| does not, `Symbol.iterator.toString()` reads `ptr - 8` as a GcHeader" | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/object/native_call_method/probe_dispatch_tests.rs`
around lines 150 - 157, Add an inline comment immediately before the well-known
symbol assertion explaining that classify(wk) is intentionally not asserted
because WELL_KNOWN_SYMBOLS is process-global while SYMBOL_POINTERS is
thread-local; when the pointer is created on another thread, classify may not
recognize it and could inspect ptr - 8 unsafely. Keep the existing magic-byte
assertion unchanged.
Comment on lines
+165
to
+167
| let pattern = crate::string::js_string_from_str("a+b"); | ||
| let flags = crate::string::js_string_from_str("g"); | ||
| let re = crate::regex::js_regexp_new(pattern, flags) as usize; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Root pattern across the second string allocation.
Line 165 allocates pattern on the GC heap. Line 166 allocates flags, and that allocation can trigger a collection that relocates pattern. Line 167 then passes the possibly stale pattern into js_regexp_new. Raw Rust pointer locals are neither GC roots nor pins, so the value is not protected across the second allocation.
Allocate both strings inside a crate::gc::RuntimeHandleScope and reload pattern from its handle before the js_regexp_new call.
Based on learnings: "In PerryTS production GC, Rust stack locals are not conservatively scanned (SkipDisabled), and raw Rust pointer locals are neither GC roots nor reliable pins... root the value using crate::gc::RuntimeHandleScope and reload it from the rewritten handle... before any subsequent reuse."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/object/native_call_method/probe_dispatch_tests.rs`
around lines 165 - 167, Update the test setup around js_regexp_new to create a
crate::gc::RuntimeHandleScope, allocate pattern and flags through rooted
handles, and reload pattern from its handle immediately before calling
js_regexp_new. Ensure pattern remains rooted across the flags allocation and no
stale raw pointer is reused.
Source: Learnings
Comment on lines
+246
to
+264
| for i in 0..8 { | ||
| let sym = leaked_symbol(&format!("perry-7850-magic-{i}")); | ||
| let obj_type = unsafe { | ||
| (*((sym as *const u8).sub(crate::gc::GC_HEADER_SIZE) as *const crate::gc::GcHeader)) | ||
| .obj_type | ||
| }; | ||
| let excluded_without_the_screen = match obj_type { | ||
| crate::gc::GC_TYPE_SET => crate::set::is_registered_set(sym), | ||
| crate::gc::GC_TYPE_MAP => crate::map::is_registered_map(sym), | ||
| crate::gc::GC_TYPE_OBJECT => crate::regex::is_regex_pointer(sym as *const u8), | ||
| _ => false, | ||
| }; | ||
| assert!( | ||
| !excluded_without_the_screen, | ||
| "leaked symbol {sym:#x} (allocator bytes read as obj_type {obj_type}) would \ | ||
| be excluded even without the magic screen — the screen is then not \ | ||
| load-bearing and this suite is vacuous" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for Miri or sanitizer jobs that would build perry-runtime tests.
set -euo pipefail
fd -t f -e yml -e yaml . .github 2>/dev/null | while IFS= read -r f; do
if rg -qi 'miri|sanitizer|-Zsanitizer|asan|valgrind' "$f"; then
echo "== $f =="
rg -ni -C 4 'miri|sanitizer|-Zsanitizer|asan|valgrind' "$f"
fi
done
echo "== cargo config / toolchain sanitizer flags =="
fd -t f 'config.toml|rust-toolchain.toml|rust-toolchain' . --hidden | while IFS= read -r f; do
echo "-- $f --"; cat "$f"
doneRepository: PerryTS/perry
Length of output: 1435
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="crates/perry-runtime/src/object/native_call_method/probe_dispatch_tests.rs"
printf '%s\n' '== relevant test code =='
sed -n '220,275p' "$file"
printf '%s\n' '== repository-wide Miri and sanitizer references =='
rg -n -i --hidden \
-g '!target' -g '!node_modules' \
'miri|sanitizer|-Zsanitizer|addresssanitizer|asan|valgrind|RUSTFLAGS|CARGO_TARGET[^ ]*SANITIZE' . \
|| true
printf '%s\n' '== Box allocation and symbol helper definitions/usages =='
rg -n -C 5 'fn leaked_symbol|leaked_symbol|Box::into_raw|Box<' \
crates/perry-runtime/src/object/native_call_method/probe_dispatch_tests.rs \
crates/perry-runtime/src \
|| trueRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== sanitizer roadmap context =='
sed -n '175,210p' docs/memory-perf-roadmap.md
sed -n '350,372p' docs/generational-gc-plan.md
printf '%s\n' '== workflow jobs that run perry-runtime tests =='
rg -n -i -C 3 \
'cargo (test|nextest)|perry-runtime|workspace|sanitizer|miri' \
.github/workflows \
| rg -i -C 3 'perry-runtime|cargo test|nextest|sanitizer|miri' \
| head -n 240
printf '%s\n' '== Box-backed symbol allocation =='
sed -n '260,305p' crates/perry-runtime/src/symbol.rs
sed -n '430,458p' crates/perry-runtime/src/symbol.rsRepository: PerryTS/perry
Length of output: 24336
Replace the raw allocator-metadata read. js_symbol_for returns a Box allocation without a GcHeader, so reading sym - GC_HEADER_SIZE is undefined behavior and fails under Miri or AddressSanitizer. No current CI job enables either tool, but planned sanitizer runs would expose this test failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/object/native_call_method/probe_dispatch_tests.rs`
around lines 246 - 264, Replace the unsafe GC_HEADER_SIZE-based metadata read in
the leaked-symbol validation loop with a defined way to determine each symbol’s
relevant type or registration status. Preserve the assertion’s purpose of
proving these symbols are not independently excluded by set, map, or regex
checks, without dereferencing memory before the Box allocation returned by
js_symbol_for.
proggeramlug
pushed a commit
that referenced
this pull request
Aug 11, 2026
This was referenced Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #7850 — with a null perf result and the reason for it. Read the Validation
section before the rest: the issue's 6.5% is gone, and it is not this PR that removed it.
TL;DR
gc_pointer_and_type_from_valuenow consults one side registry instead of four onthe common path, and never the process-global symbol mutex. On the current corpus that is
worth nothing measurable — 21 programs, all within +/-0.7%, which is the noise floor —
because #7852 already removed the dynamic-dispatch load the probe was riding
(
pipeline0.483 s -> 0.274 s). #7850 said this might happen; it did.What the measuring found instead is that the family did not go away, it moved: on
pipeline_bigevery remainingis_registered_symbol_slowsample now comes from theproperty-get IC-miss tail, identically on both arms. Filed as #7867.
So: merge this for the structural property and the tests that lock it in, or close it —
but please do not merge it believing it is a speedup. It is not, today.
What this changes
object::native_call_method::gc_pointer_and_type_from_valuesits on the path ofevery dynamic method call (
js_native_call_method→class_vtable_fast_guard).It ran four address-keyed side-registry probes —
set::is_registered_set,map::is_registered_map,regex::is_regex_pointer,symbol::is_registered_symbol—purely to exclude object kinds, and only then read the
GcHeaderthat already recordsthe kind three of them were looking for.
The symbol one is the expensive one: a process-global
pthread_mutexplus a SipHashover a
HashSet<usize>. It already has aRegistryLatchand the latch is correct — itis just armed by almost every realistic program, because
well_known_symbol()iswhat materialises
Symbol.iteratorand that is what afor…oflowering reaches for. Alatch a program arms in its first loop is not protection; it only moves the cost behind
a branch that is always taken.
The header now selects the probe. Every implication is enforced by the probe itself, so
this is a re-ordering rather than a new assumption:
set::is_registered_setobj_type == GC_TYPE_SET(set.rs:262)map::is_registered_mapobj_type == GC_TYPE_MAP(map.rs:244)regex::is_regex_pointergc_malloc(_, GC_TYPE_OBJECT);js_regexp_newis the soleREGEX_POINTERSinsertSymbolof any storageSYMBOL_MAGICin its own first wordA
GC_TYPE_OBJECTreceiver — the overwhelmingly common case — now consults oneregistry (regex, which genuinely is a
GC_TYPE_OBJECTallocation) instead of four, andnever the symbol mutex.
The hole the header cannot cover, and a design that was refuted on the way
symbol.rshas five registration sites and they do not agree on storage. Three ofthem —
well_known_symbol,intl_legacy_constructed_symbol,js_symbol_for— areBox::into_raw: process-lifetime allocations with noGcHeaderat all, soptr - 8is foreign allocator bytes that can read as anyobj_type. Trusting theheader for those is exactly the #7846 shape — a proof that is true at one site and
assumed everywhere.
The first design screened them by address: a monotone
(lo, hi)window over theleaked-symbol addresses, two atomic loads,
falseexact. Its own invariant testrefuted it. The filtered test run was green; the full one printed
One outlier
Boxwidens an address range to span the arena and the fast path silentlystops firing — still sound, worth nothing. An address range over allocator-chosen
addresses is not a screen, and this is precisely the failure mode CLAUDE.md's "a gate
must assert its subject was live" is about: the optimisation would have shipped, been
green, and done nothing.
What every symbol does have, whatever its storage, is
SYMBOL_MAGICin its own firstfour bytes —
alloc_symboland all threeBoxsites set it, and the field is at offset0 precisely so cheap discrimination is possible.
symbol::may_be_symbol_header(ptr)isone 4-byte load of the object the caller is already about to inspect.
falseisexact (no symbol reads
false); a falsetruemerely pays the old probe and gets theold answer. It cannot be defeated by allocator placement, and it covers GC-heap and
leaked symbols with one test — so the
AddressWindowand theSymbolStorageenum itneeded were both deleted rather than left in the tree as unexercised machinery.
Tests — a counter, a sabotage, and a performance invariant
probe_dispatch_tests::plain_object_dispatch_probes_no_side_registryasserts thesaving instead of assuming it: with the symbol latch armed, a plain-object
dispatch must not move the symbol / map / set probe counters. Delete the
obj_typedispatch and it goes red. (New
symbol::TEST_SYMBOL_REGISTRY_PROBEScounter, thesame
#[cfg(test)]idiommap.rs,set.rsandarguments.rsalready use.)header_directed_dispatch_needs_the_symbol_magic_screenis a sabotage test: withthe screen defeated, the dispatch must fall back into
is_registered_symbol— andstill give the same answer. A future edit that drops the screen cannot leave the suite
quietly green.
the_magic_screen_covers_every_symbol_and_no_ordinary_objectpins both halves:soundness (every leaked and
gc_malloc'd symbol carries the magic, and — mirroringthe production
matchdeliberately — no other arm would exclude a leaked symbol,so the screen is the only thing keeping them out) and the performance invariant (0/64
fresh GC objects may read as the magic). The second assertion is what refuted the
address-window design above.
exotic_receivers_are_still_excluded/regexp_receiver_is_still_excluded— theanswer is unchanged for Set / Map / RegExp / fresh
Symbol()/ leaked symbol,including one created after the idle fast path already ran (perf(runtime): cache the hot thread-locals so one allocation pays one _tlv_get_addr #7474 shape).
Validation — quiet M1 mini, absolute seconds, best-of-7, exit-checked
Both arms built from the same tree with only
perry-runtimediffering. The baseline isprovably pristine: the first fix build printed
Compiling perry-runtime, i.e. cargohad to recompile it, which is only true if the baseline archive predates the edit.
(mtimes could not settle it — the baseline rlib landed 17:58:32 and the first source edit
was 17:59:20.) Baseline
bf98134ba. One batched lock window, load 1.91 before / 2.05after, zero foreign benchmark processes at both ends, outputs re-verified on the mini
against recorded checksums before timing.
That is a null. The signs are scattered, the two largest cells are the two shortest
programs (
churn_readat 0.022 s,push_numat 0.069 s), andbestandmeddisagreein sign on four rows. Nothing here is a win and nothing is a regression.
dyncallanddynmixare new, written specifically for this change: a base-typedpolymorphic tree-walk and a mixed object/array/
Mapreceiver loop, both with afor...ofso the symbol latch is armed the way a real program arms it.
--trace llvmconfirms 20call @js_native_call_method_by_idsites indyncall's recursive inner loop, so thecallsites really are on the dynamic path — codegen's inline guard simply resolves them and
the runtime tower stays cold.
Why: the subject stopped being hot, and where it went
gc-handoff/bench/pipeline_big.ts,PERRY_DEBUG_SYMBOLS=1,sampleat 1 ms(dev machine, so attribution only, no timing claim):
is_registered_symbol_slowjs_object_get_field_ic_miss->get_field_by_name_tailZero samples on either arm come from
gc_pointer_and_type_from_value. #7850 measured6.5% there before #7852; that dispatch load is gone, and the residual symbol probing moved
to the property-get miss path — a different function, filed as #7867.
Correctness
node --experimental-strip-typeswith exit 0, on both arms, verified before timingand again on the mini after shipping.
perry-runtimeunit tests: 2123 passed, 0 failed (RUST_TEST_THREADS=1, docs: perry-runtime's tests must run single-threaded locally, as CI already does #7791).gc-handoff/apps/iso_miss.ts->checksum 437840 misses 0, including underPERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800 PERRY_GC_VERIFY_EVACUATION=1and underPERRY_GC_SCHEDULE_RATE=1 PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1(PERRY_GC_ZEALno longerexists, Remove the older collector stress-test setting, now that the newer one covers it #7741).
cargo fmt --all -- --check,scripts/check_file_size.shandscripts/addr_class_inventory.pyall clean.Why merge a change that measures zero
Three reasons, and none of them is "it might be faster":
instead of four, no process-global mutex — and the diff is a re-ordering whose every
implication is enforced by the probe it replaces.
is_registered_symbolalone takes a mutex + SipHash (6.5% ofpipeline) #7850's 6.5% appeared because alatch that looked protective is armed by the first
for...of. The counter test makes"a plain-object dispatch touches no side registry" an assertion instead of a hope, so
the next program with real megamorphic dispatch does not quietly re-pay it.
paying for: an optimisation that is sound, green, and does nothing. It was caught here
by a test rather than by a profile six weeks later, and that test stays.
Refuted while scoping — #7850 named three sightings, two were already closed
visit_object_static_prototype_slot_mut's mutex + SipHash per traced object wasfixed by perf(gc): cut the copying minor's per-promoted-object cost (retain 0.351 -> 0.269, 87.6 -> 66.5 ns/object) #7859;
prototype_chain.rs:390already opens with theOBJECT_PROTOTYPES_NONEMPTYgate and carries theretain.tscomment the issuequotes.
interp'sis_registered_set/is_registered_map/is_arguments_objectarealready latched by perf(runtime): allocation path spends 34% of self time in _tlv_get_addr — 24× behind Node on object churn with the collector already idle #7469 and perf(codegen,runtime): inline precheck for boxed class-field stores, arguments-registry emptiness latch, declared-type refinement for property reads (interp 1.236 -> 1.097, iso_miss 1.670 -> 1.465) #7854;
PROFILE-interp-round3.md's shares predate both.Follow-up found while measuring (not in this PR)
sampleonapps/interpputs the residualset::is_registered_setunderjs_dyn_index_get, not undergc_pointer_and_type_from_value:value/dyn_index.rs:232and:544runis_registered_set(raw_ptr) || is_registered_map(raw_ptr)on every dynamic index read, ~90 lines ahead of theGcHeaderread the same function performs — the identical shape to #7765'sjs_array_lengthfix. Different function, different risk surface; filed as #7865.Sibling follow-up #7867 — the property-get IC-miss tail, which is where the profile
says this family actually lives today.
Summary by CodeRabbit