perf(runtime): class dispatch and instanceof stop consulting locked hash maps — shapes.ts 0.28s → 0.23s - #7769
Conversation
|
Warning Review limit reached
Next review available in: 42 seconds You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe runtime adds dense parent metadata, latched registry lookups, a thread-local class-dispatch cache, generation invalidation, stack-backed vtable arguments, and regression tests for inheritance, dispatch, ChangesClass dispatch runtime
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant js_native_call_method
participant obj_dispatch_ic
participant vtable
participant class_method
js_native_call_method->>obj_dispatch_ic: Lookup class ID and method name
obj_dispatch_ic-->>js_native_call_method: Return cached function and metadata
js_native_call_method->>vtable: Invoke cached target
vtable->>class_method: Pass receiver and arguments
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
…ash maps (#7769) The class parent chain becomes a dense atomic mirror instead of a process-global RwLock<HashMap>; the hasInstance / toStringTag / extends-Error / fetch-parent / generic-origin / class-static-symbol / timer-id registries get monotone latches; the dispatch tower caches its own per-(class, method name) resolution behind a re-checked receiver-shape guard; and vtable argument marshalling drops two Vec allocations per dynamic call. shapes.ts 0.28s -> 0.23s on the pinned quiet mini, no protected floor crossed.
c0fab58 to
9d6a18e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
crates/perry-runtime/src/object/class_meta_registry.rs (3)
76-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the doc comment with the actual memory ordering.
The doc says the in-window read is "one relaxed-ordering atomic load". Line 85 uses
Ordering::Acquire. In a lock-free publish/consume pair the stated ordering is load-bearing documentation:parent_dense_storepublishes withRelease, so the reader must acquire to pair with it. A future reader who trusts the comment could downgrade the load toRelaxedand break the pairing.📝 Proposed doc fix
-/// In-window ids answer from one relaxed-ordering atomic load. Everything else -/// (builtin reserved bands, synthetic high-bit ids) falls back to the locked -/// map, exactly as before. +/// In-window ids answer from one acquire-ordered atomic load, which pairs with +/// the `Release` store in [`parent_dense_store`]. Everything else (builtin +/// reserved bands, synthetic high-bit ids) falls back to the locked map, +/// exactly as before.🤖 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/class_meta_registry.rs` around lines 76 - 95, Update the doc comment for get_parent_class_id to state that in-window IDs are read with one acquire-ordering atomic load, matching PARENT_DENSE[idx].load(Ordering::Acquire) and the Release publication in parent_dense_store; leave the implementation unchanged.
372-379: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test can fail because of a sibling test, not because of a regression.
assert!(PARENT_DENSE_INCOMPLETE.is_idle())reads process-global state. Cargo runs the unit tests of this crate in one process. Any test that callsregister_class(child, u32::MAX)with an in-window child arms the latch permanently and fails this assertion, with a message that does not name the real cause.The dense-mirror behavior this test targets does not need the global assertion: a zero slot plus an idle latch is what makes the answer
None, and the followingassert_eq!already covers the observable result. Either drop the global assertion or make it a documented precondition skip.♻️ Proposed change
- assert!(PARENT_DENSE_INCOMPLETE.is_idle()); - assert_eq!(get_parent_class_id(60_050), None); + // `PARENT_DENSE_INCOMPLETE` is process-global; a sibling test that + // registers a `u32::MAX` parent would arm it for the whole process. + // Only assert the observable answer when the precondition holds. + if PARENT_DENSE_INCOMPLETE.is_idle() { + assert_eq!(get_parent_class_id(60_050), None); + }🤖 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/class_meta_registry.rs` around lines 372 - 379, Remove the process-global PARENT_DENSE_INCOMPLETE.is_idle() assertion from an_unregistered_in_window_id_answers_none_without_touching_the_map, leaving the get_parent_class_id(60_050) result assertion to validate the observable behavior without depending on sibling test state.
97-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused test-only helper.
parent_dense_clearhas no callers and can triggerdead_code. Remove it unless a test actually needsPARENT_DENSEreset.🤖 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/class_meta_registry.rs` around lines 97 - 104, Remove the unused test-only parent_dense_clear function and its associated reset logic from the class metadata registry; leave PARENT_DENSE and other registry behavior unchanged.Source: Learnings
crates/perry-runtime/src/object/native_call_method.rs (1)
36-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate this doc block; it currently contains two merged docs.
The comment describes the guard twice and the second copy starts mid-sentence:
- Lines 59-71 list the guard conditions. Lines 83-99 list them again with more detail. A reader must diff two lists to learn whether they agree.
- Line 73 ends the first doc ("A miss (
None) is always safe: the caller falls through to the full tower."), then line 74 begins a sentence fragment ("The receiver-shape predicateGshared by the fast path and by the sites that are allowed to populate its cache.") with no verb. The first half reads like the doc fortry_class_vtable_fast_dispatch, which is defined at line 195.Gis never defined anywhere in the file.This is the soundness argument for the whole fast path, so keep one authoritative list. Move the "why a cache hit is sound" and "a miss is always safe" paragraphs onto
try_class_vtable_fast_dispatch, keep the detailed condition list here, and name the predicate instead of usingG.🤖 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 36 - 99, Consolidate the dispatch documentation by removing the duplicated guard list and the incomplete `G` sentence from the current block. Keep the detailed receiver-shape conditions here, replace the undefined predicate reference with an explicit description or named predicate, and move the “why a cache hit is sound” plus “a miss is always safe” rationale to `try_class_vtable_fast_dispatch`.
🤖 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 `@changelog.d/7769-class-dispatch-locked-hash-maps.md`:
- Around line 31-40: Update the changelog heading to accurately describe all
seven metadata registries listed in the paragraph, and complete the dangling
possessive phrase near the end so it reads as a grammatically complete sentence.
Preserve the existing registry names and technical details.
In `@crates/perry-runtime/src/object/class_registry/dispatch.rs`:
- Around line 587-595: Scope the insert-and-lookup precondition in test
a_class_registration_invalidates_every_entry with with_stable_gen, while keeping
test_bump_vtable_generation() and the subsequent miss assertion outside that
stability window. In crates/perry-runtime/src/object/class_meta_registry.rs
lines 372-379, guard the get_parent_class_id(60_050) assertion with
PARENT_DENSE_INCOMPLETE.is_idle() rather than asserting it unconditionally.
- Around line 252-276: Update the VTABLE_GEN load in obj_dispatch_ic_lookup to
use Ordering::Acquire instead of Ordering::Relaxed, preserving the existing
generation comparison and cache lookup behavior.
---
Nitpick comments:
In `@crates/perry-runtime/src/object/class_meta_registry.rs`:
- Around line 76-95: Update the doc comment for get_parent_class_id to state
that in-window IDs are read with one acquire-ordering atomic load, matching
PARENT_DENSE[idx].load(Ordering::Acquire) and the Release publication in
parent_dense_store; leave the implementation unchanged.
- Around line 372-379: Remove the process-global
PARENT_DENSE_INCOMPLETE.is_idle() assertion from
an_unregistered_in_window_id_answers_none_without_touching_the_map, leaving the
get_parent_class_id(60_050) result assertion to validate the observable behavior
without depending on sibling test state.
- Around line 97-104: Remove the unused test-only parent_dense_clear function
and its associated reset logic from the class metadata registry; leave
PARENT_DENSE and other registry behavior unchanged.
In `@crates/perry-runtime/src/object/native_call_method.rs`:
- Around line 36-99: Consolidate the dispatch documentation by removing the
duplicated guard list and the incomplete `G` sentence from the current block.
Keep the detailed receiver-shape conditions here, replace the undefined
predicate reference with an explicit description or named predicate, and move
the “why a cache hit is sound” plus “a miss is always safe” rationale to
`try_class_vtable_fast_dispatch`.
🪄 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: ec378b05-a159-4d47-96d1-68363b5e5355
📒 Files selected for processing (14)
changelog.d/7769-class-dispatch-locked-hash-maps.mdcrates/perry-runtime/src/object/class_meta_registry.rscrates/perry-runtime/src/object/class_registry.rscrates/perry-runtime/src/object/class_registry/dispatch.rscrates/perry-runtime/src/object/class_registry/parent_static.rscrates/perry-runtime/src/object/class_registry/prototype_methods.rscrates/perry-runtime/src/object/instanceof.rscrates/perry-runtime/src/object/native_call_method.rscrates/perry-runtime/src/object/native_call_method/handle_methods.rscrates/perry-runtime/src/symbol.rscrates/perry-runtime/src/symbol/properties.rscrates/perry-runtime/src/timer.rstest-files/test_gap_7769_class_dispatch_shapes.tstest-files/test_issue_7769_thread_class_dispatch.ts
| #### Five metadata registries got monotone latches | ||
|
|
||
| `Symbol.hasInstance` hooks, `Symbol.toStringTag` hooks, `extends Error`, the | ||
| fetch-builtin parent kind, the generic-origin table, the class static-symbol | ||
| table, and the timer-id registry are all empty in a program that does not use | ||
| those features — but `js_instanceof` probed three of them on every evaluation, | ||
| and `class_chain_reaches` probed one on every hop. They now use | ||
| `registry_latch::RegistryLatch` (#7755), so an unused feature answers from one | ||
| atomic load. The `Symbol.hasInstance` latch also keeps the string-keyed | ||
| `well_known_symbol("hasInstance")` interning probe off the path entirely. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the registry count and the truncated heading.
Two user-facing text defects:
- Line 31 says "Five metadata registries got monotone latches", but lines 33-35 enumerate seven:
Symbol.hasInstancehooks,Symbol.toStringTaghooks,extends Error, the fetch-builtin parent kind, the generic-origin table, the class static-symbol table, and the timer-id registry. Five of those latches are added incrates/perry-runtime/src/object/class_meta_registry.rs; the class static-symbol and timer-id latches live incrates/perry-runtime/src/symbol.rsandcrates/perry-runtime/src/timer.rs. - Line 139 ends with a dangling possessive: "why it is not this change's".
📝 Proposed text fixes
-#### Five metadata registries got monotone latches
+#### Seven metadata registries got monotone latches-#### What the remaining lock traffic is, and why it is not this change's
+#### What the remaining lock traffic is, and why it is not this change's problem🤖 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/7769-class-dispatch-locked-hash-maps.md` around lines 31 - 40,
Update the changelog heading to accurately describe all seven metadata
registries listed in the paragraph, and complete the dangling possessive phrase
near the end so it reads as a grammatically complete sentence. Preserve the
existing registry names and technical details.
| if name.is_empty() || name.len() > OBJ_DISPATCH_IC_NAME_MAX { | ||
| return None; | ||
| } | ||
| let cur_gen = VTABLE_GEN.load(Ordering::Relaxed); | ||
| let slot = obj_dispatch_ic_slot(class_id, name); | ||
| OBJ_DISPATCH_IC.with(|cell| { | ||
| // SAFETY: the cache is thread-local and never handed out by reference | ||
| // across a call that could re-enter this module. | ||
| let entry = unsafe { &(**cell.get())[slot] }; | ||
| if entry.gen == cur_gen | ||
| && entry.class_id == class_id | ||
| && entry.name_len as usize == name.len() | ||
| && entry.name[..name.len()] == *name | ||
| { | ||
| Some(( | ||
| entry.func_ptr, | ||
| entry.param_count, | ||
| entry.has_synthetic_arguments != 0, | ||
| entry.has_rest != 0, | ||
| )) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Map every read and write of VTABLE_GEN and its memory ordering.
set -euo pipefail
# Test: locate all VTABLE_GEN accesses with their Ordering arguments.
rg -n -C4 'VTABLE_GEN' --type=rust
# Test: confirm whether prototype registration is reachable off the main thread.
rg -n -C4 'js_register_prototype_method|class_prototype_method_root_store' --type=rustRepository: PerryTS/perry
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git status =="
git status --short || true
echo "== Rust files matching dispatch/prototype =="
git ls-files '*.rs' | rg 'perry-runtime/src/object/class_registry/(dispatch|prototype_methods)\.rs|thread' || true
echo "== VTABLE_GEN accesses =="
rg -n -C4 'VTABLE_GEN' --type=rust . || true
echo "== Prototype registration symbols =="
rg -n -C4 'js_register_prototype_method|class_prototype_method_root_store|invalidate_class_prototype_fast_guards|register_mutable_root_scanner' --type=rust . || true
echo "== Thread/public threading APIs =="
rg -n -C3 'thread::|sc_thread|pthread|JsContext|executor|spawn|threading' --type=rust crates perry || trueRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Candidate files =="
git ls-files 'crates/perry-runtime/src/**' | sed -n '1,200p'
echo "== dispatch outline =="
ast-grep outline crates/perry-runtime/src/object/class_registry/dispatch.rs --view expanded || true
echo "== prototype_methods outline =="
ast-grep outline crates/perry-runtime/src/object/class_registry/prototype_methods.rs --view expanded || trueRepository: PerryTS/perry
Length of output: 11688
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Exact object/class_registry files =="
git ls-files 'crates/perry-runtime/src/object/class_registry/*'
echo "== dispatch.rs relevant sections =="
sed -n '1,150p' crates/perry-runtime/src/object/class_registry/dispatch.rs | cat -n
sed -n '230,310p' crates/perry-runtime/src/object/class_registry/dispatch.rs | cat -n
sed -n '536,620p' crates/perry-runtime/src/object/class_registry/dispatch.rs | cat -n
echo "== prototype_methods.rs relevant sections =="
sed -n '1,110p' crates/perry-runtime/src/object/class_registry/prototype_methods.rs | cat -n
sed -n '110,180p' crates/perry-runtime/src/object/class_registry/prototype_methods.rs | cat -n
echo "== VTABLE_GEN/readers narrow =="
rg -n -C2 'vtble|V_TABLE|VTABLE' --type=rust crates/perry-runtime/src/object/class_registry crates/perry-runtime/src/object crates/perry-runtime/src/closure crates/perry-runtime/src/promise crates/perry-runtime/src/thread crates/perry-runtime/src/promise/mod.rs 2>/dev/null || true
echo "== prototype registration narrow =="
rg -n -C3 'js_register_prototype_method|class_prototype_method_root_store|class_prototype_fast_guards_invalidated|invalidate_class_prototype_fast_guards' --type=rust crates/perry-runtime/src crates/perry-codegen/src 2>/dev/null | sed -n '1,240p'
echo "== thread symbols narrow =="
rg -n -C2 'js_thread|perry/thread|std::thread::spawn|thread::spawn|worker|Worker|JsThread|Thread|RUNNER|main_thread|js_thread_spawn|js_thread_process_pending' --type=rust crates/perry-runtime/src crates/perry 2>/dev/null | sed -n '1,260p'Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Candidate files =="
git ls-files 'crates/perry-runtime/src/**' | sed -n '1,200p'
echo "== dispatch outline =="
ast-grep outline crates/perry-runtime/src/objecticlass_registry/dispatch.rs --view expanded || true
echo "== prototype_methods outline =="
ast-grep outline crates/perry-runtime/src/object/class_registry/prototype_methods.rs --view expanded || trueRepository: PerryTS/perry
Length of output: 9854
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== perry_thread_local macro =="
rg -n -C8 'macro_rules! perry_thread_local|perry_thread_local!' crates/perry-runtime/src crates --type=rust | sed -n '1,260p'
echo "== thread spawn symbols narrow =="
rg -n -C4 '\bjs_thread_spawn\b|\bjsthr\b|js_thread_process_pending|perry/thread|JsThread|Worker|std::thread::spawn|thread::spawn' crates --type=rust | sed -n '1,320p'
echo "== registration calls narrow =="
rg -n -C4 '\bregister_class_method\b|\bjs_register_class_method\b|\bclass_own_method\b|\bclass_method\b' crates --type=rust | sed -n '1,260p'
echo "== prototype write callers narrow =="
rg -n -C4 'js_register_prototype_method\(|class_prototype_method_root_store\(' crates --type=rust | sed -n '1,240p'
echo "== exact generation reader locations with methods/classes around =="
python3 - <<'PY'
import pathlib, re
root=pathlib.Path('crates/perry-runtime/src')
for p in root.rglob('*.rs'):
txt=p.read_text(errors='ignore')
idx=0
while True:
idx=txt.find('Ordering::Relaxed', idx)
if idx==-1: break
m=txt.strip().split()[:200]
print(p, idx, txt[max(0,idx-60):idx+70].replace('\n','\\n'))
idx+=1
PYRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== perry_thread_local macro definition location =="
rg -n 'macro_rules! perry_thread_local|const perry_thread_local|macro.*thread_local|thread_local!|use std::thread::' crates --type=rust perry-runtime crates/perry-runtime/src state.rs lib.rs 2>/dev/null | sed -n '1,260p'
echo "== macro definition likely crate root files =="
git ls-files | rg '(^|/)lib\.rs$|state\.rs$|macros\.rs$|prelude\.rs$' | sed -n '1,200p'
echo "== macro search exact text =="
rg -n 'macro_rules! perry_thread_local' . 2>/dev/null || true
echo "== std::thread::spawn from crates/perry-runtime and perry-stdlib =="
rg -n -C3 'std::thread::spawn'|rg -n -C3 'thread::spawn|spawn\(' crates/perry-runtime crates/perry-stdlib --type=rust | sed -n '1,260p'Repository: PerryTS/perry
Length of output: 17982
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== macro definition source exact =="
rg -n 'macro_rules! perry_thread_local' --glob '*.rs' .
echo "== macro definition with context =="
FILE="$(rg -l 'macro_rules! perry_thread_local' --glob '*.rs' . | head -n 1 || true)"
if [ -n "${FILE:-}" ]; then
LINE="$(rg -n 'macro_rules! perry_thread_local' "$FILE" | head -n 1 | cut -d: -f1)"
START=$(( LINE > 80 ? LINE - 80 : 1 ))
END=$(( LINE + 140 ))
sed -n "${START},${END}p" "$FILE" | cat -n
fi
echo "== thread spawn files in perry-runtime/stdlib =="
rg -n -C2 '\bstd::thread::spawn\b|\bthread::spawn\b|\bcrate::thread::spawn\b|\bstd::thread::Builder\b' crates/perry-runtime crates/perry-stdlib --type=rust | sed -n '1,260p'Repository: PerryTS/perry
Length of output: 30168
Use Ordering::Acquire when reading VTABLE_GEN.
VTABLE_GEN.fetch_add(1, Ordering::Release) in invalidate_class_prototype_fast_guards is the invalidation barrier; relaxed loads in vtable_ic_lookup and obj_dispatch_ic_lookup do not form the required acquire-release pair. Load the generation with Acquire before comparing cache entries so prototype mutations retire per-thread dispatch caches.
🤖 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/class_registry/dispatch.rs` around lines 252
- 276, Update the VTABLE_GEN load in obj_dispatch_ic_lookup to use
Ordering::Acquire instead of Ordering::Relaxed, preserving the existing
generation comparison and cache lookup behavior.
| #[test] | ||
| fn a_class_registration_invalidates_every_entry() { | ||
| obj_dispatch_ic_insert(CID, b"perimeter", 0xCCCC, 1, false, false); | ||
| assert!(obj_dispatch_ic_lookup(CID, b"perimeter").is_some()); | ||
| // Registering a method anywhere bumps `VTABLE_GEN`; every cached | ||
| // resolution predates the new vtable shape and must stop being used. | ||
| test_bump_vtable_generation(); | ||
| assert_eq!(obj_dispatch_ic_lookup(CID, b"perimeter"), None); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Two new unit tests depend on process-global state that sibling tests mutate. Cargo runs all unit tests of this crate in one process. VTABLE_GEN and PARENT_DENSE_INCOMPLETE are process-global, so a concurrent class registration in an unrelated test can make either test fail with a message that blames the feature under test instead of naming the race. Scope each assertion to a window where the precondition is verified.
crates/perry-runtime/src/object/class_registry/dispatch.rs#L587-L595: wrap the insert-then-lookup pair inwith_stable_gen, and keep the explicittest_bump_vtable_generation()call and the follow-up miss assertion outside that window.crates/perry-runtime/src/object/class_meta_registry.rs#L372-L379: makePARENT_DENSE_INCOMPLETE.is_idle()a guarding condition for theget_parent_class_id(60_050)assertion instead of an unconditionalassert!.
📍 Affects 2 files
crates/perry-runtime/src/object/class_registry/dispatch.rs#L587-L595(this comment)crates/perry-runtime/src/object/class_meta_registry.rs#L372-L379
🤖 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/class_registry/dispatch.rs` around lines 587
- 595, Scope the insert-and-lookup precondition in test
a_class_registration_invalidates_every_entry with with_stable_gen, while keeping
test_bump_vtable_generation() and the subsequent miss assertion outside that
stability window. In crates/perry-runtime/src/object/class_meta_registry.rs
lines 372-379, guard the get_parent_class_id(60_050) assertion with
PARENT_DENSE_INCOMPLETE.is_idle() rather than asserting it unconditionally.
…ash maps (#7769) The class parent chain becomes a dense atomic mirror instead of a process-global RwLock<HashMap>; the hasInstance / toStringTag / extends-Error / fetch-parent / generic-origin / class-static-symbol / timer-id registries get monotone latches; the dispatch tower caches its own per-(class, method name) resolution behind a re-checked receiver-shape guard; and vtable argument marshalling drops two Vec allocations per dynamic call. shapes.ts 0.28s -> 0.23s on the pinned quiet mini, no protected floor crossed.
9d6a18e to
94bd66f
Compare
…ash maps (#7769) The class parent chain becomes a dense atomic mirror instead of a process-global RwLock<HashMap>; the hasInstance / toStringTag / extends-Error / fetch-parent / generic-origin / class-static-symbol / timer-id registries get monotone latches; the dispatch tower caches its own per-(class, method name) resolution behind a re-checked receiver-shape guard; and vtable argument marshalling drops two Vec allocations per dynamic call. shapes.ts 0.28s -> 0.23s on the pinned quiet mini, no protected floor crossed.
94bd66f to
1d99f98
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/timer/ref_states.rs`:
- Around line 120-131: Make
latch_tests::starts_idle_so_a_timer_free_program_pays_nothing deterministically
verify a fresh idle TIMER_IDS_NONEMPTY state instead of conditionally skipping
when the process-global latch was already armed. Use an isolated test process or
a fresh-latch fixture, and preserve the assertion that timer ID 1 is not known.
- Around line 88-92: Update the documentation for is_known_timer_id to state
that timer IDs are retained only until bounded eviction by
TimerRefStates::insert_bounded, without claiming entries are never removed or
IDs are never reused; keep the existing active/cleared timer semantics and
fast-path usage description accurate.
- Around line 100-107: Update the module documentation describing unavailable
timer-registry state to explicitly state that a poisoned mutex causes a
panic/abort rather than returning false. Keep is_known_timer_id and
is_known_timer_id_slow unchanged unless implementing consistent Err handling is
required by the documented policy.
🪄 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: 6f31befe-1d25-4ea4-ada1-22eaf30503cd
📒 Files selected for processing (3)
changelog.d/7769-class-dispatch-locked-hash-maps.mdcrates/perry-runtime/src/timer.rscrates/perry-runtime/src/timer/ref_states.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/perry-runtime/src/timer.rs
| crate::registry_latch::RegistryLatch::new(); | ||
|
|
||
| /// Whether `id` corresponds to a timer that was scheduled by this runtime | ||
| /// (active or already cleared). Used by the small-handle method/property | ||
| /// fast paths in `object/*.rs` and by `js_number_coerce` to decide whether |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate ref_states.rs and related cap/test definitions"
fd -a 'ref_states\.rs$' . | sed 's#^\./##'
rg -n "TIMER_REF_STATES_CAP|TimerRefStates|is_known_timer_id|insert_bounded|never removed|hasRef|unref|clearTimeout|RegistryLatch" crates/perry-runtime/src -S
echo
echo "Show relevant ref_states.rs sections"
if [ -f crates/perry-runtime/src/timer/ref_states.rs ]; then
wc -l crates/perry-runtime/src/timer/ref_states.rs
sed -n '1,190p' crates/perry-runtime/src/timer/ref_states.rs | cat -n
fiRepository: PerryTS/perry
Length of output: 30856
Resolve the is_known_timer_id eviction documentation without relying on monotonic ids.
TimerRefStates::insert_bounded removes the oldest entries when TIMER_REF_STATES_CAP is reached, but the is_known_timer_id docs still say entries are “never removed” and justify retaining ids as “never reused.” If runtime id reuse happens, is_known_timer_id will return false for an existing timer handle after eviction, letting post-clear .hasRef() / .unref() / +timer miss timer dispatch. Either guard evicted ids against real id reuse before removing them, or rewrite the comment to say ids are retained only until bounded eviction without implying reuse cannot occur.
🤖 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/timer/ref_states.rs` around lines 88 - 92, Update
the documentation for is_known_timer_id to state that timer IDs are retained
only until bounded eviction by TimerRefStates::insert_bounded, without claiming
entries are never removed or IDs are never reused; keep the existing
active/cleared timer semantics and fast-path usage description accurate.
Source: MCP tools
| /// / `.unref()` still route through timer dispatch (Node keeps the | ||
| /// Timeout object alive after `clearTimeout` and methods still work). | ||
| #[inline] | ||
| pub fn is_known_timer_id(id: i64) -> bool { | ||
| if id <= 0 || TIMER_IDS_NONEMPTY.is_idle() { | ||
| return false; | ||
| } | ||
| is_known_timer_id_slow(id) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect registry lock sites and the configured panic policy.
rg -n -C 6 \
'TIMER_REF_STATES|is_known_timer_id_slow|set_timer_ref_state|panic\s*=' \
crates/perry-runtime/src Cargo.tomlRepository: PerryTS/perry
Length of output: 27439
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## ref_states impl snippets\n'
sed -n '1,140p' crates/perry-runtime/src/timer/ref_states.rs
printf '\n## timer ref_state snippets\n'
sed -n '390,402p;108,115p;573,578p;824,831p' crates/perry-runtime/src/timer.rs
printf '\n## Cargo profiles around panic settings\n'
sed -n '1,30p;80,110p;230,260p' Cargo.tomlRepository: PerryTS/perry
Length of output: 10590
Document the poisoned-mutex fallback as a panic.
is_known_timer_id_slow() uses Mutex::lock().unwrap(), so a poisoned mutex here aborts instead of falling back to false. The release/dist panic policy is abort, but the module doc still describes this path as returning false for an unavailable registry. Either handle Err consistently or update the description to match the abort behavior.
🤖 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/timer/ref_states.rs` around lines 100 - 107, Update
the module documentation describing unavailable timer-registry state to
explicitly state that a poisoned mutex causes a panic/abort rather than
returning false. Keep is_known_timer_id and is_known_timer_id_slow unchanged
unless implementing consistent Err handling is required by the documented
policy.
Source: MCP tools
| #[cfg(test)] | ||
| mod latch_tests { | ||
| /// The OFF state is the one every timer-free program takes, so it is the | ||
| /// one that must be asserted: an accidentally pre-armed latch would put the | ||
| /// mutex back on the dispatch path with nothing to notice. | ||
| #[test] | ||
| fn starts_idle_so_a_timer_free_program_pays_nothing() { | ||
| if super::TIMER_IDS_NONEMPTY.is_idle() { | ||
| assert!(!crate::timer::is_known_timer_id(1)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not silently skip the latch assertion.
Because TIMER_IDS_NONEMPTY is process-global and monotone, another test can arm it first. The if at Line 126 then skips the assertion, and the test passes without checking the idle path. Run this check in an isolated process or use a deterministic fresh-latch fixture. (raw.githubusercontent.com)
🤖 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/timer/ref_states.rs` around lines 120 - 131, Make
latch_tests::starts_idle_so_a_timer_free_program_pays_nothing deterministically
verify a fresh idle TIMER_IDS_NONEMPTY state instead of conditionally skipping
when the process-global latch was already armed. Use an isolated test process or
a fresh-latch fixture, and preserve the assertion that timer ID 1 is not known.
Source: MCP tools
…ash maps (#7769) The class parent chain becomes a dense atomic mirror instead of a process-global RwLock<HashMap>; the hasInstance / toStringTag / extends-Error / fetch-parent / generic-origin / class-static-symbol / timer-id registries get monotone latches; the dispatch tower caches its own per-(class, method name) resolution behind a re-checked receiver-shape guard; and vtable argument marshalling drops two Vec allocations per dynamic call. shapes.ts 0.28s -> 0.23s on the pinned quiet mini, no protected floor crossed.
1d99f98 to
53f8c0a
Compare
…ash maps (#7769) The class parent chain becomes a dense atomic mirror instead of a process-global RwLock<HashMap>; the hasInstance / toStringTag / extends-Error / fetch-parent / generic-origin / class-static-symbol / timer-id registries get monotone latches; the dispatch tower caches its own per-(class, method name) resolution behind a re-checked receiver-shape guard; and vtable argument marshalling drops two Vec allocations per dynamic call. shapes.ts 0.28s -> 0.23s on the pinned quiet mini, no protected floor crossed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@changelog.d/7769-class-dispatch-locked-hash-maps.md`:
- Around line 124-126: Update the protected benchmark summary in the changelog
so it excludes the changed shapes row, and correct or explicitly label the
shapes ratio as rounded according to the displayed timings.
🪄 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: a5774de8-b664-482d-a196-fa6b64b32557
📒 Files selected for processing (1)
changelog.d/7769-class-dispatch-locked-hash-maps.md
| Every protected benchmark is **identical between the two arms**. `shapes` is the | ||
| only row that moves: 0.29 → 0.24, i.e. 8.6x behind scriptc's 0.0272 s, down from | ||
| 10.7x. Outputs are byte-identical to the baseline arm and to |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the referenced changelog fragment around the reported lines.
if [ -f changelog.d/7769-class-dispatch-locked-hash-maps.md ]; then
echo "== file exists =="
wc -l changelog.d/7769-class-dispatch-locked-hash-maps.md
echo "== lines 1-180 =="
nl -ba changelog.d/7769-class-dispatch-locked-hash-maps.md | sed -n '1,180p'
else
echo "File not found"
fi
echo "== nearby benchmark mentions =="
rg -n "Every protected benchmark|shapes|scriptc|0\.0272|0\.29|0\.24|8\.6x|8\.8x" changelog.d/7769-class-dispatch-locked-hash-maps.md || trueRepository: PerryTS/perry
Length of output: 283
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Programmatically compute the ratio from the rounded values in the reported sentence.
python3 - <<'PY'
import math
base = 0.24 / 0.0272
print(round(base, 1), "x")
print("ratio using reported unrounded values:", base)
PYRepository: PerryTS/perry
Length of output: 214
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file exists =="
wc -l changelog.d/7769-class-dispatch-locked-hash-maps.md
echo "== lines 1-180 with read-only line numbers =="
awk '{printf "%6d\t%s\n", NR, $0}' changelog.d/7769-class-dispatch-locked-hash-maps.md | sed -n '1,180p'
echo "== benchmark mention locations =="
grep -n "Every protected benchmark\|shapes\|scriptc\|0\.0272\|0\.29\|0\.24\|8\.6x\|8\.8x" changelog.d/7769-class-dispatch-locked-hash-maps.md || trueRepository: PerryTS/perry
Length of output: 12079
Fix the protected benchmark summary and ratio.
Line 123-124 says every protected benchmark is identical, but shapes moves and the table shows the other rows are unchanged. Change the sentence to say every other protected benchmark is identical, or remove the “Every” claim. The displayed timing also divides to about 8.8x; label the ratio as rounded/unrounded or correct it.
🤖 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/7769-class-dispatch-locked-hash-maps.md` around lines 124 - 126,
Update the protected benchmark summary in the changelog so it excludes the
changed shapes row, and correct or explicitly label the shapes ratio as rounded
according to the displayed timings.
53f8c0a to
6bad8b4
Compare
…te, not a bare floor The addr-class ratchet caught the new site: 0x10000 sits below the fetch/zlib/proxy handle bands, so a handle id would have been dereferenced as a keys array (#7531/#7709's class). Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
Merging as v0.5.1452, with one fix added on the branchThe three load-bearing cache properties all verifyThe PR names them, and each has a test that passes here: both resolution points populate the cache (the first attempt cached only the own-vtable arm, so every inherited method missed forever — and inherited methods are the common case in a hierarchy); keyed on name bytes, not address (
The prototype-surgery finding is quietly important: The fix I addedThe addr-class ratchet failed the branch: the cache's keys-array screen used a bare Gap suite: 19 failures, every one byte-identical on a merge-base binary. One gate run flaked on |
1 participant
The finding
gc-handoff/apps/shapes.ts— a scene-graph class hierarchy (deepextendschains, virtual dispatch through a base-typed collection,super()chains,instanceof, getters, statics, a fieldless subclass and a two-level indirect subclass) — was the widest margin a competing compiler held anywhere in the corpus.A symbolicated profile on the pinned quiet mini showed the cause is not codegen: the runtime answered "who is this class's parent?" and "which method is this?" with a process-global lock plus a SipHash probe, per hop, per call.
std::hash::random::RandomStatewas 1.3% of runtime andpthread_mutex_{lock,unlock}another 2.8% — for what is semantically an indexed load in a single-threaded program.What changed
The parent chain is a dense atomic mirror.
get_parent_class_id— walked byinstanceof, vtable dispatch, static-member lookup,super()construction, symbol lookup and the typed-feedback guards — tookCLASS_REGISTRY.read()plus a hash probe on every hop. Edges whose child id fits a 64 K window now answer from one atomic load; the reserved builtin bands and high-bit synthetic ids keep using the map. The slot storesparent + 1, which is what lets one word distinguish "absent" from "registered with parent id 0".Monotone latches (
registry_latch::RegistryLatch, perf(runtime): monotone latches make unused-feature type probes free #7755) on theSymbol.hasInstance/Symbol.toStringTag/extends Error/ fetch-parent / generic-origin / class-static-symbol / timer-id registries.js_instanceofprobed three of them on every evaluation andclass_chain_reachesone on every hop — all empty in a program that uses none of those features. ThehasInstancelatch also keeps the string-keyedwell_known_symbol("hasInstance")interning probe off the path entirely.The dispatch tower caches its own resolution.
js_native_call_methodis the virtual-call path for every receiver whose static type does not pin the callee — which is every call through a base-typed collection, the shape a class hierarchy is written in. Reaching a resolution cost aStringallocation for the method name, aRuntimeHandleScope, ~900 lines of exotic-receiver probes, a GC-heapStringHeaderallocation for the prototype probe, a lock and two SipHash lookups: four heap allocations and a lock aroundshape.area()'s single multiply. A per-thread, content-keyed cache now records the tower's outcome for a(class_id, method name)pair.One argument vector instead of two
Vecs.call_vtable_methodbuilt aVec<f64>of positional args andcall_fn_with_f64_argsbuilt a second withthisprepended — twomalloc/freeround-trips for a zero-argument virtual call.Three things about the cache that are load-bearing
class Square extends RectcallingRect'sarea) missed forever, and inherited methods are the common case in any real hierarchy. The parent-chain walk inhandle_methodsis the site that mattered: adding it took shapes from 0.25 to 0.23.VTABLE_ICkeys on the rodata pointer codegen passes, butjs_native_call_method_str_keyreaches the same tower with a name materialised into a caller-stack scratch buffer, where two different short names genuinely land at the same address in successive calls. A sabotage test plants exactly that and asserts a miss.gc_pointer_and_type_from_value(buffers, typed arrays, Sets, Maps, RegExps and Symbols are raw allocations with noGcHeader, so screening them before the header read is a memory-safety requirement, not an optimisation — see test262 regressions introduced by the #5579 fix batch (Object.defineProperty writable + RegExp /u-mode escapes + TypedArray species) #5625),OBJECT_TYPE_REGULAR, a nullmeta(which rules out both a per-instance[[Prototype]]override and any own accessor descriptor), the own-key scan an own field would win on, and the recorded-prototype probe. Theusing/await usingdisposal hooks and the iterator helpers are excluded by name, because both branch on per-object state the guard cannot see.Prototype surgery now bumps
VTABLE_GEN.invalidate_class_prototype_fast_guardsis the single latch all three prototype-write entry points funnel through, but the method-dispatch caches were only retired by class registration.Thread safety
perry/threadspawns real OS threads with independent arenas. The parent mirror is process-global atomics published (Release) before the map insert, so no reader can observe an edge through the map without it also being visible densely; the latches followRegistryLatch's arm-before-publish rule, whose only possible wrong observation ("idle while non-empty") that rule excludes. The dispatch cache is per-thread and starts empty on every worker, so a worker populates it from its own tower run rather than inheriting one.test-files/test_issue_7769_thread_class_dispatch.tsruns the same hierarchy on the main thread, throughparallelMap(8 workers) and throughspawn, and compares all three — then re-checks the main thread afterwards.Measured — rebased onto
c2a96b638, both arms rebuilt from that merge-baseProtected benchmarks re-measured interleaved (arms alternating inside one window, best of 7): a sequential base-then-arm pass showed +0.01-0.02 on several rows, and interleaving proved that was host drift moving both arms together, not a regression.
Every protected benchmark is identical between the two arms.
shapesis the only row that moves: 0.29 → 0.24, i.e. 8.6x behind scriptc (0.0272 s), down from 10.7x. Outputs byte-identical to the baseline arm and tonode --experimental-strip-types, verified before timing.The win is marginally larger after the rebase than before it (0.28 → 0.23 on the old base), because #7762 put a
class_generic_originprobe insideclass_prototype_object, which the parent-chain walk calls on every hop — one more locked hash probe on main, which this change's latch answers from an atomic load.On the
shapes_bigprofile (two agreeing 7 s runs, ~5 250 samples each; measured pre-rebase) the dispatch cluster falls from 12.3% to 7.5% andRandomState+pthread_mutex_*from 5.6% to 3.6%. Four leaders leave the top ranks:get_parent_class_id(3.3% → 0.5%),class_chain_reaches(2.1% → 0.3%),js_instanceof(1.0%), andRuntimeHandle::get_nanbox_u64— the single hottest symbol in the program at 4.9% — because the fast path needs no handle scope.What this does NOT fix (and where the rest of the gap is)
is_registered_symbol/is_registered_map/is_registered_set, reached fromjs_array_get_f64(so, everyarr[i]) and from the dispatch guard's pointer classification. Those registries are already latched (perf(runtime): cache the hot thread-locals so one allocation pays one _tlv_get_addr #7474, perf(runtime): monotone latches make unused-feature type probes free #7755) — the latches are simply armed, because something in startup materialises a well-known Symbol. That turns a free atomic load into a process-global mutex for every array element read in every program. Worth chasing, but it is Map/Set/Symbol registry work, adjacent to perf(array): answer "is this receiver a Map/Set?" from the object's own GC tag (#7765) #7765.shapes.tsis array element reads (js_array_get_f64+array_object_flags+js_array_length, ~10%) and the GC layout tables (~14%).Class.prototype.m = fnafterm's first dispatch still resolves to the vtable method, andObject.setPrototypeOf(instance, donor)does not redirect an already-dispatched method on that instance. Both are produced by the tower itself — the guard rejects a receiver with ametarecord, and prototype surgery now bumpsVTABLE_GEN— so neither is reached from the cache. They are documented in the gap test rather than asserted, so the file stays byte-identical to Node.Tests
test-files/test_gap_7769_class_dispatch_shapes.ts— fieldless subclass, two-level indirect subclass, class expressions (with and withoutextends), a subclass overriding nothing, base-typed dispatch cold and warm,instanceofacross the lattice, own-field shadowing after the pair has been cached,super()/statics up the chain, computed method names. Byte-identical to Node.test-files/test_issue_7769_thread_class_dispatch.ts— cross-thread dispatch.Some(0)vs absent, out-of-window fallback, latch defaults, and five for the cache including the stack-address sabotage.cargo test --release -p perry-runtime: 2044 passed, 0 failed. Gap suite (518 tests): 19 failures, every one byte-identical on a merge-base binary — all pre-existing.Summary by CodeRabbit
Performance
Bug Fixes
instanceof, static members, computed names, and shadowed methods.Tests