perf(codegen, runtime): stop recording parameter-guard visits that can never be consulted - #8238
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe compiler now marks only cyclic or multiply reachable descriptor containers for visit tracking. The runtime uses PGT2 metadata, lazy visit storage, and a cumulative traversal limit. Tests cover cycles, shared values, duplicated values, and rejection of PGT1 descriptors. ChangesParameter-guard visit tracking
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
✨ 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 |
…n never be consulted `js_param_type_guard` kept its visited set unconditionally, so every container it touched paid a linear scan of up to 64 inline entries and, past that, a `HashSet` insert — per ARRAY ELEMENT. The compiler owns the descriptor graph and can decide which visits are worth recording, so it now does, and the runtime reads the answer instead of recomputing it. Refs #8202.
6e266d0 to
f5c9aad
Compare
Validation
Not run here: the full 569-test gap suite against node, and Why the behavioural surface is narrowThe bit only controls memoization, so it cannot make the guard accept something |
The visit-tracking analysis reasons about the descriptor graph; value-level duplication can still re-enter an untracked node with the same address, and nesting it multiplies. MAX_DEPTH bounds depth, not work. Claude-Session: https://claude.ai/code/session_01AHvBYz7E6wWKv8kmvLLGpj
1 participant
Refs #8202.
js_param_type_guardruns on every unproven call into a guarded ordinary-parameter clone. #8201 moved the scalar descriptors to the typed-abi leaf guards; what stayed on the interpretive validator is the structural half, and #8202 priced its fixed per-call component: descriptor re-parse, a ~1 KBGuardStatezero-init, and a linearseen_or_insertscan per node visited.This removes two of those three.
The visited set was unconditional
Every container the walk touched went into the set — per array element. Validating
p: { toks: Token[], pos: number }on everypeek(p)therefore recorded one entry per token, and not one of them could ever be consulted:Tokenlies on no descriptor cycle and is reachable by exactly one path, so a second arrival at the same(address, node)pair is impossible.The set is load-bearing for exactly two facts, and both are properties of the immutable compiler-emitted graph rather than of the value:
env.parent === env) can only walk forever through a node that reaches itself;So the compiler decides it.
visit_tracking_bitsruns Tarjan over the graph it just built and propagates a saturating "ways in" count from the root, then sets the high bit of the op byte on exactly the container nodes that need recording.js_param_type_guardmasks the op byte and reads the bit.On
interp.tsthat is:peek(p: Parser)— 6 nodes, 0 tracked, was one record per token;asNum(v: Value)— 123 nodes, 15 tracked, exactly the recursiveNode/Envcluster, 108 nodes stop recording. Descriptor length is unchanged (the bit rides in a byte that only ever held ops 0–16).The magic goes
PGT1→PGT2, so a mismatched compiler/runtime pair fails closed on the magic — guard returns 0, caller takes the generic function — instead of reading a v1 blob as one that opts out of tracking everywhere.GuardStatezeroed 1 KB of stack per callinline_visitedis nowMaybeUninit. Only[..inline_visited_len]is ever read, and after the change above most guarded calls never write a slot at all.Measured
Instructions retired (
/usr/bin/time -l), best-of-3, both arms built from their own tree with the same-p perry -p perry-runtime-static -p perry-stdlib-static,PERRY_RUNTIME_DIRpinned per arm, all 19 corpus stdouts byte-compared and exit-checked.iso_misskeepsmisses 0.Exactly 2 of the 19 rows emit a
js_param_type_guardcall site —interpandiso_miss, two sites each (asNum,peek). Both improve:interpiso_missIsolating the validator's own cost, by differencing against the same runtime archive (
PGT1blob +PGT2runtime = every guard fails, so binary-layout effects cancel):maininterpiso_missThe other 17 rows are not attributable, in either direction
The two arms'
libperry_runtime.adiffer in exactly two functions out of 11,185 in the crate's codegen unit —js_param_type_guard(808 → 316 bytes) andGuardState::matches(+28) — every other function byte-identical. The 17 rows that emit no guard call site execute neither, so their movement (pipeline−3.9%,retain_wide1+0.6%,retain1+0.3%,deeplist−0.5%,retain−0.3%, the remaining 12 within ±0.1%) is address-layout noise, not effect. Twomainbuilds from identical source came out byte-identical (archive andperrybinary alike), and repeat runs of one binary spread ~0.1%, so the build itself is deterministic andpipeline's ±4% is what an address-hash-sensitive program does when the heap moves. I am claiming none of it.Tests
a_tree_shaped_descriptor_records_no_visits,a_container_on_a_cycle_records_its_visits,a_shared_container_records_its_visits(codegen, through the real encoder);a_tracked_node_terminates_on_a_cyclic_value— which also asserts the untracked form of the same descriptor conservatively returns 0 rather than hanging —an_untracked_shared_node_decides_the_same_way, andthe_previous_descriptor_format_is_refused(runtime).cargo test --release -p perry-runtime --lib -p perry-codegengreen.What this does NOT fix
#8202's premise was that the fixed per-call overhead dominates. It does not: it is ~15% of the validator's cost, and the validator is ~12% of
interp. The structural walk is the other ~10.5pp, and the measurement in the issue thread shows the specialization it gates is worth ~0.2%. That is a policy question for #8094/#8079, not a per-call-overhead one — see the issue comment.Summary by CodeRabbit
Bug Fixes
Performance