Prevent nested foreach unrolling to fix exponential blowup#5732
Prevent nested foreach unrolling to fix exponential blowup#5732phpstan-bot wants to merge 2 commits into
Conversation
- Add `insideUnrolledForeach` flag to StatementContext to track whether we are already inside an unrolled foreach body - Skip unrolling when already inside an unrolled foreach, avoiding multiplicative 3^k work for k nesting levels - Remove the FOREACH_UNROLL_NESTED_LIMIT constant and foreachUnrollFactor field, which were an insufficient fix - Propagate the flag through the final convergence pass so nested foreaches inside unrolled bodies also skip unrolling - Update test expectations for bug-7978, bug-9332, bug-14489 to reflect slightly less precise (but still correct) types when nested foreach unrolling is skipped
| private const LOOP_SCOPE_ITERATIONS = 3; | ||
| private const GENERALIZE_AFTER_ITERATION = 1; | ||
| private const FOREACH_UNROLL_LIMIT = 16; | ||
| private const FOREACH_UNROLL_NESTED_LIMIT = 16; |
There was a problem hiding this comment.
instead of removing this feature as a whole, would it be enough to reduce the threshold to a lower value, so we can still have the more precise results in some cases.
There was a problem hiding this comment.
Done. Here's a summary of the changes:
Instead of completely disabling nested foreach unrolling, restored the factor-based approach with a lower limit:
FOREACH_UNROLL_NESTED_LIMIT = 8(reduced from original 16) — only checked when already inside an unrolled foreach (factor > 1), so top-level foreaches are unaffected- The factor is now properly propagated through the final convergence pass via
enterUnrolledForeach(totalKeys), fixing a bug in the original code where the final pass reset the factor to 1, allowing exponential re-unrolling - This allows 2-3 levels of nesting with small constant arrays while preventing the exponential blowup for deeper nesting
- All three test cases (bug-7978, bug-9332, bug-14489) regain their precise type expectations
- Benchmark (
bug-14674.phpwith 13 nested levels) still completes in ~3.4s
There was a problem hiding this comment.
add a test which demonstrates the type degradation happens at the configured threshold
Instead of completely disabling nested foreach unrolling, restore the factor-based approach with a reduced limit (8, down from the original 16). This preserves type precision for small nested foreaches (2 levels of 2-key arrays) while still preventing exponential blowup for deeply nested cases. Key changes: - FOREACH_UNROLL_NESTED_LIMIT = 8 (only checked when factor > 1, so top-level foreaches are unaffected by the nested limit) - Factor is properly propagated through the final convergence pass via enterUnrolledForeach(totalKeys), fixing a bug where the original code reset the factor in the final pass - Restored precise type expectations in bug-7978, bug-9332, bug-14489 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
I think this looks good. I am still not sure how to read the phpbench summaries. the repro from phpstan/phpstan#14674 completes in 1,7-2,0 seconds after this PR on my machine - while taking 90 seconds on 2.1.x |
2 participants
Summary
Fixes an exponential performance regression introduced in 2.1.54 where nested
foreachloops over constant arrays caused analysis time to blow up from ~1s to ~57s (13 nesting levels). The fix prevents recursive foreach unrolling by tracking whether we're already inside an unrolled foreach body.Root cause
In 2.1.52,
tryProcessUnrolledConstantArrayForeachwas introduced to unroll constant-array foreaches for precise per-iteration types. Each unrolled iteration runs through a 3-pass convergence loop (3 NoopNodeCallback passes + 1 final pass). When foreaches nest, this creates 3^k work for k levels of nesting.In 2.1.54 (commit
fe946e98f), the unrolled body context was changed from$context->enterDeep()to$contextto fix bug-14543, which preservedisTopLevel=trueand re-enabled the convergence loop inside unrolled bodies. This made nested unrolling dramatically worse.In 2.1.55 (commit
f46f0114d), aFOREACH_UNROLL_NESTED_LIMIT=16factor cap was added, but this was insufficient — it still allowed 2 levels of nesting (2×2=4 ≤ 16 for small arrays, but 3^2=9 convergence passes), and for arrays with ≤16 total keys across all levels, the exponential blowup remained uncapped.The fix is simple: don't unroll nested foreaches at all. The outermost foreach gets full unrolling precision; inner foreaches fall back to normal (union-based) processing. This reduces work from exponential to linear while losing only minor type precision in nested cases.
Fixes phpstan/phpstan#14674