fix(lint): split index_set.rs, over the 2000-line cap since #7342 - #7366
Merged
Conversation
`scripts/check_file_size.sh` exits 1 on main HEAD: `crates/perry-codegen/src/expr/index_set.rs` is 2035 lines against a 2000 cap. It crossed in #7342. That script runs inside the `lint` job, which is a REQUIRED context -- so this is the second independent way `lint` was red on main today (the first was rustfmt on linker.rs, #7361). A required check that is red on main blocks nothing; it means every merge is a bypass. The split follows the recipe in the script's own failure message: extract a topical group into a sibling module. `lower_inline_dyn_typed_array_set` and its `emit_inline_ta_int_store` helper are the guarded inline typed-array store for a type-erased receiver -- one coherent unit, moved verbatim to `index_set_typed_array.rs`. index_set.rs drops to 1749 lines, leaving real headroom rather than landing one line under the cap. Mechanical move: the two functions are byte-identical, only the imports they need travelled with them and `lower_inline_dyn_typed_array_set` became `pub(super)` so its one caller can still reach it. cargo test -p perry-codegen --lib: 609 passed. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ 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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-codegen/src/expr/index_set_typed_array.rs`:
- Around line 85-111: Update the index-set lowering flow in index_set.rs to root
object across index and RHS lowering, and root index across RHS lowering using
StoreOperandGuard. After RHS lowering, re-read both guarded operands and pass
the refreshed values to lower_inline_dyn_typed_array_set, keeping both guards
alive through the helper call. Release the index/key guard before releasing the
object/receiver guard, including the corresponding path around the additionally
referenced lines.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 30d3bfff-a347-4ecc-a5dc-e3ce192cf85f
📒 Files selected for processing (3)
crates/perry-codegen/src/expr/index_set.rscrates/perry-codegen/src/expr/index_set_typed_array.rscrates/perry-codegen/src/expr/mod.rs
Comment on lines
+85
to
+111
| let (raw, idx_i64, kind) = { | ||
| let blk = ctx.block(); | ||
| let obj_bits = blk.bitcast_double_to_i64(obj_box); | ||
| let raw = blk.and(I64, &obj_bits, pointer_mask); | ||
| let slot = blk.lshr(I64, &raw, "3"); | ||
| let slot = blk.and(I64, &slot, "63"); | ||
| let entry_ptr = blk.gep( | ||
| "[64 x i64]", | ||
| "@PERRY_TA_KIND_CACHE", | ||
| &[(I64, "0"), (I64, &slot)], | ||
| ); | ||
| let entry_val = blk.load(I64, &entry_ptr); | ||
| let kind = blk.and(I64, &entry_val, "255"); | ||
| let idx_i64 = blk.fptosi(DOUBLE, idx_d, I64); | ||
| (raw, idx_i64, kind) | ||
| }; | ||
| let fast_ok = { | ||
| let blk = ctx.block(); | ||
| let idx_back = blk.sitofp(I64, &idx_i64, DOUBLE); | ||
| let is_int = blk.fcmp("oeq", &idx_back, idx_d); | ||
| let hdr_ptr = blk.inttoptr(I64, &raw); | ||
| let len = blk.load(I32, &hdr_ptr); | ||
| let len_i64 = blk.zext(I32, &len, I64); | ||
| let in_bounds = blk.icmp_ult(I64, &idx_i64, &len_i64); | ||
| blk.and(I1, &is_int, &in_bounds) | ||
| }; | ||
| ctx.block().cond_br(&fast_ok, &store_label, &slow_label); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Root the receiver and dynamic key across later lowering.
index_set.rs lowers obj_box, then idx_d, then the RHS before it calls this helper. That path has no StoreOperandGuard. If index or RHS lowering evacuates the heap, these SSA values can still reference from-space. This helper then dereferences obj_box on the fast path and passes idx_d to js_dyn_index_set on the slow path.
At the call site, guard object across index and RHS lowering. Guard index across RHS lowering. Re-read both before lower_inline_dyn_typed_array_set, keep the guards through the helper call, and release the key guard before the receiver guard.
As per coding guidelines, generated GC root stores must dominate every later site that may collect. Based on learnings, re-read guarded operands after RHS lowering and release nested guards from inner to outer.
Also applies to: 253-260
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-codegen/src/expr/index_set_typed_array.rs` around lines 85 -
111, Update the index-set lowering flow in index_set.rs to root object across
index and RHS lowering, and root index across RHS lowering using
StoreOperandGuard. After RHS lowering, re-read both guarded operands and pass
the refreshed values to lower_inline_dyn_typed_array_set, keeping both guards
alive through the helper call. Release the index/key guard before releasing the
object/receiver guard, including the corresponding path around the additionally
referenced lines.
Sources: Coding guidelines, Learnings
proggeramlug
added a commit
that referenced
this pull request
Aug 4, 2026
…#7369) * fix(lint): split index_set.rs, over the 2000-line cap since #7342 (#7366) `scripts/check_file_size.sh` exits 1 on main HEAD: `crates/perry-codegen/src/expr/index_set.rs` is 2035 lines against a 2000 cap. It crossed in #7342. That script runs inside the `lint` job, which is a REQUIRED context -- so this is the second independent way `lint` was red on main today (the first was rustfmt on linker.rs, #7361). A required check that is red on main blocks nothing; it means every merge is a bypass. The split follows the recipe in the script's own failure message: extract a topical group into a sibling module. `lower_inline_dyn_typed_array_set` and its `emit_inline_ta_int_store` helper are the guarded inline typed-array store for a type-erased receiver -- one coherent unit, moved verbatim to `index_set_typed_array.rs`. index_set.rs drops to 1749 lines, leaving real headroom rather than landing one line under the cap. Mechanical move: the two functions are byte-identical, only the imports they need travelled with them and `lower_inline_dyn_typed_array_set` became `pub(super)` so its one caller can still reach it. cargo test -p perry-codegen --lib: 609 passed. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF Co-authored-by: Ralph Küpper <ralph@skelpo.com> * docs(plan): both statepoint adoption gates are closed; record the platform matrix (#7367) The plan still said statepoints were aarch64-only (#7321), that the matrix "therefore runs on macos-14", that `statepoints-refuse-x86` pinned the refusal, and it spelled the knob `PERRY_STATEPOINTS` four times. None of that is true now, and this document is what the adoption decision gets made from. What actually changed: - x86-64 is unblocked. `_Unwind_GetGR(ctx, 7)` does segfault and cannot be fixed as stated -- libgcc tracks only the columns CFI restores and RSP is derived, not tracked. #7349 stopped asking for it and derives the SP-relative base from `_Unwind_GetCFA`, with a per-arch return-address adjustment (x86-64 `call` pushes one, aarch64 `bl` does not). x86-64 Linux is a first-class arm. - Windows works via RtlVirtualUnwind (#7355), the one walker with no Itanium unwinder beneath it. - aarch64+ELF is now covered too (#7360) -- the only shape where LLVM spells 32-bit stack-map fields `.word`. - One mechanism, not two: PERRY_STATEPOINTS and the plain-map bridge are deleted, so the kill-policy line about "a mode that still exists" no longer applies to this pair. - The gate proves something now. Until today the Unix arms reported 7 frames and ZERO locations -- they would have passed with a walker that visited nothing. #7359's deep-collect probe took them to 221 locations. - watchOS/visionOS are not blocked by Perry: they build on stable without `dyn-eval`, and fail three crates away in psm's Mach-O guard. So the remaining adoption gate is `llvm-inprocess` becoming a default cargo feature, plus sequencing step 2 (root density) -- adopting today would regress binary size on root-dense code. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF Co-authored-by: Ralph Küpper <ralph@skelpo.com> * gc: admit two provably-leaf helpers, and measure that it buys nothing js_gc_register_global_root was the most frequent non-leaf callee in the probe suite (148 call sites) and is provably GC-leaf: its whole body is runtime_write_barrier_root_heap_word -- which js_write_barrier_root_heap_word, already CannotCollect, wraps in one line -- plus a TLS Vec::push. The "malloc count threshold" trigger does not apply to that push: the counter is MALLOC_STATE.objects.len(), a registry of Perry GC objects, and the #[global_allocator] is plain mimalloc/System with no GC hook. js_typed_feedback_maybe_dump_trace joins its already-admitted family siblings. Measured A/B on the same tree, and the result is a null: probe safepoints roots total bytes __text 06_string_retention 105 -> 100 27=27 0 -4 B 09_try_catch_roots 343 -> 339 259=259 0 -4 B 11_collect_at_depth 120 -> 117 36=36 0 -4 B Root counts are IDENTICAL. The 40 safepoints removed across the suite were all rootless, and a rootless safepoint costs essentially nothing -- which is what docs/engine-plan.md already says: "the axis is not 'statepoints are bigger', it is 'roots are bigger'". Recording it as evidence: the safepoint-count lever is not the binary-size lever, so sequencing step 2 must attack live-root SETS. Two tests come with it. One pins the wrapper's classification to the barrier it wraps. The other pins js_nanbox_string OUT of the allowlist: at 120 call sites it is the obvious next candidate and reads as pure bit manipulation, but its null guard calls js_string_from_bytes to allocate an empty string. Probe suite 11/11 byte-identical under forced evacuation + verification. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug
added a commit
that referenced
this pull request
Aug 4, 2026
…7368) * fix(codegen): statepoint report counted zero safepoints since #7348 #7348 deleted the explicit bridge and with it the only callers of note_statepoint and note_skipped -- they lived in the bridge, which counted safepoints as it emitted them. The methods survived with no callers, so statepoints, relocations, max_live_roots, skipped_non_safepoints, live_roots_histogram and both by-callee maps went structurally zero in production. A real compile printed "0 statepoints emitted" while its binary carried 120. Counting at IR-emission time cannot work any more, and that is the lesson: Perry no longer decides which calls become safepoints -- RewriteStatepointsForGC does, inside LLVM. The only honest source is the compact-map rewrite, which already parses the assembly LLVM emitted and computed these exact numbers before dropping them into log::debug!. The report reads from there now: 120 safepoints across 6 function(s) in 1 module(s) 36 live roots recorded, 0.30 per safepoint An absent measurement no longer renders as a measured zero: gc_map.modules == 0 means "never reported", the text report says UNAVAILABLE rather than printing zeros, and JSON carries gc_map separately from totals so a consumer can tell them apart. schema_version -> 2. The CI gate now asserts the counts, not just the label. --only-backend rs4gc passed throughout the regression -- the label was right, the numbers were fiction. It now also requires records > 0 and roots > 0; verified against a synthetic report with the #7348 shape, where the label check still reports 9 functions green while the count checks exit 1. Second round of dead counters here (#7362 removed four that never had a writer at all). The new test documents why the first invariant missed this one: every_rendered_counter_has_a_writer called the mutators itself, so "has a writer" passed while "is written" was false. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF * gc: admit two provably-leaf helpers, and measure that it buys nothing (#7369) * fix(lint): split index_set.rs, over the 2000-line cap since #7342 (#7366) `scripts/check_file_size.sh` exits 1 on main HEAD: `crates/perry-codegen/src/expr/index_set.rs` is 2035 lines against a 2000 cap. It crossed in #7342. That script runs inside the `lint` job, which is a REQUIRED context -- so this is the second independent way `lint` was red on main today (the first was rustfmt on linker.rs, #7361). A required check that is red on main blocks nothing; it means every merge is a bypass. The split follows the recipe in the script's own failure message: extract a topical group into a sibling module. `lower_inline_dyn_typed_array_set` and its `emit_inline_ta_int_store` helper are the guarded inline typed-array store for a type-erased receiver -- one coherent unit, moved verbatim to `index_set_typed_array.rs`. index_set.rs drops to 1749 lines, leaving real headroom rather than landing one line under the cap. Mechanical move: the two functions are byte-identical, only the imports they need travelled with them and `lower_inline_dyn_typed_array_set` became `pub(super)` so its one caller can still reach it. cargo test -p perry-codegen --lib: 609 passed. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF Co-authored-by: Ralph Küpper <ralph@skelpo.com> * docs(plan): both statepoint adoption gates are closed; record the platform matrix (#7367) The plan still said statepoints were aarch64-only (#7321), that the matrix "therefore runs on macos-14", that `statepoints-refuse-x86` pinned the refusal, and it spelled the knob `PERRY_STATEPOINTS` four times. None of that is true now, and this document is what the adoption decision gets made from. What actually changed: - x86-64 is unblocked. `_Unwind_GetGR(ctx, 7)` does segfault and cannot be fixed as stated -- libgcc tracks only the columns CFI restores and RSP is derived, not tracked. #7349 stopped asking for it and derives the SP-relative base from `_Unwind_GetCFA`, with a per-arch return-address adjustment (x86-64 `call` pushes one, aarch64 `bl` does not). x86-64 Linux is a first-class arm. - Windows works via RtlVirtualUnwind (#7355), the one walker with no Itanium unwinder beneath it. - aarch64+ELF is now covered too (#7360) -- the only shape where LLVM spells 32-bit stack-map fields `.word`. - One mechanism, not two: PERRY_STATEPOINTS and the plain-map bridge are deleted, so the kill-policy line about "a mode that still exists" no longer applies to this pair. - The gate proves something now. Until today the Unix arms reported 7 frames and ZERO locations -- they would have passed with a walker that visited nothing. #7359's deep-collect probe took them to 221 locations. - watchOS/visionOS are not blocked by Perry: they build on stable without `dyn-eval`, and fail three crates away in psm's Mach-O guard. So the remaining adoption gate is `llvm-inprocess` becoming a default cargo feature, plus sequencing step 2 (root density) -- adopting today would regress binary size on root-dense code. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF Co-authored-by: Ralph Küpper <ralph@skelpo.com> * gc: admit two provably-leaf helpers, and measure that it buys nothing js_gc_register_global_root was the most frequent non-leaf callee in the probe suite (148 call sites) and is provably GC-leaf: its whole body is runtime_write_barrier_root_heap_word -- which js_write_barrier_root_heap_word, already CannotCollect, wraps in one line -- plus a TLS Vec::push. The "malloc count threshold" trigger does not apply to that push: the counter is MALLOC_STATE.objects.len(), a registry of Perry GC objects, and the #[global_allocator] is plain mimalloc/System with no GC hook. js_typed_feedback_maybe_dump_trace joins its already-admitted family siblings. Measured A/B on the same tree, and the result is a null: probe safepoints roots total bytes __text 06_string_retention 105 -> 100 27=27 0 -4 B 09_try_catch_roots 343 -> 339 259=259 0 -4 B 11_collect_at_depth 120 -> 117 36=36 0 -4 B Root counts are IDENTICAL. The 40 safepoints removed across the suite were all rootless, and a rootless safepoint costs essentially nothing -- which is what docs/engine-plan.md already says: "the axis is not 'statepoints are bigger', it is 'roots are bigger'". Recording it as evidence: the safepoint-count lever is not the binary-size lever, so sequencing step 2 must attack live-root SETS. Two tests come with it. One pins the wrapper's classification to the barrier it wraps. The other pins js_nanbox_string OUT of the allowlist: at 120 call sites it is the obvious next candidate and reads as pure bit manipulation, but its null guard calls js_string_from_bytes to allocate an empty string. Probe suite 11/11 byte-identical under forced evacuation + verification. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com> * fix(ci): report assertion pinned to a probe Windows cannot compile Three review fixes on #7368. The report assertion ran on 09_try_catch_roots, which contains four `try` blocks. RS4GC cannot rewrite WinEH funclet pads, so linker.rs's rs4gc_funclet_refusal rejects that probe on windows-msvc -- the probe loop above tolerates it by grepping the compile log for "funclet", but this step did not. A gate pinned to a probe that cannot compile on one arm fails for a reason unrelated to its subject. The portable assertion now uses 11_collect_at_depth (no `try`, compiles on all four arms); 09_try_catch_roots keeps its own non-Windows step so the try-specific coverage that justified deleting the bridge is not lost. The gc_map doc claimed records/roots would be ABSENT when unmeasured. They are plain u64 fields on a plain derive and always serialise; `modules` is the sentinel. Fixed to describe what the code actually does -- the same class of comment-vs-code drift this PR exists to clean up. The "map never reported" guard fired for any --require-*/--print, including fields that live in `totals` and are counted at IR-emission time whether or not the rewrite ran. Now scoped to map-backed fields: --require-positive textual_calls is answered from its measured value (verified exit 0) while --require-positive records still fails on an unreported map (exit 1). Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
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.
scripts/check_file_size.shexits 1 on main HEAD:crates/perry-codegen/src/expr/index_set.rsis 2035 lines against a 2000 cap. It crossed in #7342.That script runs inside the
lintjob, which is a required context. So this is the second independent waylintwas red on main today — the first was rustfmt onlinker.rs(#7361). Fixing one didn't fix the other, and I reported lint clean after #7361 having only checkedcargo fmt. A required check that's red on main blocks nothing; it means every merge is a bypass.The split follows the recipe in the script's own failure message — extract a topical group into a sibling module.
lower_inline_dyn_typed_array_setplus itsemit_inline_ta_int_storehelper are one coherent unit (the guarded inline typed-array store for a type-erased receiver), moved verbatim intoindex_set_typed_array.rs.index_set.rsindex_set_typed_array.rsI deliberately took a 286-line block rather than the minimum needed. Landing one line under a cap on a file that just grew past it buys nothing.
Mechanical: the two functions are byte-identical; only the imports they use travelled with them, and
lower_inline_dyn_typed_array_setbecamepub(super)so its single caller reaches it.check_file_size.shPASS ·cargo test -p perry-codegen --lib609 passed ·cargo fmt --checkclean.Summary by CodeRabbit
Performance
Bug Fixes