refactor: split gc/layout.rs and codegen/artifacts.rs under the 2000-line cap (unblocks lint on main) - #8212
Merged
Merged
Conversation
…line cap #8204 pushed both files over scripts/check_file_size.sh's hard cap (layout.rs 1975 -> 2110, artifacts.rs 2000 -> 2005), turning the required lint context red on main for every PR. Pure code moves, no logic change: - gc/layout.rs: the typed-shape layout installation protocol (TypedShapeProof, mask_words, init_typed_shape_layout, install_typed_shape_layout_slow, typed_shape_layout_entry, js_gc_init_typed_shape_layout, js_gc_declare_typed_shape_layout) moves to gc/layout/typed_shape.rs, next to the existing layout/slot_mask.rs. 2110 -> 1778 lines. The two extern "C" entry points keep their crate::gc:: paths via an explicit named re-export. - codegen/artifacts.rs: synthesized_ctor_param_count moves to a new sibling codegen/ctor_arity.rs. 2005 -> 1930 lines. Claude-Session: https://claude.ai/code/session_01AHvBYz7E6wWKv8kmvLLGpj
|
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 (2)
📝 WalkthroughWalkthroughThe change extracts constructor arity calculation and typed-shape layout installation into dedicated modules. Existing codegen integration and typed-shape C ABI paths remain available through module wiring and re-exports. ChangesConstructor and typed-shape layout modularization
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
added 2 commits
August 16, 2026 16:45
…ray site Pure path rename in the exact callsite multiset: the one keys_array access inside the moved typed-shape install block now lives in gc/layout/typed_shape.rs (raw_member_files 65 -> 66, total sites unchanged). Claude-Session: https://claude.ai/code/session_01AHvBYz7E6wWKv8kmvLLGpj
proggeramlug
pushed a commit
that referenced
this pull request
Aug 16, 2026
Rebase moved the base to current main, so both arms were rebuilt there and the whole measurement retaken. Counters are bit-identical (releases == allocs, residue constant at 65,915) and peak RSS reproduces within 0.3 MB, so none of #8204/#8196/#8211/#8212/#8162 moves this residue. Also records, rather than rounds away, the fixed +80 KB per-process startup cost the change adds: it is page-granular first touch, not code size (binary +80 B, __TEXT unchanged) and not the pool data (144 B of empty Vec headers). Claude-Session: https://claude.ai/code/session_01AHvBYz7E6wWKv8kmvLLGpj
proggeramlug
pushed a commit
that referenced
this pull request
Aug 16, 2026
Rebase moved the base to current main, so both arms were rebuilt there and the whole measurement retaken. Counters are bit-identical (releases == allocs, residue constant at 65,915) and peak RSS reproduces within 0.3 MB, so none of #8204/#8196/#8211/#8212/#8162 moves this residue. Also records, rather than rounds away, the fixed +80 KB per-process startup cost the change adds: it is page-granular first touch, not code size (binary +80 B, __TEXT unchanged) and not the pool data (144 B of empty Vec headers). Claude-Session: https://claude.ai/code/session_01AHvBYz7E6wWKv8kmvLLGpj
This was referenced Aug 16, 2026
proggeramlug
pushed a commit
that referenced
this pull request
Aug 16, 2026
Rebase moved the base to current main, so both arms were rebuilt there and the whole measurement retaken. Counters are bit-identical (releases == allocs, residue constant at 65,915) and peak RSS reproduces within 0.3 MB, so none of #8204/#8196/#8211/#8212/#8162 moves this residue. Also records, rather than rounds away, the fixed +80 KB per-process startup cost the change adds: it is page-granular first touch, not code size (binary +80 B, __TEXT unchanged) and not the pool data (144 B of empty Vec headers). Claude-Session: https://claude.ai/code/session_01AHvBYz7E6wWKv8kmvLLGpj
proggeramlug
added a commit
that referenced
this pull request
Aug 17, 2026
…x cells (#7933 follow-up) (#8208) * fix(async): release a completed plain-async activation's box cells for reuse (#7933 follow-up) The async-to-generator transform's #7933 release cleared cells but kept them registered and malloc-resident forever: ~500 B of cell + registry bytes per completed activation, ~119 MB over an asyncpipe_big run whose live heap is ~250 KB. Replace the LocalSet(id, undefined) release with a Stmt::ReleaseBoxes HIR statement that codegen lowers to js_*box_release: clear + de-register + park the cell in a quarantine that drains into a per-kind free pool at the outermost microtask-pump boundary once the task queue is empty; js_*box_alloc* then reuses pooled cells instead of touching std::alloc. Also release the state-machine control cells, with parked values chosen so a stray duplicate resume takes byte-for-byte the pre-release terminal path (bool cells park true = the done short-circuit; i32 cells park -1 = no dispatch case). Claude-Session: https://claude.ai/code/session_01AHvBYz7E6wWKv8kmvLLGpj * test(transform,runtime): cover the ReleaseBoxes shape; route release plausibility through the canonical predicate Claude-Session: https://claude.ai/code/session_01AHvBYz7E6wWKv8kmvLLGpj * test(codegen): pin the ReleaseBoxes lowering — kind selection, capture path, hint skip Claude-Session: https://claude.ai/code/session_01AHvBYz7E6wWKv8kmvLLGpj * docs: changelog fragment for #8208 Claude-Session: https://claude.ai/code/session_01AHvBYz7E6wWKv8kmvLLGpj * fix(async,gc): close the ReleaseBoxes id-remap holes and re-argue the box exemption Follow-up hardening on the #8208 release/reuse change, from an audit of the 94 exhaustive-match arms the new `Stmt::ReleaseBoxes` variant required. Six sites were NOT among those 94, because `ReleaseBoxes` falls into a pre-existing `_ => {}` catch-all there — so rustc said nothing. Three of them renumber LocalIds, which is exactly the case the variant's own doc comment declares incorrect: an unremapped `PreallocateBoxes` merely allocates a cell nobody reads, but an unremapped `ReleaseBoxes` releases a STILL-LIVE local's cell and hands it to the next allocation. None is reachable today — intra-module inlining runs before the async transform, the cross-module harvest refuses bodies containing a release, and the two max-id scans feed a `next_local_id` computed earlier — but that safety rests entirely on pipeline ordering that nothing enforces. Remapped rather than left latent: - `inline/substitute.rs` `substitute_locals_in_stmts_inner` — the neighbouring prealloc arm already remaps (issue #569); the release now does too. - `perry-hir/src/analysis.rs` `remap_local_ids_in_stmt{,_propagating}` — the canonical HIR remappers, whose own doc says to keep the variant list in sync. - `generator/per_iteration.rs` `rename_in_stmt` — a LocalId renamer inside the generator transform itself; its `each_expr_mut` helper only reaches ids that live inside an Expr, so all three bare-id-list variants were walked past. - `generator/id_scan.rs` and `deforest/walk.rs` max-id scans now include the release ids, matching the deliberate #1029/#5143 defence on the prealloc arm. - `perry-codegen/src/boxed_vars.rs` keeps NOT collecting release ids (a reclamation hint must not decide a local's representation) but says so explicitly instead of falling into the catch-all. The invariant those last two lean on — the transform never releases an id it did not also preallocate, or `emit_release_boxes` skips it and the release goes silently inert with every test still green — is now asserted in both directions (`every_released_id_is_also_preallocated`, with vacuity guards). gc_root_dominance_check.py: - The "box" immovable-source exemption rested on "boxes are never freed", which this PR falsified, while its probe only grepped for `dealloc(`/`arena_alloc(` — all of which a *recycle* path passes. The exemption stayed green on a dead premise, which the script's own docstring calls strictly worse than no exemption. Re-argued on the property #8208 actually preserves (cell memory is never returned to the allocator, so an address never stops naming box-cell memory and can never become another kind of object), and the probe now also requires the reuse path to stay quarantine-gated. Sabotage-tested: bypassing the quarantine and introducing a real `dealloc` each turn it red. - Added the three `js_*box_release` names to NONCOLLECTING. This PR had added them to `gc_call_effects.rs` only, breaking the documented one-way containment — the same one-sided drift that cost #7510 358 spurious violations. `cannot_collect_stays_a_subset_of_the_checker_authority` now machine-checks that relation instead of trusting four comments that assert it. Also refreshes the monotonicity docs the release invalidated, including the load-bearing correctness argument in `expr/literals_vars.rs` that let a `box_ptr` outlive a collecting call on the strength of "never freed". Claude-Session: https://claude.ai/code/session_01AHvBYz7E6wWKv8kmvLLGpj * fix(hir,transform): scope the new id-remap arms strictly to ReleaseBoxes The previous commit grouped `ReleaseBoxes` with `PreallocateBoxes` / `PreallocateTdzBoxes` in `analysis.rs`'s two canonical remappers and in `per_iteration.rs`'s renamer. In those three places the prealloc variants were previously UNHANDLED, so the grouping quietly started remapping them too — a behaviour change to existing programs riding along inside a PR about a new statement variant. That prealloc gap is real but pre-existing and benign in its failure direction: an unremapped prealloc allocates a cell nobody reads, whereas an unremapped release frees a live local's cell. Closing it can shift codegen and deserves its own evidence, so it is documented at both sites and left alone. With this, the hardening changes alter behaviour only for `ReleaseBoxes`, which no pass in the tree can reach today — so they cannot move codegen output at all. The sites where `ReleaseBoxes` was grouped with an arm that ALREADY handled the prealloc variants (`inline/substitute.rs`, `generator/id_scan.rs`, `deforest/walk.rs`) are unaffected and keep the grouping. Claude-Session: https://claude.ai/code/session_01AHvBYz7E6wWKv8kmvLLGpj * docs: re-measure #8208 on 07c8040 and record the hardening Corrects the residue figure (65,906 -> 65,915 after the rebase onto #8204/#8196, neither of which moved it), and reports instructions and peak RSS together per corpus row against a stated noise floor. Claude-Session: https://claude.ai/code/session_01AHvBYz7E6wWKv8kmvLLGpj * docs: record the flush-boundary limitation and the exit-path coverage Adds the measured degenerate case (an await cascade with no timer or I/O never reaches the flush boundary, so releases are performed but never harvested: +1.32% instructions, +0.3 MB RSS) and the seven-shape exit-path fixture that matches the Node oracle byte-for-byte on both arms. Claude-Session: https://claude.ai/code/session_01AHvBYz7E6wWKv8kmvLLGpj * test(gap): pin every async exit path across the #8208 box release Behavioural half of the #8208 gate. Drives normal return, throw after an await, early return from inside a loop after a suspend, await on a rejected promise, try/finally across a suspend on both terminal arms, loop-created closures capturing a per-iteration binding across a suspend, and async-generator .return() versus a full drain — 400 iterations each — and prints values that only come out right if every cell outlived its last reader. A cell released while still reachable, or reused by a second live activation, is a wrong answer rather than a crash, which is why this asserts printed values against the Node oracle instead of merely running to completion. Claude-Session: https://claude.ai/code/session_01AHvBYz7E6wWKv8kmvLLGpj * docs: re-measure #8208 with both arms rebuilt at b8d32ab Rebase moved the base to current main, so both arms were rebuilt there and the whole measurement retaken. Counters are bit-identical (releases == allocs, residue constant at 65,915) and peak RSS reproduces within 0.3 MB, so none of #8204/#8196/#8211/#8212/#8162 moves this residue. Also records, rather than rounds away, the fixed +80 KB per-process startup cost the change adds: it is page-granular first touch, not code size (binary +80 B, __TEXT unchanged) and not the pool data (144 B of empty Vec headers). Claude-Session: https://claude.ai/code/session_01AHvBYz7E6wWKv8kmvLLGpj * perf(runtime): thread the box reuse pool through the cells, deleting the side table The free pool was a `Vec<usize>` per kind: one 8-byte slot per pooled cell, on top of the cell. Its high-water mark is ~330 cells per unit of PEAK CONCURRENCY (measured: resident_cells/SIZE is 329-334 across a 16x sweep of the fan-out width), held for the life of the thread, so at SIZE=200 it was ~1 MB of side table and made small async workloads a net RSS REGRESSION. A free cell's own 8 bytes are dead, and every box kind is exactly pointer-sized (now asserted at compile time), so the free list is threaded through the cells themselves and costs zero side-table bytes. Overwriting the cell is why only POST-QUARANTINE cells join the list: a quarantined cell must keep the parked terminal value a stray duplicate resume reads, and `flush_released_boxes` publishing it is exactly the point at which the task queue is empty and no such resume can exist. The checker probe is updated to fail if a release ever publishes directly. The quarantine is deliberately NOT shrunk on flush: it refills to the same size every interval, and handing the buffer back cost +5.3 MB peak RSS at BATCHES=1200 in allocator churn (measured). Measured on asyncpipe, matched arms at b8d32ab (peak RSS, best-of-5): BATCHES 30 60 90 120 300 600 1200 delta MB +0.80 +0.92 -0.19 -0.19 -8.17 -25.06 -69.73 Crossover moves from ~200 batches to between 60 and 90, and the 1200 row improves from -63.8 MB to -69.7 MB. stdout is byte-identical at every size. The residual sub-crossover cost is NOT this pool -- see the changelog. Claude-Session: https://claude.ai/code/session_01AHvBYz7E6wWKv8kmvLLGpj * docs: record the RSS sweep, the remaining floor, and why a cap cannot fix it Claude-Session: https://claude.ai/code/session_01AHvBYz7E6wWKv8kmvLLGpj * docs: record why no earlier publish point is safe (per-kind split refuted) Claude-Session: https://claude.ai/code/session_01AHvBYz7E6wWKv8kmvLLGpj * docs: final numbers on matched 9233429 arms; gc-ratchet shared_ci OK Claude-Session: https://claude.ai/code/session_01AHvBYz7E6wWKv8kmvLLGpj * fix(async): publish box cells at activation reachability zero * test(async): close PR review and CI coverage gaps * ci: classify the stale loop safepoint assertion * ci: record inherited codegen integration failures * fix(async): complete final review coverage --------- 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.
lintis red onmain; this unblocks it./scripts/check_file_size.shexits 1 against the hard 2000-line cap:Bisected to #8204 (
bf8fd868e, the 56 → 48 B header shrink):gc/layout.rswent 1975 → 2110, andcodegen/artifacts.rswas sitting at exactly 2000 — right on the cap, so any addition at all broke it.check_file_size.shruns inlint, a required status context, so every open PR is currently blocked behind a failure that has nothing to do with it. That is the "required and red for a pre-existing reason" state — it trains everyone to bypass, and the next genuine failure arrives indistinguishable from the standing one.The split
codegen/artifacts.rscodegen/ctor_arity.rs(new)gc/layout.rsgc/layout/typed_shape.rs(new)Both land with real headroom rather than scraping under at 1999 —
artifacts.rsbeing at exactly 2000 is what made #8204 break it, and leaving it there would just re-arm the trap.This is relocation-only, and here is the evidence
A file-size fix is the easiest place for an accidental semantic change to hide behind a large mechanical diff, so the claim is checked rather than asserted. Comparing the multiset of non-blank source lines before against (remainder + extracted) after:
layout.rs→typed_shape.rsartifacts.rs→ctor_arity.rsAll six "vanished" lines reappear in adjusted form, and they are exactly the two categories a relocation produces:
super::shape_install::{words_contain_slot, words_intersect, hit, record}becameshape_install::…, because the extracted module sits insidegc/layout/and no longer needs thesuper::hop.fn synthesized_ctor_param_count(becamepub(super) fn, plususe super::ctor_arity::synthesized_ctor_param_count;at the call site.The 23 + 8 new lines are module headers,
usestatements and doc comments. No logic edits, no renames visible outside the modules.Seam choice
Both extractions are cohesive rather than "the last N lines":
typed_shape.rsis the typed-shape layout family,ctor_arity.rsis the synthesized-constructor arity helper and its single caller.Deliberately not touched, both near the cap and owned by in-flight work:
gc/cycle.rs(1991) andgc/barrier/mod.rs(1995).Note for anyone splitting in this area next: adding a matching
pub use <new_mod>::*;is an unused-import warning — hence a-D warningsfailure — when the extracted items are#[no_mangle]and reached by symbol rather than by path. The module needsmod <new_mod>;only, plus fixing any internalcrate::…::<moved_fn>call sites.No version bump.
Summary by CodeRabbit