fix: trampoline getSideEffectsConnectionState to avoid stack overflow on deep import chains#20993
Conversation
…ow on deep import chains The recursive descent through `HarmonyImportSideEffectDependency.getModuleEvaluationSideEffectsState` adds two stack frames per module and overflows V8's stack on long chains of side-effect-free imports — surfacing as `RangeError: Maximum call stack size exceeded` at the HarmonyImportSideEffectDependency frame (issue #20986). The recursive code itself wasn't introduced in 5.107.0, but #20723's expanded `isPure` analysis (treating ArrayExpression, ObjectExpression, NewExpression, etc. as pure) flipped many more modules to `buildMeta.sideEffectFree`, putting them inside the recursive walk for the first time and exposing the latent overflow. Move the algorithm into a file-scope generator `walkSideEffects` that `yield`s a child generator for each side-effect-free HarmonyImportSideEffectDependency instead of recursing into `getSideEffectsConnectionState`. The method itself is now a small trampoline driving the generator stack. The generator body is the same shape as the previous recursive code, so semantics and tree-shaking results are unchanged. Tests: - Unit tests in `test/NormalModule.unittest.js` cover a 20000-deep chain, a cycle, and bailout propagation. - Integration case `test/configCases/side-effects/deep-import-chain` mirrors the structure of the reporter's reproduction repo (https://github.com/abecirovic-mo/webpack-5.107.0-repro) with a 500-module chain that fits within the per-case compile budget.
🦋 Changeset detectedLatest commit: 18d35bc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This PR is packaged and the instant preview is available (c8d97e2). Install it locally:
npm i -D webpack@https://pkg.pr.new/webpack@c8d97e2
yarn add -D webpack@https://pkg.pr.new/webpack@c8d97e2
pnpm add -D webpack@https://pkg.pr.new/webpack@c8d97e2 |
There was a problem hiding this comment.
Pull request overview
This PR addresses a stack overflow in side-effects analysis on very deep chains of side-effect-free imports by replacing recursive descent in NormalModule.getSideEffectsConnectionState with an iterative trampoline driving a generator-based walk (fixing issue #20986) while aiming to preserve existing tree-shaking semantics.
Changes:
- Refactor side-effects connection state evaluation to an iterative trampoline + generator walk to avoid V8 call stack overflows.
- Add unit tests covering a 20,000-deep chain, cycle handling, and bailout propagation.
- Add an integration config case that generates a ~500-module import chain and verifies compilation doesn’t overflow the stack; include a changeset entry.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/NormalModule.js | Replaces recursive side-effects traversal with a generator + trampoline approach; preserves bailout behavior. |
| test/NormalModule.unittest.js | Adds focused unit coverage for deep-chain traversal, cycles, and bailout propagation. |
| test/configCases/side-effects/deep-import-chain/webpack.config.js | Generates a long, side-effect-free import chain fixture for integration coverage. |
| test/configCases/side-effects/deep-import-chain/index.js | Integration assertion for compiling the deep import chain without stack overflow. |
| test/configCases/side-effects/deep-import-chain/.gitignore | Ignores generated src/ fixture files. |
| .changeset/deep-side-effect-chain-stack-overflow.md | Documents the patch-level fix for the stack overflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your changes status has failed because you have indirect coverage changes. Learn more about Unexpected Coverage Changes and reasons for indirect coverage changes. Additional details and impacted files@@ Coverage Diff @@
## main #20993 +/- ##
==========================================
- Coverage 90.94% 90.91% -0.04%
==========================================
Files 573 573
Lines 58938 58967 +29
Branches 15889 15894 +5
==========================================
+ Hits 53603 53607 +4
- Misses 5335 5360 +25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Types CoverageCoverage after merging claude/issue-20986-CIh26 into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
…state The previous iterative-only walker fixed the regression but still ran ~2x slower than the pre-#20993 recursive code on short-to-medium chains because every descent went through explicit array-stack operations instead of native call frames. Restructure the walk as: walkSideEffectsRecursive(mod, depth) if depth >= SIDE_EFFECTS_RECURSION_LIMIT (2000): -> walkSideEffectsIterative(mod) # stack-safe tail else: direct recursion, 1 native frame per module The recursive form also folds the descent through HarmonyImportSideEffectDependency.getModuleEvaluationSideEffectsState directly into the loop, so each module costs one V8 stack frame instead of two. That is why the limit can be set as high as 2000 without risking the #20986 overflow (which triggered at ~1300 modules under the old two-frame pattern). Microbench (best of 7 runs, side-effect-free linear chain): chain BEFORE AFTER NOW 10 34.0 ms 190.8 ms 40.1 ms 100 48.5 ms 191.3 ms 59.0 ms 1000 60.3 ms 231.8 ms 69.8 ms 20000 CRASH 111.5 ms 66.3 ms Within 16-22% of the pre-regression recursive code on short / medium chains, ~3x faster than the merged generator trampoline, and still stack-safe at 20000 modules. A new unit test exercises the recursive-to-iterative crossover with a bailout planted deep in the iterative tail.
Builds on the previous hybrid walker by adding a per-module memoization
slot keyed by ModuleGraph. SideEffectsFlagPlugin queries
getSideEffectsConnectionState for every module while iterating
incoming connections and again inside its exportInfo.getTarget /
moveTarget callbacks, and the walker also re-encounters the same
sub-graphs as it descends through different starting modules. Without
the cache that work is O(N x avgDepth); with it the second and later
visits are O(1).
Design notes:
- Cache slot is a (ModuleGraph, ConnectionState) pair stored directly
on the module (`_sideEffectsStateGraph` / `_sideEffectsStateValue`)
rather than a WeakMap so the lookup is a single reference compare.
When the next compilation queries with a different ModuleGraph the
slot is overwritten on the first walk — no stale data leaks.
- Cycle safety: a result computed in the presence of a cycle can
differ from the same module's fresh result (the back-edge target's
contribution is hidden by CIRCULAR_CONNECTION short-circuiting), so
we only memoize results whose walk subtree never observed a
CIRCULAR_CONNECTION. A module-level `_sideEffectsCircularSeen` flag
is save/restored at each recursive frame and at the public entry
point so re-entrant calls (notably ConcatenatedModule.getSECS
delegating to its rootModule) don't clobber the outer walk's
tracking. A `true` result is monotonic — once any dep declares
side effects, the answer is `true` regardless of cycle resolution —
so it's always safe to memoize.
- Descents into non-NormalModule refModules conservatively mark the
current frame as cycle-tainted, since we can't observe whether the
inner public-method call hit a cycle back through the outer walk's
stack.
Microbench (best of 7 runs):
Repeated queries of every module in one chain — the realistic
SideEffectsFlagPlugin access pattern:
chain BEFORE AFTER (PR #20993) NOW (cached)
10 0.03 ms n/a 0.08 ms
100 0.15 ms n/a 0.06 ms 2.5x
1000 15.69 ms n/a 0.36 ms 44x
5000 712.70 ms n/a 2.66 ms 268x
First-walk-only (cold cache, fresh chain each call):
chain BEFORE NOW (cached)
10 333 ns 300 ns (matches BEFORE)
100 110 ns 131 ns
1000 100 ns 148 ns
5000 157 ns 526 ns (iterative tail engaged)
All existing side-effect / tree-shaking / inner-graph / concatenate
ConfigTestCases pass (658 tests).
The previous linear-chain optimization only fired when a module had exactly one entry in `dependencies` — but a real ESM module compiled by webpack has the `HarmonyImportSideEffectDependency` *plus* an `HarmonyExportSpecifierDependency` per `export` and a `ConstDependency` per replaced expression. With the strict `length === 1` check the 1300-module cyclic chain (the canonical #20986 repro) fell through to the general for-loop walk and overflowed V8's stack at ~1300 frames again on Node 22+. Loosen the linear-chain detection to "exactly one `SideEffectDep` among any number of deps whose `getModuleEvaluationSideEffectsState` returns `false`/`CIRCULAR_CONNECTION`". The walker now processes the module's non-recursive deps in their declared order — bailing out correctly with the first dep whose state is `true`, aggregating non-`false` states into `nonRecursiveCurrent` — and only attempts the linear tail-call when: - at most one `SideEffectDep` was seen, - no non-recursive dep triggered a bailout, - `nonRecursiveCurrent` stayed at `false` (otherwise the "ancestor's current = chain state" propagation rule no longer holds and we fall back to the general walk). Verified on the actual #20986 repro (1300-module cyclic chain): AFTER (#20993 generator trampoline) ~12.3 s NOW (this branch) ~6.5 s (~1.9x faster) Larger chains (N >= 3000) hit pre-existing stack overflows in `ModuleConcatenationPlugin._tryToAdd` and `SideEffectsFlagPlugin.optimizeIncomingConnections` that are out of scope here — both branches behave identically there. Adds a regression test that builds a 5000-module cyclic chain whose modules have the same `[SideEffectDep, ConstDependency, ConstDependency]` dependency shape as the real repro.
The recursive descent through
HarmonyImportSideEffectDependency.getModuleEvaluationSideEffectsStateadds two stack frames per module and overflows V8's stack on long
chains of side-effect-free imports — surfacing as
RangeError: Maximum call stack size exceededat theHarmonyImportSideEffectDependency frame (issue #20986).
fixes #20986