perf(codegen): compute the callee-rooting window instead of hardcoding it (#8159) - #8240
Conversation
…g it (#8159) #8084 closed a real moving-GC defect: five call arms lowered the CALLEE into a bare register, lowered the arguments below it -- each of which can allocate -- and handed the original register to the consuming call. Under the shipping statepoint lowering that register is in no live bundle, so nothing marks it and nothing relocates it. The fix asked rooting/'s existing machinery for protection, and paid for it with a hardcoded `collects = true` at every site. `collects` is not a strategy, it is a WINDOW: "can anything between this operand and its consumer collect?" Hardcoding it true buys a slot, a re-read and a release at every call site whether or not the window contains a collection point. `operand_protection` has always been able to answer this -- its `Reuse` arm emits no push, no re-read and no truncate, keeping the pre-#8084 IR byte for byte. What it needed was the truthful window, which `any_operand_may_collect` computes and `with_operands_rooted_window` has passed all along. lower_call/early_branches.rs the window is the argument lowering. The re-read sits above the unmask because that is where the value leaves the tracked domain; a collection point BELOW the unmask is a separate exposure that rooting the box cannot repair either way. expr/new_dynamic.rs both js_new_function_construct arms: the callee's window is every argument, argument i's is the arguments after it. Neither reaches a collection point of its own -- lower_js_args_array is an entry alloca plus stores, emit_call_location_at emits nothing in a default build -- so `new C(a, b)` over plain locals is back to emitting no rooting, the shape operand_needs_root's own doc already claims for it. expr/new_dynamic.rs's NewDynamicSpread and expr/call_spread.rs KEEP true, and now say why: bundle_args_rooted opens with js_array_alloc and a spread call reaches js_array_like_to_array unconditionally, so those windows allocate in every instance of the arm. Nothing to narrow, as opposed to nothing narrowed. Each flag is computed BELOW the operand it protects, as with_operands_rooted_window computes it: the predicate reads ctx, so asking before the lowering asks about a different state. temp_root_coverage/call_callee.rs is two differential pairs, in src/ so they run in the per-PR cargo-test gate rather than the nightly tier. The two fixtures in each pair differ in exactly one thing -- the argument's kind. Sabotage-checked against #8084's hardcoded true: both NEGATIVES go red there and both positives stay green, so neither half passes for nothing. The callee local is initialized to an object literal, and that is load-bearing. expr_is_known_non_pointer_shadow_value suppresses rooting for a LocalGet with no reserved shadow slot whose type proof is not pointer-bearing, so a first draft using `Expr::Undefined` had both POSITIVES failing against a correct compiler and -- worse -- both negatives passing for that reason rather than the one they claim. MEASURED, and the headline is negative: this recovers none of #8159's 3.9% on pipeline. That row never reaches these arms. Its `rec = stage(rec)` lowers through lower_dynamic_closure_call (js_closure_unbox_callee_checked), already fully rooted pre-#8084, and the emitted IR for pipeline.ts is BYTE-IDENTICAL across this change. Corpus instructions retired, min-of-5, same runtime archives both arms: pipeline +0.02%, interp -0.04%, iso_miss -0.05%, asyncpipe +0.11%, shapes -0.18% -- all inside noise, all outputs sha-identical. What it does move is zod: dep-native live bundles 36611 -> 36598, relocates 432545 -> 432338. Correctness, both corpora, base vs this change on the same build: dep-native unrooted 2 (budget 3), stale 0, seeded 40/40 -- IDENTICAL curated-native unrooted 0, 1 stale, seeded 39/40 -- IDENTICAL, and both halves of that verdict are pre-existing on clean 07c8040
|
Warning Review limit reached
Next review available in: 3 minutes Limit details: You’ve used all 8 included reviews currently available under your plan. 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 selected for processing (6)
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 |
1 participant
Closes #8159 — but not in the direction the issue expected, so the headline first.
The narrowing works, and it does not recover pipeline's 3.9%
#8159 asked whether #8084's per-call-site rooting sequence can be narrowed "e.g. skipping the reread when nothing between adopt and use can allocate". It can, this PR does it, and it recovers none of the cost on
pipeline— becausepipelinenever reaches the arms #8084 touched.pipeline's inner loop isrec = stage(rec), which lowers throughlower_dynamic_closure_call(js_closure_unbox_callee_checked) — the #7154 lowering, already fully rooted before #8084. The emitted IR forgc-handoff/apps/pipeline.tsis byte-identical across this change, andgrepfinds no call tojs_new_function_constructorjs_closure_call_apply_with_spreadin it at all. That rules the mechanism out structurally rather than failing to detect it.So the issue's attribution to the commit stands; its attribution to that hunk does not. The +3.95% is somewhere in #8084's other ~2,500 lines —
gc_map.rs(+355),gc/roots/stack_maps*(~1,660),gc/copying.rs+copying_pointer_set.rs,pin.rs,tenuring.rs,object/this_binding.rs. See the issue comment for the sharpest suspect.What the change is
collectsis not a strategy, it is a window: "can anything between this operand and its consumer collect?" #8084 hardcoded ittrueat five arms, which buys a slot store, a re-read and a release at every call site whether or not the window contains a collection point.operand_protectionhas always been able to answer this — itsReusearm emits no push, no re-read and no truncate, keeping the pre-#8084 IR byte for byte. It just needed the truthful window, whichany_operand_may_collectcomputes andwith_operands_rooted_windowhas passed all along.Three arms now compute it; two deliberately keep
trueand say why —bundle_args_rootedopens withjs_array_alloc, and a spread call reachesjs_array_like_to_arrayunconditionally, so those windows allocate in every instance of the arm. Nothing to narrow, as opposed to nothing narrowed.Each flag is computed below the operand it protects, as
with_operands_rooted_windowcomputes it: the predicate readsctx, so asking before the lowering asks about a different state.Measurements
Both arms are the same tree at
07c8040bfdiffering only in these three files; identicallibperry_runtime.a/libperry_stdlib.a(codegen-only change,cmp-verified untouched across the second build), per-armPERRY_CACHE_DIR,PERRY_NO_AUTO_OPTIMIZE=1,/usr/bin/time -l, min-of-5, stdout sha256 identical on every row.pipelineinterpiso_missasyncpipeshapesAll inside noise. Where it does move something is zod, which is the population that has these shapes: dep-native live bundles 36,611 → 36,598, relocates 432,545 → 432,338.
Soundness
gc_root_dominance_check.py, base vs this PR on the same build, both corpora — identical verdicts:--self-testpasses, so a green here means "examined and clean" rather than "declined to examine". #7803's own shape keeps its root by construction — its arguments are freshly-allocated object literals.Two observations about clean
main, neither caused by this PR (both arms report them identically), recorded so they aren't rediscovered:07c8040bf: onestalehazard (test_gap_class_forward_capture_6523::__closure_7,unmasked->js_box_set, moving viajs_object_get_field_by_name_f64) against--max-stale 0, plus 39/40 seeded violations caught where CI requires 40. It was invisible until fix(gc): the root-dominance symbol scan must accept the C-unwind ABI #8207, because the job aborted earlier at--audit-poll-capable.opt, not CI's — lowering a budget I cannot verify in CI is how the gate goes red for the next person. Worth CI measuring and tightening.Tests
temp_root_coverage/call_callee.rs— two differential pairs, insrc/so they run in the per-PRcargo-testgate rather than the nightly tier. The two fixtures in each pair differ in exactly one thing, the argument's kind.Sabotage-checked: against #8084's hardcoded
true, both NEGATIVES go red and both positives stay green. Neither half passes for nothing.The callee local's object-literal initializer is load-bearing and is commented as such:
expr_is_known_non_pointer_shadow_valuesuppresses rooting for aLocalGetwith no reserved shadow slot whose type proof is not pointer-bearing, so a first draft usingExpr::Undefinedhad both POSITIVES failing against a correct compiler and — worse — both negatives passing for that reason rather than the one they claim.Local:
cargo test -p perry-codegen --lib temp_root_coverage17/17 under both lowerings,cargo fmt --all --checkclean.