perf(runtime): give the shadow-stack root state the hot-TLS fast path (−8% try entry) - #10619
proggeramlug wants to merge 2 commits into
Conversation
…path
Every non-throwing `try` entry paid a real out-of-line `_tlv_get_addr`
call, unconditionally, that nothing else in `try_push_with_kind` did.
Profiling `try { t += v; } catch { t = 0; }` (differenced inside each
binary: `(try80-try16)/64` vs `(loop80-loop16)/64` control, per
CLAUDE.md's verification method, N=20000, median of 7) confirmed the
178-instruction/entry baseline and pinned the cost to one leaf via
`sample`+`PERRY_KEEP_SYMBOLS=1` at 20M iterations:
`try_push_with_kind`+176 -> `_tlv_get_addr`, ~6% of instructions
retired per entry.
Root cause: `CatchSavepoint::capture()`'s shadow field is captured
unconditionally on every entry (`shadow_stack_savepoint`), and once any
function anywhere in the process has pushed a shadow frame — true
almost immediately for any real program, since a `catch (e)` binding
itself needs one — the `SHADOW_FRAMES` latch from #7469's wave-1 fix is
already set, so the latch stops making this read rare in practice.
Unlike `EXCEPTION_STATE`, `CALL_METHOD_DEPTH`, and the named
`runtime_handle_stack`/`temp_roots` fields, `SHADOW` was still a raw
`thread_local!`, so this one read never got the `tls_hot` fast path at
all.
Fix: move `SHADOW` (crates/perry-runtime/src/gc/roots/shadow_stack.rs)
from `thread_local!` to `crate::perry_thread_local!` — a pure
storage-mechanism swap (same type, same `.with()`/`.try_with()` call
sites, same const-init/drop-free semantics: `ShadowStackState` still
has no `Drop`, so no destructor is registered) that changes nothing
about liveness, throw-time restore, or the fixed-address contract
`js_shadow_frame_enter` depends on for its whole-activation pointer
cache. Confirmed via `--trace llvm`/`sample` that the leaf disappears
entirely post-change.
Measured (mybase = own build at this commit's parent, 9df5075,
codegen-units=16; both PERRY_NO_AUTO_OPTIMIZE=1):
before: try 179.0-179.4 instr/entry, control -0.001..-0.2
after: try 164.3-165.7 instr/entry, control -0.14..+0.44
-> ~14-15 instructions/entry (~8%), controls stay noise-floor both arms.
`try_push_with_kind`'s remaining `CatchSavepoint::capture()` work and
`js_try_end`'s own (already tls_hot-fast) EXCEPTION_STATE resolution
are UNTOUCHED — js_try_end disassembles to ~28 near-minimal
instructions with nothing left to cut without threading a pointer
through codegen from push to end, which was evaluated and deferred as
materially riskier (touches try_stmt.rs's early-exit/closure/
generator/async call sites) for a smaller remaining win.
scripts/thread_local_cold_allowlist.json: only the
`shadow_stack.rs: 2 -> 1` line this change causes. `_hot_declarations`
is left untouched — verify() never reads it (only `files` is
enforced) — because a plain `--update` on unmodified main already
produces 466, not the committed 460: six pre-existing, unrelated
`perry_thread_local!` additions had drifted the count before this
change touched anything. Fixing that drift is not this PR's job.
Correctness: the shadow stack is the GC's precise root set, not pure
exception bookkeeping, so this was checked past "it compiles":
- Added test-files/test_gap_try_entry_shadow_hot_tls.ts (byte-identical
to node v26.5.1 --experimental-strip-types --no-warnings, checked
against both this change and the pristine parent arm): same-level
catch, a throw 4 frames deep, a throw crossing Array.prototype.map's
runtime trampoline, finally on both the normal and throwing paths,
nested try with an inner rethrow, a catch that itself throws, 25
levels of try/finally nesting with finally-order and try_depth
restored checked afterward, and a non-throwing control loop. Every
caught value is read back after GC-pressure allocation.
- Ran that fixture, plus the pre-existing
test_gap_gc_catch_param_rooting.ts, test_gap_try_savepoint_subsystems.ts,
test_gap_try_finally_no_catch_rethrow.ts and test_gap_try_setjmp_volatile.ts,
under PERRY_GC_SCHEDULE_SEED (5 seeds) + PERRY_GC_SCHEDULE_RATE=1 +
PERRY_GC_SCHEDULE_ALLOC_KB=0 + PERRY_GC_PROTECT_FROMSPACE=1 +
PERRY_GC_DIAG=1 (per CLAUDE.md's rooting-bug instruments) — a
collect-at-every-safepoint, evacuating, quarantine-and-mprotect
schedule. All 5 seeds matched node byte-for-byte; the diagnostic
output confirms the instrument was live, not vacuous
(forced_collections=2566, copying_minors=2566, moved_objects=37000,
fromspace retired_set up to #4 with bytes_protected growing) — no
stale-shadow-stack SIGSEGV, no output drift.
- `RUST_TEST_THREADS=1 cargo test --release -p perry-runtime --lib`:
4010 passed. The 2 failures
(gc::tests::copy_slot_decode::sabotaged_remembering_arm_is_refused_by_the_coverage_cross_check,
gc::tests::heap_generation::a_free_or_move_outside_every_scope_is_caught_in_debug_builds)
are pre-existing: reverted this change with `git checkout --` and
reran just those two on the pristine parent commit — identical
failures, same messages, confirming they assert debug_assert!-gated
behavior compiled out under --release, unrelated to this change.
- `cargo fmt --all -- --check`, `scripts/check_file_size.sh`,
`scripts/check_test_registration.py`,
`RUSTFLAGS="-D warnings" cargo check -p perry-runtime --all-targets`:
all clean.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe change moves ChangesShadow stack TLS and exception handling
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to The TLS migration preserves the required storage behavior and is covered by deterministic exception and GC-pressure tests, so the change is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 56.25% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 2 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
Entering and leaving a
tryblock that does not throw costs 179 instructions; node does it in 13.tryappears 4.3 times per 1k lines of real TypeScript.179.3 → 164.8, −8.1%. This is the smallest win in this series and worth saying so plainly — but the probe measures only one of the paths it helps, and it moves a ratchet the repo actively wants moved.
What it is
CatchSavepoint::capture()runs on everytryentry. Wave 1 already latched most of its fields behind "has any thread used this subsystem", but that latch does not help forSHADOW: it is set by the first shadow-frame push anywhere in the process, and for any real program that is effectively immediate — acatch (e)binding needs a shadow slot itself. So the read happens on essentially every entry.And unlike
EXCEPTION_STATE,CALL_METHOD_DEPTHand the namedruntime_handle_stack/temp_rootsfields — all already routed throughtls_hot—SHADOWwas still a rawthread_local!, so this one read never got the fast path. On Darwin that means_tlv_get_addrper read. Profiling pinned it to a single call site:try_push_with_kind+176 → _tlv_get_addr, ~6% of instructions retired per entry.The change is a storage-mechanism swap to
crate::perry_thread_local!— same type, same.with()/.try_with()call sites, same const-init and drop-free semantics, so no destructor is registered and the address-stability contractjs_shadow_frame_enterdepends on is unchanged. After it,_tlv_get_addrno longer appears anywhere in the leaf profile.scripts/thread_local_cold_allowlist.json's own header says new code should useperry_thread_local!for exactly this reason. This converts one and the enforced per-file count goes 2 → 1.Measurement
(try80−try16)/64(loop80−loop16)/64o.aDifferenced within each binary so driver dispatch and code layout cancel before the arms are compared. N=20000, median of 7.
The 8% understates the change:
SHADOWis the shadow-stack root state, read on many paths, and this probe measures only the one where it was profiled.Validation — deeper than the size of the win, because this is precise-root state
The shadow stack is the GC's precise root set, not bookkeeping, so the fixture attacks rooting rather than confirming the happy path.
test_gap_try_entry_shadow_hot_tls.tscovers a same-level catch, a throw four frames down, a throw crossingArray.prototype.map's runtime trampoline,finallyon both the normal and throwing paths, nestedtrywith an inner rethrow, acatchthat itself throws, 25-level nesting with finally-order andtry_depthchecked afterwards, and a non-throwing control — every caught value read back after GC-pressure allocation. Byte-identical to node 26.5.1.That fixture plus four pre-existing exception/rooting gap fixtures then ran under five
PERRY_GC_SCHEDULE_SEEDvalues atPERRY_GC_SCHEDULE_RATE=1withPERRY_GC_SCHEDULE_ALLOC_KB=0and from-space protection: all five byte-identical to node, with the diagnostics confirming the instrument was live rather than vacuous —forced_collections=2566,copying_minors=2566,moved_objects=37000, quarantine retiring sets with growingbytes_protected, no faults.cargo test --release -p perry-runtime --lib: 4,010 passed, 2 failed — both confirmed pre-existing by reverting withgit checkout --and re-running them on the pristine parent. They assertdebug_assert!-gated behaviour that--releasecompiles out.cargo fmt --all --check, file-size cap, test registration andRUSTFLAGS=-D warnings cargo check -p perry-runtime --all-targetsall clean.Ratchet note
_hot_declarationsin the allowlist had drifted onmainindependently of this change: a clean--updateon unmodified9df5075fbealready produces 466 against a committed 460. That aggregate is not what the checker enforces — only the per-filefilescounts are — so it is left untouched here rather than silently absorbed into this diff. The allowlist change is one line.Left on the table
try_push_with_kind's remainingCatchSavepoint::capture()work is unchanged, and so isjs_try_end's own state resolution — it disassembles to about 28 near-minimal instructions. The larger win would be threading a pointer fromjs_eh_try_pushtojs_try_endso the exit skips resolving the state again, but that needs codegen changes acrosstry_stmt.rs's early-exit, closure, generator and async call sites. Deliberately deferred as materially riskier for a smaller win than the runtime-only change here.Summary by CodeRabbit
Performance
tryblocks, reducing overhead by approximately 8% on supported platforms.Reliability
try,catch, andfinallyflows, including rethrows and deep unwinding.Tests
tryexecution.