perf(runtime): stop hoisting cold-path thread-locals into o[k]'s fast lane (−5.8%) - #10651
Open
proggeramlug wants to merge 2 commits into
Open
proggeramlug wants to merge 2 commits into
proggeramlug wants to merge 2 commits into
Conversation
… lane js_object_get_field_by_name's Proxy-receiver block and RuntimeHandleScope's raw-thread_local! fallback were both small enough to inline into the hot dynamic-key-read path. A thread_local! address resolution is readnone from LLVM's point of view, so once inlined, the optimizer hoisted the proxy registry's and the transient-handle root stack's TLS lookups out of their guards (is_proxy_id_band, a "size"-key check) and ran them unconditionally on every js_object_get_field_by_name call, Proxy or not. Splitting each into its own #[inline(never)] function keeps the optimizer from seeing inside at the call site, so nothing gets hoisted past the guard. Also skip try_read_gc_header's redundant is_plausible_heap_addr recheck in try_data_get_bytes's prototype-chain loop, where the caller already proved it true one statement above. Measured on a two-property-object o[k] loop (never touching a Proxy or a .size key): 544.9 -> 513.0 instructions/access (-5.9%), via (dyn80-dyn16)/64 differenced against 2x the iteration count to cancel per-process fixed overhead. The (loop80-loop16)/64 no-op control reads ~0 in both arms (base: -0.01..-0.04, mine: -0.02..0.13), confirming the technique resolves changes this small. Verified via disassembly that both _tlv_get_addr calls are gone from the function's prologue.
|
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 (6)
Included review availability: Your plan provides up to 8 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe change isolates cold Proxy and runtime-handle TLS paths, adds a GC-header helper for already-validated addresses, and adds dynamic-key Proxy coverage. ChangesRuntime hot-path optimization
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to No actionable correctness, stability, security, or repository-contract issue remains from the reviewed change. 🚥 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 |
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.
Two thread-local resolutions sat unconditionally in
js_object_get_field_by_name's prologue on ano[k]loop that never touches a Proxy or a.sizekey — both belonging to cold arms that are logically guarded and never taken.544.7 → 512.9 instructions per access, −5.8%. The number is modest; the mechanism is the point, and one of the two fixes reaches far beyond this path.
Why a guard did not keep the TLS out
A
thread_local!address resolution isreadnoneto LLVM — it has no observable side effect. So once a guarded cold arm is visible to the inliner, the optimizer is free to hoist just the address computation above the guard and run it unconditionally; it does not need the guard to be true to preserve behaviour. The Rust-level gate survives, the TLS call escapes it.That is what put two
adrp+add+ldr+blrsequences in the prologue:PROXIESregistry lookup, reached viajs_proxy_is_proxybehindis_proxy_id_band(raw_addr)— always false for an ordinary object;RUNTIME_HANDLE_STACK's cold fallback, reached viaRuntimeHandleScope::new()from the.size-key arm, whose own source comment already records losing this exact fight at the Rust level.Fixing the Proxy block alone removed only one of them; a rebuild and second disassembly found the other still there by a different route. Both arms are now
#[cold] #[inline(never)], which keeps them opaque to the inliner.The
runtime_handleshalf is the general fix.RuntimeHandleScope::new()is called from dozens of arms across the runtime, many of them small and gated behind a cheap guard inside an otherwise hot function. Splitting that fallback protects all of them, not just this path, and it lets the fast published arm stay#[inline(always)]without dragging the raw TLS call to every call site.A third, smaller change:
try_data_get_bytescalledis_plausible_heap_addr(addr)explicitly and then again insidetry_read_gc_header, and LLVM could not CSE the two acrossclassify_heap_generation's intervening cache write. The one call site that has already proved the predicate now usestry_read_gc_header_known_plausible.Found by disassembly, not by reading
The profile charged both
_tlv_get_addrcalls directly tojs_object_get_field_by_namerather than to any callee, which is what said "inlined into the prologue" rather than "called from somewhere".otool -tVon the real function plusnmto resolve the branch targets identified which two thread-locals they were. After rebuilding, the same disassembly confirms both sequences are gone and the only remainingblrs are ordinary vtable and closure dispatch.Measurement
(dyn80−dyn16)/64(loop80−loop16)/640058babd83)Differenced within each binary so fixed per-process cost and code layout cancel before the arms are compared. N=20000, median of 7.
Three hypotheses that came back negative
Stated because they are worth not re-running:
try_data_get_bytes'sfrom_utf8and Bloom-hash preamble is spec-required work, not re-derivation.is_anon_shape_class_idis still 11.2% of the loop after the lock-free mirror added in perf(runtime): stop re-deriving the receiver on every dynamic-key read (−19%) #10570, and that residue is the per-imagecurrent()lookup — hash plus an 8-slot probe. It is already the cheapest form available given that a process-global mirror would answer one image's question from another image's registrations.keys_find_slot_by_bytesand itsmemcmpare genuine key-byte comparison. A hand-rolled short-key compare was judged not worth the correctness risk for the expected win.Validation
test_gap_dynamic_key_proxy_receiver.ts(new): trapped, pass-through and nested Proxies, an array accessed through a Proxy, and loops interleaving plain and Proxy receivers — the arm that was made cold must still be correct when it is taken. Byte-identical to node 26.5.1.test_gap_dynamic_key_read_paths.tsfrom perf(runtime): stop re-deriving the receiver on every dynamic-key read (−19%) #10570 still byte-identical.PERRY_GC_FROMSPACE_SCAN_ABORT=1: four runs, all exit 0 and node-matching,dangling=0,missing_rewrites=0, copying minors non-zero throughout (32, 29, 241, 247).test_gap_9592_child_timeout_threads, is a host artifact —/bin/truedoes not exist on this machine, so node itself throwsENOENTregardless of Perry, and base and arm produce byte-identical Perry output.cargo test -p perry-runtime --lib,RUST_TEST_THREADS=1: 4,016 passed, 2 failed — both confirmed pre-existing by reverting to pristineorigin/mainvia a patch round-trip (notgit stash, which is shared across worktrees here) and reproducing the identicaldebug_assert!panics.cargo fmt --check, file-size cap, test registration,check_thread_locals.py, andRUSTFLAGS=-D warnings cargo check --all-targetsall clean.Summary by CodeRabbit
Performance
Bug Fixes
Tests