Skip to content

fix(hir): memoize class-mutates-capture recursion to stop exponential HIR lowering - #10801

Closed
proggeramlug wants to merge 2 commits into
mainfrom
fix-10757-exponential-class-capture-lowering
Closed

proggeramlug wants to merge 2 commits into
mainfrom
fix-10757-exponential-class-capture-lowering

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #10757: compiling ethers 6.17.0 from real source never finished — HIR lowering of @noble/curves' weierstrass.js spun for 8+ minutes on one core before being killed.

It is a bounded superlinear blowup, not a true hang. Bisected weierstrass.js down to a single factory function (weierstrassPoints), then to a package-independent synthetic reproducer. Instrumented call counts to class_mutates_capture (in crates/perry-hir/src/lower/shared_mutable_capture.rs) grow from 252 (8 methods) → 35,770 (12 methods) → 688,870 (13 methods) as the Point class gains one arithmetic method at a time — fitting (Bⁿ-1)/(B-1) for n=9 (the hardcoded MAX_NESTED_CLASS_DEPTH) almost exactly, at branching factors B=1, 2, 3 respectively. It terminates, but not in any practical time for a realistic method count.

Root cause: Point's methods (double, add, fromAffine, multiplyUnsafe, …) each construct new Point(...), forwarding Point's own captured outer context (Fp, CURVE, …) — completely ordinary self-referential-class code. for_each_nested_capture was written to find a class genuinely nested inside another class's method body (class Outer { make() { return class Inner { ... } } }) by scanning member bodies for capture-forwarding constructions. A self-referential new Self(...) matches that same scan, so it misreads Point as nested inside Point and class_mutates_capture recurses back into the class it started from — once per self-constructing method, at every depth up to the cap — redoing the identical (class, id) subproblem from scratch each time.

Fix: memoize class_mutates_capture by (class_name, id), shared across every id detect_shared_in_body checks, with an in-progress set to break cycles (replacing the depth cap, which could in principle also have under-covered a legitimately deep but acyclic chain — the new termination argument is "finitely many distinct (class, id) pairs," not "bounded to 8 levels"). HIR output is byte-identical before/after on every fixture size small enough for the unfixed pass to finish (verified via --print-hir diff).

Instruction-count differential (perf stat -e instructions, --no-auto-optimize --no-link to isolate lowering):

fixture pristine fixed
8 self-constructing methods 2.34B 2.21B
12 self-constructing methods 26.2B (11.2× vs its own 8-method run) 2.26B (flat, +2%)

Real-world acceptance: with the fix, the entire ethers@6.17.0 dependency tree — ethers + @noble/curves + @noble/hashes + @adraffy/ens-normalize + aes-js, 153 modules — lowers and codegens natively (0 JS fallback) in about a minute, versus never finishing on main. Separate finding, not fixed here: the final link step for that full build fails on undefined references (perry_fn_..._crypto_ts__createHmac/pbkdf2Sync/randomBytes/createHash, and a WebSocket wrapper symbol) — this is ethers/src.ts/crypto/crypto.ts's export { createHash, createHmac, pbkdf2Sync, randomBytes } from "crypto" re-export shape not resolving against Perry's native crypto module, unrelated to HIR lowering. Reporting this as a candidate follow-up issue, not fixing it in this PR.

Also gave the parity harness's compile step a timeout (PERRY_COMPILE_TIMEOUT, default 300s, in run_parity_tests.sh) — it had none before (only the executed binary's run did, via PERRY_RUN_TIMEOUT), so a compiler hang on any one fixture would have wedged the whole harness rather than failing that fixture. The gap test below relies on this.

What I did not run

  • Did not resolve the separate ethers link-step failure described above (crypto/WebSocket native-module wiring) — out of scope for this issue per its own instructions ("if ethers then fails for a different reason, that is a separate finding").
  • Compile-tier of run_lint_gates.sh (SKIP_COMPILE_GATES=1; known-red on this Linux host per campaign notes).
  • Full gap sweep (host policy — fixed port 17891, shared with other agents).
  • cargo test --workspace (ran the full script-tier run_lint_gates.sh plus targeted cargo test -p perry-hir, not the whole workspace, given host build-time constraints for perry-ui-*/GTK4 crates unavailable on this Linux box).

Test plan

  • Reproduced on pristine origin/main (1a4fa6507e): isolated weierstrassPoints() factory (no @noble/curves dependency, hand-written stubs) hangs; bisected by method count (8→14) showing 0.14s → 1.48s → 31.1s → timeout.
  • test-files/test_gap_10757_self_referential_class_capture.ts added: fails as COMPILE_FAIL via the new harness timeout (22s) on pristine main; passes byte-identical to Node 26.5.1 with the fix (real harness run, not a hand probe).
  • cargo check --workspace --all-targets clean (0 warnings/errors) on the default dev profile, excluding the UI crates this Linux host can't build (missing system glib/GTK4 — pre-existing, unrelated).
  • cargo fmt --all -- --check clean.
  • cargo test -p perry-hir (the touched crate): all tests pass.
  • scripts/run_lint_gates.sh SKIP_COMPILE_GATES=1: 78 of 79 gates passed; the one failure is the pre-existing "Public benchmark evidence freshness" (ci: two reds on main fail every PR — gap-suite shard 5 parity regression (test_gap_10430) and a stale public benchmark baseline #10707), not touched by this change. git diff --stat confirmed clean (no destructive doc regen) after running gates.
  • Real ethers@6.17.0 acceptance: prints ethers-version:6.17.0, HIR lowering + codegen succeed for the full 153-module dependency tree; link step fails for the unrelated reason above (reported, not fixed).

Summary by CodeRabbit

  • Bug Fixes

    • Fixed compiler hangs and severe slowdowns when compiling self-referential classes that capture outer variables.
    • Preserved generated behavior while improving compilation reliability for affected code patterns.
  • Tests

    • Added regression coverage for self-referential class captures.
  • Chores

    • Added a configurable timeout to compiler steps in parity testing, preventing hangs from blocking test runs.

… HIR lowering (#10757)

A class whose own methods construct fresh instances of itself while the
class also captures an outer local (an everyday shape for arithmetic/
builder classes, e.g. @noble/curves' Point.double()/add() each returning
new Point(...)) made for_each_nested_capture misread the class as nested
inside itself. class_mutates_capture then recursed back into the same
class on every self-constructing method, at every depth up to the
hardcoded MAX_NESTED_CLASS_DEPTH cap, recomputing the identical (class,
id) subproblem from scratch each time -- exponential in the class's
method count, bounded only by that depth cap, so a single ordinary
elliptic-curve arithmetic class (weierstrass.js in ethers' @noble/curves
dependency) never finished lowering within any practical wait.

Memoize class_mutates_capture by (class_name, id), shared across every
id detect_shared_in_body asks about, and use an in-progress set to break
cycles instead of the depth cap (which could also, in principle, have
under-covered a legitimately deep but acyclic chain). Bisected on
weierstrass.js confirmed this is a bounded blowup, not a true hang:
instrumented call counts fit (B^9-1)/(B-1) almost exactly for branching
factors 1, 2, 3 as the class grows by one method at a time. HIR output
is byte-identical before/after on every fixture size small enough for
the unfixed pass to complete.

Also give the parity harness's compile step a timeout
(PERRY_COMPILE_TIMEOUT, default 300s) -- it previously had none (only
the executed-binary run did), so a compiler hang on any one fixture
wedged the whole harness instead of failing that fixture.

Add test-files/test_gap_10757_self_referential_class_capture.ts: fails
via the harness's compile timeout on unfixed main, passes byte-identical
to node with the fix.
@coderabbitai

coderabbitai Bot commented Sep 20, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: cd48c22f-4d4e-451f-9d01-90f388dcb1e6

📥 Commits

Reviewing files that changed from the base of the PR and between b9ba951 and 69700bb.

📒 Files selected for processing (4)
  • changelog.d/10801-exponential-class-capture-lowering.md
  • crates/perry-hir/src/lower/shared_mutable_capture.rs
  • run_parity_tests.sh
  • test-files/test_gap_10757_self_referential_class_capture.ts

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The HIR lowering pass now memoizes recursive class-capture analysis and detects cycles. A regression fixture covers self-referential class captures. The parity harness now limits compiler execution with PERRY_COMPILE_TIMEOUT.

Changes

Class Capture Lowering

Layer / File(s) Summary
Memoized recursive capture analysis
crates/perry-hir/src/lower/shared_mutable_capture.rs, test-files/test_gap_10757_self_referential_class_capture.ts, changelog.d/10801-exponential-class-capture-lowering.md
The lowering pass caches (class, id) results and breaks recursive re-entry with a visiting set. The regression test exercises a self-referential captured class. The changelog records the fix.
Compile timeout protection
run_parity_tests.sh
The parity harness validates PERRY_COMPILE_TIMEOUT and applies it to the initial compile and JavaScript-runtime retry.

Priority: ⬆️ High

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: High

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies the main fix: memoizing class-mutates-capture recursion to prevent exponential HIR lowering.
Description check ✅ Passed The description is substantially complete and directly explains the issue, root cause, implementation, scope, related issue, and test results. It omits explicit Changes, Related issue, and Checklist h…
Linked Issues check ✅ Passed Issue #10757 requires identifying why ethers lowering does not finish and fixing the responsible lowering behavior. The PR identifies exponential recomputation in class_mutates_capture for self-co…
Out of Scope Changes check ✅ Passed The changed lowering code, regression fixture, changelog entry, and parity compile timeout all support issue #10757. The PR does not add a fix for the separate crypto or WebSocket link-step failure. N…
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. (1 skipped: 1 …
✨ Finishing Touches 💡 1
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Queued as the next train, behind #10795 which is validating now.

The diagnosis is the strongest part and worth naming precisely: (Bⁿ-1)/(B-1) at n=9, fitting 252 → 35,770 → 688,870 as branching goes 1 → 2 → 3, is what turns "it hangs" into "it is a bounded superlinear blowup with a known exponent and a known cap". Those are different bugs with different fixes, and the second one is fixable. Bisecting real weierstrass.js down to weierstrassPoints and then to a package-independent synthetic reproducer is the part that makes it reviewable by someone without ethers checked out.

Replacing the depth cap with a termination argument is the right trade and you said why. "Finitely many distinct (class, id) pairs" is a property of the algorithm; MAX_NESTED_CLASS_DEPTH = 9 was a bound that happened to hold, and as you note it could also have under-covered a legitimately deep acyclic chain. Swapping a magic number for an invariant is worth more than the speedup.

class_mutates_capture misreading a self-referential new Self(...) as "nested inside itself" is a satisfying root cause, because the scan it reuses was written for class Outer { make() { return class Inner {} } } and a self-construction is genuinely indistinguishable from that shape by the criterion it applies.

Three things I will check when it lands, flagged now:

  • The byte-identical --print-hir claim is the safety property, since this changes HIR lowering for every program. It is verified only on fixture sizes small enough for the unfixed pass to finish, which is the honest limit — so the train's gap sweep is where the larger-input behaviour actually gets exercised. I will weight the areas toward class, extends and closure.
  • PERRY_COMPILE_TIMEOUT is a real gap you closed incidentally: the parity harness timed the executed binary but not the compile, so a compiler hang wedged the whole harness rather than failing one fixture. That is the same shape as three other things fixed on main today — a check that cannot distinguish "still working" from "stuck". Good catch; it deserves more than a parenthetical in the PR.
  • The link failure on ethers/src.ts/crypto/crypto.ts's export { createHash, createHmac, pbkdf2Sync, randomBytes } from "crypto" re-export shape is correctly reported and not fixed here. Please do file it — a re-export form that does not resolve against Perry's native crypto is exactly the kind of thing that gets rediscovered from scratch in three months. Compiling ethers from source never finishes: HIR lowering of @noble/curves' weierstrass.js spins indefinitely on one core #10757 should stay open or be superseded by that issue rather than closed outright, since "ethers compiles" is not yet "ethers links".

One note on ordering: main moved five times in the last two hours (v0.5.1614 → v0.5.1619 in flight), including two fixes to the same fcmp operand-type bug that broke Buffer.readFloat* and width_aware_buffer_kernels. If this PR's base drifts far enough to conflict, the train will rebase it rather than asking you to.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Landed via merge train 241 (#10825) as v0.5.162080434ce650.

The diagnosis is what made this fixable, and it is worth restating: 252 → 35,770 → 688,870 fitting (Bⁿ-1)/(B-1) at n=9 for B=1,2,3 turns "it hangs" into "a bounded superlinear blowup with a known exponent and a known cap". Those are different bugs. Bisecting real weierstrass.js down to weierstrassPoints and then to a package-independent synthetic reproducer is what makes it reviewable without ethers checked out.

Replacing MAX_NESTED_CLASS_DEPTH with a termination argument is worth more than the 26.2 B → 2.26 B. "Finitely many distinct (class, id) pairs" is a property of the algorithm; the cap was a bound that happened to hold, and as you note it could also have under-covered a legitimately deep acyclic chain. Trading a magic number for an invariant is the kind of change that stops a class of bug rather than an instance.

The root cause reads cleanly too — a self-referential new Self(...) is genuinely indistinguishable from class Outer { make() { return class Inner {} } } by the criterion for_each_nested_capture applies, so misreading Point as nested inside Point is not a careless bug.

#10757 stays open, deliberately. ethers now lowers and codegens — 153 modules, 0 JS fallback, about a minute — but it does not link. Closing #10757 on "compiles" would sever the thread from the original never-finishes report to what actually remains.

On that: #10802 is the same defect as #10432, which has had a six-line reproducer since 2026-09-06 —

export { createHash } from "crypto";   // dep.ts

— confirmed during an audit today with a two-file, no-ethers reproduction producing Undefined symbols: "_perry_fn_lib_ts__createHash". #10432 also covers the second form (import { x }; export { x } links but yields undefined at call time), which the ethers path cannot surface because it never gets past the link. Worth working from #10432 rather than a 153-module tree.

Your PERRY_COMPILE_TIMEOUT addition deserved more than a parenthetical, so it is called out in the train body. The parity harness timed the executed binary but never the compile, so a compiler hang wedged the whole harness instead of failing one fixture — the same "cannot distinguish still-working from stuck" shape as three other things fixed on main today.

Validation: ten cheap gates, -D warnings across all targets, five pinned artifacts byte-identical before and after, six unit suites with an empty failing set, both compiler-output suites at failed_workloads=[], repsel_census rc=0, and a seven-area 212-fixture sweep weighted to this change's blast radius (class 84, gc_ 54, import 20, property 18, closure 14, module 14, extends 8) with zero unexplained regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Compiling ethers from source never finishes: HIR lowering of @noble/curves' weierstrass.js spins indefinitely on one core

2 participants