perf(size): outline the CJS factory body, not just hir.init (#10575) - #10603
proggeramlug wants to merge 1 commit into
Conversation
#8595's entry outliner only ever chunked hir.init. For a CommonJS module, cjs_wrap::wrap_commonjs_for_target wraps the whole body as text inside a `function __perry_cjs_factory() {...}` closure nested in an anonymous IIFE; hir.init ends up with only a handful of wrapper statements, so admission never fired and the real body stayed one giant function. On typescript 5.9.3's _tsc.js this was a single 463,716-instruction/6.30 MB closure, past the machine-pipeline budget. find_cjs_factory_closure(_mut) locates that closure by walking hir.init's statement/expression tree (it is a Stmt::Let naming an Expr::Closure, not a hir.functions entry, since it is lexically nested). outline_entry_module now tries hir.init first (unchanged #8595 behavior) and falls back to the factory's body with the identical chunk_statements/analyze_stmts_outlining machinery, so a module is only ever outlined from one origin per compile. The factory always captures its own name from the wrapper's IIFE scope (`__cjs_module.__perry_cjs_factory = __perry_cjs_factory;`, which perry-runtime's module_require.rs calls through on a circular-require recovery path). A chunk is a plain, non-capturing function and can't read a captured id, so classify_for_chunking keeps any statement referencing one inline in the residual body rather than promoting it to a module global -- a global would turn a per-invocation-fresh capture into one program-wide instance and could silently break that recovery path. module_globals_emit.rs folds the factory's own logical statements into the same cross-chunk-let promotion emit_module_globals already does for hir.init, so a var shared across the factory's new chunks gets the same @perry_global_* treatment hir.init cross-chunk lets get. Verified on a synthetic 2107-statement CJS fixture (cross-chunk vars plus a closure created early and invoked from far-later statements) against Node's own output, and on a full typescript 5.9.3 build: the 463,716-instruction closure is gone, entry-outline reports "cjs factory: ... candidate=true", nm shows __perry_entry_chunk_* symbols, and `--noEmit demo.ts` / `--version` output and exit codes are byte-identical to before.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughChangesCommonJS factory outlining
Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant ModuleEntry
participant EntryOutline
participant GeneratedChunks
participant ModuleGlobals
ModuleEntry->>EntryOutline: outline hir.init or __perry_cjs_factory
EntryOutline->>GeneratedChunks: generate chunks and residual statements
GeneratedChunks-->>ModuleEntry: update module entry
ModuleGlobals->>EntryOutline: read logical outlined statements
EntryOutline-->>ModuleGlobals: return factory residual statements
ModuleGlobals->>ModuleGlobals: promote cross-chunk bindings
Merge Risk: ⚪ Minimal · up to No concrete current-head issue was established, so the change is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
Picking this up for a merge train. It has no Two things surfaced while reading the diff closely enough to describe it accurately. Neither blocks the change; both are documentation rather than behaviour. 1. The self-reference is an object-literal field, not an assignment. The module doc, __cjs_module.__perry_cjs_factory = __perry_cjs_factory;but the generated text emits it as a field inside the single literal ( inside Related, and not covered by the "captures exactly one id" wording: 2. Test count. The PR body says "6 new #10575-specific tests"; the diff adds 5 ( Everything else I checked held up: the shared |
|
Landed via merge train #10710 (v0.5.1597). All source commits preserve authorship; merged main matches the validated train exactly. |
1 participant
Summary
hir.init. For a CommonJS module,cjs_wrap::wrap_commonjs_for_targetwraps the whole body as text insidefunction __perry_cjs_factory() {...}, nested in an anonymous IIFE. Ordinary lowering represents that as aStmt::Let(naming anExpr::Closure) insidehir.init's expression tree — not ahir.functionsentry, since it's lexically nested, not a top-level declaration.hir.inititself ends up with only a handful of wrapper statements (the_cjsbinding,export default, …), so admission never fired: ontypescript@5.9.3'slib/_tsc.jsthe real body stayed a single 463,716-instruction / 6.30 MB closure, past the machine-pipeline budget and emitted through LLVM's O0 fallback.find_cjs_factory_closure/find_cjs_factory_closure_mutlocate that closure by walkinghir.init's statement/expression tree.outline_entry_modulenow trieshir.initfirst (unchanged codegen: structured intra-function outlining of oversized generated functions (module-entry IIFE) #8595 behavior) and, only if that's not a candidate, falls back to the factory's body using the identicalchunk_statements/analyze_stmts_outliningmachinery — same admission thresholds, same fail-safe gates (top-level await, TDZ preallocation), same__perry_entry_chunk_*naming. A module is only ever outlined from one origin per compile.__cjs_module.__perry_cjs_factory = __perry_cjs_factory;, a load-bearing self-referenceperry-runtime'smodule_require.rscalls through (js_closure_call0) on a circular-require recovery path. A chunk is a plain, non-capturinghir.functionsentry and can't read a captured id the way the original closure could, soclassify_for_chunkingkeeps any statement referencing a captured id inline in the residual body — never relocated into a chunk — preserving the exact closure-capture codegen already emits for it. This is deliberately not solved by promoting the captured id to a module global the way a cross-chunkhir.initlet is: a global is one program-wide instance, but a closure capture is fresh per invocation: promoting it would silently change re-invocation semantics on that recovery path.module_globals_emit.rsfolds the factory's own logical statements into the same cross-chunk-let promotionemit_module_globalsalready does forhir.init, so avarshared across the factory's new chunks gets the same@perry_global_*treatment anhir.initcross-chunkletgets.Verification
cargo test -p perry-codegen --lib— 1598 passed (19 inentry_outline, including 6 new perf(size): #8595 entry outlining only splitshir.init, so a CJS module body is never outlined — tsc becomes one 6.30 MB function #10575-specific tests: factory-shape matching/rejection, factory outlining whenhir.initisn't a candidate,hir.initstill winning when both independently qualify, and the self-reference-capture-stays-inline invariant).origin/mainwith a synthetic 1200-statement CJS-factory-shaped module —outline_entry_modulereturnedSkipped("below automatic outlining threshold"), reproducing the issue exactly.cjs_wrap_builtin_require,issue_4872_barrel_default_reexports) — 7 passed, confirming small/ordinary CJS modules are unaffected.vars plus a closure created early and invoked from far-later statements, exercising the same escape-analysis path as the self-reference capture): output matches Node's own execution of the same file exactly ({"total":1500,"counter":1000,"logLength":1500,"tag":"bigcjs-fixture"}), both with outlining forced on and withPERRY_OUTLINE_ENTRY=0(the disable path reproduces the original 211,164-instruction/O0 pathology, confirming the fix is real and the kill switch still works).typescript@5.9.3build (node_modules/typescript/lib/_tsc.js, the issue's own repro):PERRY_OUTLINE_ENTRY_REPORT=1now reportscjs factory: 20 stmts → 2 chunk(s) ...; candidate=true (already outlined; ...)for_tsc.js, where main previously only reported onhir.init(candidate=false, 3-8 stmts).nmon the linked binary shows real__perry_entry_chunk_*-derived symbols (perry_fn_..._entry_chunk_..._0through_19, plus wrapper trampolines) — none existed before../tsc-perry-10575 --noEmit demo.ts→demo.ts(2,23): error TS2322: Type 'string' is not assignable to type 'number'., exit 2 — byte-identical tonode node_modules/typescript/lib/_tsc.js --noEmit demo.ts.--version→Version 5.9.3.Honest caveat on size for this specific input
_tsc.jsstill has two individual chunks around 190k/396k instructions (down from one 463,716-instruction whole-factory function) — a few of_tsc.js's top-level statements are themselves large enough (e.g. sizable literals/tables) that even isolated into their own chunk they remain sizable; that's a sub-statement-granularity problem this issue's own suggested direction (route the CJS factory through the existing chunking path) doesn't attempt to solve. The linked binary grew from 86.4 MB to 100.0 MB here — splitting one function into ~20 adds per-function prologue/GC-safepoint-scaffolding overhead that isn't fully offset by escaping the O0 fallback for every chunk, since a couple of chunks still hit it. Correctness is unaffected either way; a synthetic fixture without_tsc.js's size-outlier statements saw a real size reduction (11.0 MB → 9.2 MB) alongside eliminating the O0 fallback entirely.Test plan
cargo test -p perry-codegen --lib(1598 passed)cargo test -p perry --test cjs_wrap_builtin_require --test issue_4872_barrel_default_reexports(7 passed)cargo fmt -p perry-codegen -- --check/cargo clippy -p perry-codegen --libclean on touched filesorigin/maintypescript@5.9.3build:nmchunk symbols present,--noEmit demo.ts/--versionbyte-identical to baselineSummary by CodeRabbit