perf(codegen): inline short heap string equality - #8597
Closed
proggeramlug wants to merge 2 commits into
Closed
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe compiler now emits inline length and payload checks for statically proven short heap strings. Longer strings and invalid handles use ChangesString equality optimization
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change localizes short heap-string equality handling and retains the existing fallback for other comparisons; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant StringEqualityLowering
participant ShortHeapEquality
participant JSStringEquals
StringEqualityLowering->>ShortHeapEquality: Lower statically proven heap-string equality
ShortHeapEquality->>ShortHeapEquality: Check handle, length, and payload bytes
ShortHeapEquality->>JSStringEquals: Compare longer strings or invalid handles
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
proggeramlug
marked this pull request as ready for review
August 22, 2026 09:56
proggeramlug
added a commit
that referenced
this pull request
Aug 22, 2026
…icts resolved (#8602) * fix(codegen): replace the pre-RS4GC optnone knob with a post-rewrite budget assertion and per-unit codegen stats (#8583) Three things, none of them an optimization-policy change. 1. `PERRY_LL_PREOPT_OPTNONE_INSTRS` is deleted. It stamped `optnone` BEFORE `rewrite-statepoints-for-gc`, and the new pass manager skips `mem2reg`/ `sccp` on an optnone function while RS4GC (a module pass keyed on the `gc` attribute) still runs — so a demoted function kept its root allocas unpromoted and the collector never saw them. The new `optnone_before_rs4gc_hides_every_root_from_the_collector` test shows the rewritten fixture with no `gc-live` bundle and its allocas intact. The cap defaulted to 0, so no build hit it; it is removed rather than calibrated. 2. `PERRY_LL_RS4GC_MAX_INSTRS`: after the rewrite and its verify, any function past the per-function budget (default 1.5 Mi instructions — between #8128's ~413k known-fine and ~2.1M known-hang) makes the unit fail with the function's name, its size before and after the rewrite, and the override spellings (`<n>`, `warn:<n>`, `0`). This is an assertion about relocation fan-out, not a fallback: nothing is demoted, #8421's contract that every function is optimized at the requested level stands, and an estimator miss now fails in seconds instead of hanging for hours. `rs4gc_budget_fires_only_on_the_rewritten_module` pins that the check is post-rewrite: the same fixture is under the budget before RS4GC and over it after. Both caches key on the new variable; the old key is gone. 3. `UnitCodegenStats`: under `PERRY_CODEGEN_UNIT_TIMINGS` the native unit pipeline prints, per unit, the function count, instruction totals and the widest function before and after RS4GC, the growth factor, and the rewrite/optimize/emit times; at freeze time it names each unit's widest function by estimated IR size before LLVM starts. A stuck build now says which function it is stuck on. * docs: changelog fragment for #8586 * perf(codegen): spill giant-function GC roots to a shadow frame to bound RS4GC fan-out (#8583) `rewrite-statepoints-for-gc` inserts one relocation per GC value live across each safepoint, so its cost scales with `live_roots × safepoints`. The Claude Code bundle's 68 MB minified entry function measured 795 root slots × ~106k safepoints; RS4GC grew it 439k → 6.5M instructions and the `-Os` middle-end then did not finish in practical time. Measured on that isolated unit: without RS4GC the whole thing optimizes at `-Os` in ~5s and 520 MB; the fan-out is the entire pathology. This makes the precise-root LOWERING a per-function choice. A function whose estimated relocation count (`root_slots × safepoint_sites`, counted from the HIR) exceeds `PERRY_ROOT_SPILL_RELOCATIONS` (default 4,000,000) keeps its roots in the heap shadow frame — the pre-#7370 lowering — instead of native statepoints, so RS4GC skips it. Nothing else changes: the function is still compiled at the requested optimization level (this is not an opt-level downgrade — see #8586's contract), the runtime already scans shadow-frame and stack-map roots in one walk, the frame pointer is kept so the FP-chain walker steps over the spilled frame, and each spilled function is reported at default verbosity. `=0` disables spilling (every function on statepoints, prior behavior); the estimate is also backstopped by #8586's post-RS4GC instruction budget, which fails loudly if a function the estimate missed still fans out. Mechanism: a `force_shadow_frame` flag on `LlFunction` routes `enable_shadow_frame_inner` / `reserve_shadow_slot` / `define_header` into the existing shadow path; `codegen/helpers::maybe_spill_roots_to_shadow_frame` sets it, before the frame is built, at the five sizing sites (function / method×2 / closure / module entry). No new lowering or runtime code — it reuses exactly what `PERRY_RS4GC=0` builds. Tests: a codegen test that a spilled function drops the statepoint strategy, keeps the frame pointer, and emits the shadow frame while its sibling keeps statepoints; `count_safepoint_sites` unit tests (including that nested closures are not counted); and an end-to-end differential GC test (`gc_root_spill_mixed_frames_8583`) that compiles one program with spilling off and aggressively on, then runs both under every moving-collector arm and requires byte-identical output — a mixed statepoint/shadow-frame stack whose live roots a relocating minor must find in both frame kinds. Both caches key on the new variable. * perf(codegen): CPU-aware default for native LLVM unit workers (#8583) The native codegen-unit pipeline hard-coded 2 concurrent LLVM workers (#8017, chosen for Windows pagefile pressure and applied on every platform), while the text path already scales with the machine. On the Claude Code `cli.js` — ~84 codegen units, many of them dense generated closures whose RS4GC pass + ISel run for minutes each — two-at-a-time was the dominant wall-clock bottleneck once the giant entry function's roots were spilled (#8583) so no unit fans out unboundedly and per-unit peak RSS is a bounded ~1-2 GiB. Default is now half the logical CPUs clamped to [2, 8] on non-Windows; Windows keeps 2 until its pagefile behavior under higher fan-out is measured. `PERRY_CODEGEN_UNIT_JOBS` overrides everywhere. Bounds test included. * perf(codegen): inline short heap string equality * docs(changelog): note short string equality optimization * chore: PR-key the fragments for #8589 and #8593 --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
Contributor
Author
|
Landed via #8602, which carries this branch's commits with the cache-key union conflicts against current #8586 merged first and added its own entry to the compile cache-key lists; #8589 and #8593 each append theirs at the same insertion point in Verified after resolution: Validation on the resolved tree: |
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.
Summary
Inline equality for statically proven heap strings when their lengths differ or their equal-length payloads contain at most three bytes. This targets the short identifier comparisons that dominate the tree-walking interpreter workloads in #8591 while keeping generic-key and long-string comparisons on the existing helper path.
Changes
js_string_equals.Related issue
Fixes #8591
Performance
Measured against
fefdc367bon the issue's quiet M1 mini with five shuffled interleaved repeats. Control and candidate used the same freshly rebuilt runtime archives and matching binary basenames. All 20 corpus programs were byte-exact against Node; onlyinterpandiso_missproduced different binaries, and no other corpus row moved.interpiso_missVerdict: CLEAN. No timing, instruction, or RSS result outside the two target rows moved more than 1%.
Test plan
cargo build --releaseclean (isolated target directory)test-files/and in the affected crateCommands/checks:
cargo test -p perry-codegen --lib --no-fail-fast— 1,133 passedcargo test -p perry-runtime --lib --no-fail-fastviascripts/test_affected_crates.sh— 2,633 passed, 4 ignoredscripts/test_affected_crates.sh— 1,025 passed; the only failure is the pre-existing staleext_zlib_covers_every_stdlib_symbol_the_flip_stripsratchet in untouched coderun_parity_tests.sh --filter test_strict_eq_string_literal_inlineagainst pinned Node 26.5.1 — passed, byte-exactpython3 scripts/check_test_registration.py— passedscripts/pre-tag-check.sh --quick— passedcargo fmt --all -- --check— passedScreenshots / output
Not applicable.
Checklist
feat:/fix:/docs:/chore:prefix convention used in the logCONTRIBUTING.mdand agree to the Code of ConductSummary by CodeRabbit
Performance
Bug Fixes
Tests