Drop repeated UnionType operands in TypeCombinator::intersect() before distributing them - #6195
Drop repeated UnionType operands in TypeCombinator::intersect() before distributing them#6195phpstan-bot wants to merge 3 commits into
UnionType operands in TypeCombinator::intersect() before distributing them#6195Conversation
3b94b04 to
696a0d2
Compare
|
ran the benchmarks locally and it looks really good. no side effects on the other benchmarks, and the newly added one is now wicked fast: |
|
//cc @SanderMuller please review |
…efore distributing them * `TypeCombinator::doIntersect()` now calls a new `removeDuplicateUnions()` before the `A & (B | C)` distribution, so n copies of the same union no longer get multiplied out into 2^n recursive `intersect()` calls. Restricted to the exact `UnionType` class, like the finite fast path right above it: `equals()` ignores a `TemplateUnionType`'s variance strategy, and `BenevolentUnionType` keeps its dedicated handling. * The blowup was reached through `TypeCombinator::doIntersect()`'s accessory-base-type branch: `HasOffsetType` and `HasOffsetValueType` both return `array|ArrayAccess` from `getDefaultBaseType()`, so intersecting n of them intersected n identical two-member unions. `isset()` (and `!empty()`, which narrows through `!isset()`) with 26 array offsets went from "does not finish" to 1.6 s. * Probed the sibling accessory types: every other `getDefaultBaseType()` returns a non-union (`string`, `array`, `ObjectWithoutClassType`), so `HasOffsetValueType` is `HasOffsetType`'s only twin here - it is covered by the same fix and by its own test. `array_key_exists()` chains, `isset()` on property fetches (`HasPropertyType`) and `??` chains were measured and were never affected. * Added `TypeCombinatorTest::testIntersectManyAccessoryTypesSharingAUnionBaseType` (both accessory types, asserting the unchanged result plus a wall-clock budget: 64 s before, 4 ms after), `testIntersectRepeatedUnions`, and `tests/bench/data/bug-15061.php` with the reported reproducer.
696a0d2 to
d1aee39
Compare
| return 0; | ||
| }; | ||
| if ($unionTypesCount >= 2) { | ||
| $types = self::removeDuplicateUnions($types); |
There was a problem hiding this comment.
instead of cleaning up after the fact, should we instead prevent building a duplicate union at the call-site in
phpstan-src/src/Analyser/ExprHandler/BooleanAndHandler.php
Lines 216 to 221 in c0c39ea
There was a problem hiding this comment.
Pushed. Here's what I found and changed.
On the suggestion
BooleanAndHandler isn't where the duplicate unions come from. Each arm of the flattened chain narrows a different offset, so the SpecifiedTypes it merges hold hasOffset('a'), hasOffset('b'), … — all distinct, nothing to prevent building there.
The duplicates are built inside TypeCombinator::doIntersect() itself, at the accessory base-type site (src/Type/TypeCombinator.php:2131):
$accessoryBaseTypes[] = $type->getDefaultBaseType();
...
return self::intersect(self::intersect(...$accessoryBaseTypes), ...$types);HasOffsetType::getDefaultBaseType() and HasOffsetValueType::getDefaultBaseType() both return array|ArrayAccess, so n offset accessories build a list of n identical unions, which the A & (B | C) distribution then multiplies out one at a time.
What I did
That is a genuine call site building duplicate operands, so I moved the fix there — skip a base type an earlier accessory already contributed — and reverted the removeDuplicateUnions() cleanup in doIntersect() entirely. Soundness is the same argument (intersection is idempotent), but nothing else in doIntersect() is touched, and the get_class(UnionType::class) / TemplateUnionType caveats disappear: the only base types in play are string, array, ObjectWithoutClassType and array|ArrayAccess.
It's also faster. Reproducer at -l 8, 26 offsets:
| wall clock | |
|---|---|
| before any fix | > 3 min (timed out at 180 s) |
removeDuplicateUnions() in doIntersect() |
4.3 s |
| dedupe at the base-type site | 2.1 s |
Commits
e896fe974— removes the timing test fromTypeCombinatorTest(your earlier comment; that change had been made locally but never pushed, so it's in this batch).2cd33ec69— the fix above.tests/bench/data/bug-15061.phpstays as the regression pin.
make tests OK (21235 tests, 96758 assertions, 97 skipped), make phpstan no errors, make cs clean.
…s it tests/bench/data/bug-15061.php measures the same blowup end to end, so the wall-clock budget in the unit test only adds a flaky duplicate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Deduplicate where the duplicate operands are built instead of dropping repeated unions after the fact in doIntersect(): every accessory type contributes its getDefaultBaseType(), and hasOffset()/hasOffsetValue() both return `array|ArrayAccess`, so n of them handed intersect() n identical two-member unions to distribute over each other - 2^n recursive calls. Skipping a base type that an earlier accessory already contributed is sound because intersection is idempotent, and it keeps the rest of doIntersect() untouched. The reproducer from the issue goes from over three minutes (timeout) to 2.1 s, against 4.3 s for the previous placement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
staabm
left a comment
staabm
left a comment
There was a problem hiding this comment.
update PR title and description to reflect the latest state of the PR
|
Both checks are green and the tree is clean at New titleNew description
What changed versus the description currently on the PR: the fix now lives at the accessory base-type collection site instead of a |
2 participants
Summary
isset()with many array offsets made analysis time double with every extra offset — thereported snippet (26 optional keys of a
@phpstan-typearray shape checked in oneisset()) never finished, while 13 offsets already took a minute.Each
isset()subject narrows the array with aHasOffsetTypeaccessory type, andTypeCombinator::intersect()supplies those accessory types with their default base type,array|ArrayAccess. Intersecting n of them therefore intersected n identical two-memberunions, and the
A & (B | C)distribution multiplies operands out one union at a time —2^n recursive
intersect()calls. This change drops repeated union operands before thedistribution, which is sound because intersection is idempotent.
Changes
src/Type/TypeCombinator.phpremoveDuplicateUnions(): drops every plainUnionTypeoperand thatrepeats an earlier one (
equals()).doIntersect()calls it right where it already detects that at least two operands areunions — i.e. only in the case that can blow up — and returns early if that leaves a
single operand.
UnionTypeclass, mirroring the restrictionon the finite-set fast path a few lines above.
TemplateUnionType::equals()ignoresthe variance strategy, so two template unions that compare equal are still not
interchangeable (
AnalyserIntegrationTest::testPr5880pins that), andBenevolentUnionTypekeeps its dedicated handling.tests/PHPStan/Type/TypeCombinatorTest.php— regression tests (see below).tests/bench/data/bug-15061.php— the reported reproducer, added to the benchmark suitethe way other performance regressions in this repo are pinned.
Root cause
The pattern is repeated operands in an intersection that distributes unions. Every
AccessoryTypemust be given a base type, whichdoIntersect()does with$accessoryBaseTypesholds onegetDefaultBaseType()per accessory type. Two accessorytypes return a union from that method —
HasOffsetTypeandHasOffsetValueType, botharray|ArrayAccess— so a list of n of them produced n equal unions, and the innerintersect()distributed them over each other:2^ncalls before the duplicates werefinally recognized at the leaves by the pairwise
isSuperTypeOf()pass. The fix removesthem up front, so the same call is linear.
Analogous cases probed:
HasOffsetValueType—HasOffsetType's twin, samearray|ArrayAccessbase type,same blowup at the unit level. Fixed by the same change; covered by its own data set in
the new test.
NonEmptyArrayType,AccessoryArrayListType,OversizedArrayType,HasPropertyType,HasMethodType, and the six accessory stringtypes) returns a non-union base type (
array,string,ObjectWithoutClassType), sotheir base-type intersection is polynomial and was never affected.
after:
isset($a['x']) && isset($a['y']) && …and nestedisset($a['x']['v'], …)wereaffected (59 s → 1.4 s),
!empty($a['x']) && …was affected becauseempty()narrowsthrough
!isset()(10 s → 4 s at 26 subjects), whilearray_key_exists()chains,isset($obj->x, $obj->y, …)(HasPropertyType) and($a['x'] ?? '') !== ''chains werealready linear and are unchanged.
The change is purely about how the answer is computed: the resulting types are identical
before and after, verified by asserting the exact
describe()output in the new testswith the fix reverted.
Test
TypeCombinatorTest::testIntersectManyAccessoryTypesSharingAUnionBaseType— intersects22
HasOffsetTypes (and, as a second data set, 22HasOffsetValueTypes), asserts theexact resulting type description and a wall-clock budget. Without the fix each data set
takes ~64 s and fails the budget; with it, ~4 ms.
TypeCombinatorTest::testIntersectRepeatedUnions—int|stringintersected with itselfthree times is still
int|string.tests/bench/data/bug-15061.php— the reproducer from the issue, so the benchmark suitecatches a reintroduction end to end.
make testsandmake phpstanare green.Fixes phpstan/phpstan#15061