merge: land #8623, #8625, #8627 with metadata fixes and a non-vacuous GC fixture - #8628
Merged
Conversation
…#8620) `DEFAULT_ROOT_SPILL_RELOCATIONS` was 4,000,000, low enough to spill moderate-fan-out functions whose native RS4GC statepoints would have optimized fine. On an ~8M-relocation entry function, spilling to the shadow frame was *slower* than the fan-out it replaced (#8620), so the default paid shadow-frame overhead for nothing. Measured the RS4GC fan-out cliff with synthetic entry functions compiled at -Os with spilling OFF (`PERRY_ROOT_SPILL_RELOCATIONS=0`), timing the `@main` codegen unit: 8.0M -> ~325 s (finished) 16.0M -> ~235 s (finished) 32.0M -> ~511 s / 8.5 min (finished) 40.0M -> did not finish in 20 min 48.0M -> did not finish in 20 min Fan-out finishes in bounded time up to 32M and does not past 40M, so the default is raised to 32,000,000 — the largest estimate whose fan-out still finished. Below it fan-out is the cheaper lowering; above it fan-out risks not finishing and the shadow frame wins. The change is compile-time only (spilled `main` is run-once init) and is backstopped by the post-RS4GC instruction-budget assertion (#8586), which fails loudly rather than hanging if a function this estimate misses still fans out. Pins the new default in a unit test. Claude-Session: https://claude.ai/code/session_01HHAsEkP5A9Y5rGx6kprJ9j
Addresses #8619 for the typed-array PARAMETER case. A function that folds a spec-ABI-proven typed-array parameter into an accumulator (`let x = arr[i] + 1.0; s = s + x`) kept its numeric locals `x` and `s` as NaN-boxed GC roots with a per-write `js_write_barrier_root_nanbox`, and lowered `s`'s update to the opaque `js_dynamic_string_or_number_add` instead of an inline `fadd` — even though every value is a genuine Number. Root cause: the `number_by_construction` fixpoint's `numeric_view_value_or_undefined` (collectors/ptr_shape_numeric.rs) recognised a typed-array element read as "Number-or-undefined, never a pointer" only for a LOCAL view with a compiler-visible `TypedArrayNew` init — not for a spec-proven `TaPtr` parameter. So the fresh, read-derived `x` failed the numeric proof, which cascaded to the loop-carried accumulator `s = s + x`. Fix: the fixpoint now also treats a read off a `spec_ta_lens` binding as Number-or-undefined. `spec_ta_lens` is keyed exactly by `SpecParamRep::TaPtr` parameters, and `collectors::spec_abi_sites` admits a `TaPtr` only for `spec_ta_kind_is_numeric` kinds (the BigInt typed arrays are never `TaPtr`), so `arr[numeric_index]` off one is provably a Number in-bounds and `undefined` out of range, which `+` launders into a genuine Number (NaN at worst). The `rec(index)` guard is retained: a non-numeric key reads a property, which can be a pointer. Soundness rests on the entry contract, not the erased annotation, so a reassigned or unproven receiver is untouched. Measured on a 200000x4096 Float64Array reduction passed by parameter: the accumulator's per-iteration dynamic add + root barrier become a single `fadd` in a raw double slot — ~5x faster (5.1-7.3s -> ~1.0s), byte-identical output to the rooted build under every moving-GC configuration and to Node. Tests: unit (perry-codegen) `spec_ta_param_view_admits_read_derived_number_locals` and `ta_read_without_spec_proof_stays_dynamic` prove the fix is load-bearing; integration (perry) `gc_ta_view_accumulator_unroot_8619` is a rooted-vs-fix differential across the moving-GC matrix, covering Float64Array/Int32Array kinds and OOB/negative indices. Not covered: the issue's module-global reproducer — on main that read is still a runtime call (module-global read inlining, #8617, is unmerged), so its rooting is a secondary cost; extending the same proof to `module_global_proven_types` is the follow-up once the read inlines. Claude-Session: https://claude.ai/code/session_01HHAsEkP5A9Y5rGx6kprJ9j
…med size Two accuracy fixes to the report enrichment from #8579, both found by testing against a live compiled binary rather than trusting the code: - Duplicate function/static-data body detection keyed on an FNV-1a hash + size check. CodeRabbit correctly flagged that a hash collision at the same size could fabricate a false duplicate. Fixed properly rather than reworded: key directly on the exact byte slice (&[u8] is Ord) instead of hashing it, which is exact by construction and no more expensive to implement. - The "duplicate crate instance" finding's first draft claimed its total_bytes were recoverable shipped-binary size. Verified directly (md5 + objdump on the extracted archive members from a real compiled program) that perry-runtime and perry-stdlib DO redundantly compile some shared dependencies (gimli, confirmed byte-identical across their two separate .a archives) -- but a successful link only pulls ONE physical copy per symbol (a linker errors on true duplicate- symbol inclusion), so every byte attributed is real, in-use code in the shipped binary, not a duplicate sitting in it twice. Renamed duplicate_crate_versions -> duplicate_crate_instances in the JSON schema (unreleased, so free to fix) and set estimated_bytes to 0 for this finding so it can't misrank against suggestions that genuinely shrink the shipped binary. The report and suggestion text now say this is a compile-time/archive-size finding explicitly, instead of overclaiming a shipped-binary-size win that isn't there.
The fixture measured copied_objects=0 with a single GC cycle: `keep` pushed unboxed numbers, so the nursery never filled and the collector never moved anything. The differential still compared arithmetic, but as a GC test it could not fail for the reason it exists. Push a fresh object per iteration instead (~100 copied objects per cycle over ~600 cycles), and assert the two arms lower differently so a fixpoint that stops firing cannot make the comparison vacuous. Verified on a #8625-bearing release build: 14 runs across default, SCAVENGE_NURSERY_MB=1/2/4, GEN_GC=0, FORCE_EVACUATE=1 and VERIFY_EVACUATION=1 all agree at acc:34378500, matching the node oracle.
This was referenced Aug 22, 2026
|
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 (10)
📝 WalkthroughWalkthroughThe PR raises the native GC root-spilling threshold, extends number-by-construction analysis to proven numeric typed-array parameters, and corrects duplicate detection and reporting in ChangesGC root spilling threshold
Typed-array numeric inference
Compile size-report accuracy
Estimated code review effort: 4 (Complex) | ~45 minutes ✨ 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 |
4 tasks
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
2 participants
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.
Lands #8623, #8625 and #8627 together, with the metadata fixes and one test
strengthening they need.
What is in here
DEFAULT_ROOT_SPILL_RELOCATIONS4M → 32M, the measured RS4GC fan-out cliffspec_ta_lens→number_by_construction)--report-sizeduplicate-body detection keys on exact bytes, not an FNV-1a hashAudit
#8625 is a rooting removal, so the soundness chain was traced from source
rather than taken from the PR body:
judge_argadmitsTaPtronly for a pre-pass-proven,ready,numeric-kind, non-view typed-array binding (
spec_abi_sites.rs:727).of the raw ABI (
codegen/mod.rs:2763, "reassigned params would stale theentry-bound proofs"). This is the load-bearing one:
spec_ta_lensis keyedby the callee's
p.id, so "never reassigned" had to hold callee-side.spec_ta_lenskeeps onlyTaPtr { const_len: Some(_) }.spec_ta_kind_is_numeric's 9-arm match omitsBIGINT64/BIGUINT64, so aBigInt typed array — whose elements genuinely are pointers — can never reach
this path.
#8623 raises the threshold 8x, so the check that mattered is that the case
it was built for still spills: the Next
cli.js@mainestimate (~8.4e7) iswell above 32M. Its two tests pin both endpoints rather than just the constant.
Test strengthening (the reason this is a branch and not three merges)
gc_ta_view_accumulator_unroot_8619measuredcopied_objects=0over asingle GC cycle:
keeppushed unboxed numbers, so the nursery never filled andthe collector never relocated anything. The differential still compared
arithmetic, but as a GC test it could not fail for the reason it exists —
CLAUDE.md's "the gate runs but its subject never did".
Two fixes:
keepnow pushes a fresh object per iteration (~100 copied objectsper cycle over ~600 cycles), and the test asserts the two arms produce
different binaries, so a fixpoint that stops firing cannot make the comparison
vacuous.
Validation
Run against a #8625-bearing release build (
perry+ both-staticwrappers,archives confirmed rebuilt after the edit):
SCAVENGE_NURSERY_MB=1/2/4,GEN_GC=0,FORCE_EVACUATE=1,VERIFY_EVACUATION=1, both arms — all agree atacc:34378500, matching the node oracle.not inferred from an aggregate count.
cargo fmt --all --checkpass;cargo check --workspace --all-targetsexits 0.Metadata
8619-→8625-(fragments are PR-keyed;8619 is the issue).
its 154-line
Cargo.lockdiff was only version lines — no substantivechange lost.
Summary by CodeRabbit
Performance
Bug Fixes
perry compile --report-sizeaccuracy by preventing false duplicate-body matches and clarifying duplicate compilation reporting.Quality