fix(build-cache): register the two unkeyed codegen env vars (main CI is red) - #9044
Conversation
CI's codegen_env_vars_are_build_cache_inputs gate fails on current main: PERRY_BOX_CAPTURE_ENTRY_CELLS (PerryTS#9026's once-per-closure-entry cell resolution gate) and PERRY_GUARDED_PREINLINE_MAX_IR_BYTES (the guarded-preinline size ceiling) both change emitted code but key neither the cache nor an exclusion — every open PR is red on it. Register both as cache inputs, same rationale as the RS4GC and TRE budgets beside them. Claude-Session: https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP
|
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 (1)
📝 WalkthroughWalkthroughThe build-cache fingerprint now includes two codegen settings that affect emitted code: closure capture-cell resolution and the guarded-preinline IR-size ceiling. ChangesBuild cache fingerprinting
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to The change adds two existing code-generation settings to the build-cache fingerprint, preventing artifacts built with different settings from being reused. It introduces no new runtime or privilege path, and no actionable merge-blocking risk remains after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. Full details: Description checkExplanation The description explains the problem, identifies both variables, states why they must be cache inputs, and reports local test results. It does not follow the required template because it omits the Summary, Changes, Related issue, Test plan, Screenshots / output, and Checklist sections.
✨ 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 |
|
Merged — and this is fallout from two of my own merges, so thank you for catching it. Confirmed the breakage on
Input rather than exclusion is the right call for both, and the preinline reason states the actual hazard: "a run with a raised ceiling must not be served objects a default run produced". Registering either as an exclusion would let a cached build survive a knob flip and hand back objects built under the other setting. Worth noting the blast radius, because it explains why this was worth a dedicated PR: the assertion lives in a bin-crate unit test, so its failure doesn't just fail one test — the The gap on my sideMy per-PR validation runs Verification: Added the missing |
…his PR's knob Merging current main into this branch REVERTED PerryTS#9044: commit e3164ee removed `PERRY_BOX_CAPTURE_ENTRY_CELLS` and `PERRY_GUARDED_PREINLINE_MAX_IR_BYTES` from BUILD_CACHE_ENV_VARS along with PerryTS#9044's changelog fragment. The branch predates that fix, so the commit was built over a stale tree and carries the removal as an intentional-looking deletion -- which a merge then honours. That alone would have re-reddened main: the assertion lives in a bin-crate unit test, so its failure stops the whole `perry` test binary compiling and every open PR's cargo-test job goes red. This PR also adds a third codegen knob, PERRY_PACKED_LOOP_NUMERIC_ACCUMULATOR, without registering it. It is a cache INPUT, not an exclusion: with it on, `s += arr[i]` lowers to an inline fadd instead of `js_dynamic_string_or_number_add`, so the two settings emit different code and must never share a cached object. All three registered; fragment renumbered 0000 -> 9060.
…ic proof — s += arr[i] at node parity (was 5.3×) (#9060) * codegen: reduce accumulators earn the stable-packed fast clone's numeric proof `for (let i = 0; i < arr.length; i++) s += arr[i]` — the most common reduce shape in JavaScript — ran 5.3x slower than node, and BOTH halves of the reason were invisible to profiling alone: 1. Inside the fast clone, `s += arr[i]` still lowered `+` through `js_dynamic_string_or_number_add` (25% of the isolated loop): the loop guard proves the ELEMENT is raw f64, but the accumulator's own writes are circular for every whole-function numeric fact, so the add had one unproven operand. The element-shape clone already solved this with its `numeric_accumulator` (preheader tag test = the induction base case; every in-clone write numeric-preserving = the step). This ports that design: `collect_numeric_accumulators` admits plain, uncaptured, unboxed locals whose every body write is numeric with all leaves provable in-loop (fail-closed fixpoint; nested closures not descended — their captures are boxed and excluded anyway), the fast preheader tag-tests each one and takes the slow clone on any non-Number, and the fact rides `StablePackedLoopFact::numeric_accumulators`, scoped to the fast-clone lowering exactly as the element facts are. 2. With the add fixed, the clone was STILL dead: the accumulator's per-statement shadow CLEAR — `js_shadow_slot_set(slot, 0)`, emitted precisely BECAUSE the stored value is a proven non-pointer — failed `fast_clone_call_free`, and the admission arm then emits an UNCONDITIONAL branch to the slow preheader while still calling (and discarding) the guard. Timing shows slow, lldb on the guard shows "admitted", the IR shows a perfect fast body: nothing points at the terminator. `js_shadow_slot_set` is a bounds-checked TLS store (`gc/roots/shadow_stack.rs`) that cannot allocate, collect, or revoke a layout — which is precisely what the two call-free clone scans exist to exclude — so `is_gc_unsafe_call` now exempts it, for both this tier and the element-shape tier. The accumulator machinery lives in `stmt/stable_packed_accumulator.rs` (the 2,000-line file gate). `PERRY_PACKED_LOOP_NUMERIC_ACCUMULATOR=0` restores the old lowering; the scan exemption is unconditional (it is a factual classification, not a policy). Isolated reduce loop (Mac, 1k elements): 5341 -> 993-1040 ns = node parity (node 1009). wolf-ecs: +-0.08%, neutral. Differential vs node identical: string accumulators (concat preserved via the slow clone), mixed-element arrays (guard declines numeric mode), NaN/-0, in-loop reassignment to string (admission declines), multiple accumulators, Math chains, update-form counters. Kill switch output-identical. Claude-Session: https://claude.ai/code/session_019WVcWKmYsUBnnFB7nBgbBJ * fix(build-cache): restore #9044's registrations and register this PR's knob Merging current main into this branch REVERTED #9044: commit e3164ee removed `PERRY_BOX_CAPTURE_ENTRY_CELLS` and `PERRY_GUARDED_PREINLINE_MAX_IR_BYTES` from BUILD_CACHE_ENV_VARS along with #9044's changelog fragment. The branch predates that fix, so the commit was built over a stale tree and carries the removal as an intentional-looking deletion -- which a merge then honours. That alone would have re-reddened main: the assertion lives in a bin-crate unit test, so its failure stops the whole `perry` test binary compiling and every open PR's cargo-test job goes red. This PR also adds a third codegen knob, PERRY_PACKED_LOOP_NUMERIC_ACCUMULATOR, without registering it. It is a cache INPUT, not an exclusion: with it on, `s += arr[i]` lowers to an inline fadd instead of `js_dynamic_string_or_number_add`, so the two settings emit different code and must never share a cached object. All three registered; fragment renumbered 0000 -> 9060. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
codegen_env_vars_are_build_cache_inputsfails on current main —PERRY_BOX_CAPTURE_ENTRY_CELLS(#9026) andPERRY_GUARDED_PREINLINE_MAX_IR_BYTESlanded without build-cache registration, so every open PR's cargo-test job is red (that's how I found it, on #9042). Both change emitted code, so they're cache inputs, not exclusions — same rationale as the RS4GC/TRE budgets beside them. The four build_cache tests pass locally with the registration.Summary by CodeRabbit
Bug Fixes
Documentation