Draft: Intersect optimization for large unions #1471
Conversation
| @@ -638,11 +638,13 @@ public static function intersect(Type ...$types): Type | |||
| $topLevelUnionSubTypes = []; | |||
| $innerTypes = $type->getTypes(); | |||
| usort($innerTypes, $sortTypes); | |||
| $slice1 = array_slice($types, 0, $i); | |||
There was a problem hiding this comment.
Hi,
Just out of curiosity, why creating intermediary variables here?
There was a problem hiding this comment.
otherweise these arrays get sliced within the loop over and over again
There was a problem hiding this comment.
Oh you're right, for some reason I haven't seen the foreach loop ! Well done.
There was a problem hiding this comment.
@neclimdul I feel this array_slice change is a no-brainer and should be submitted as a seperate PR.
the other change breaks the tests atm, so needs further inspection
There was a problem hiding this comment.
I agree, please send a microoptimization PR, then we'll discuss the rest.
Array slice spends a good bit of time allocating memory for new arrays even for small arrays. For larger computed unions this can be a non-trivial amount of time. We can minimize the cost by not repeating the slice for ever iteration of the inner loop since it doesn't depend on the loop.
Don't know if this will have side effects so testing needed but if it works... oh lord. Fixes #7421 Slow processing of Class::* type hints
ondrejmirtes
force-pushed
the
type-specification-optimization
branch
from
eca9bf2
to
9abc52c
Compare
|
Ah whatever, I really like the optimization, the result can be slightly worse in this case :) |
|
I pushed test adjustments, will merge it if it comes back green. |
|
Thank you! |
| @@ -422,7 +422,7 @@ public function dataCondition(): array | |||
| new Variable('foo'), | |||
| new Variable('bar'), | |||
| ), | |||
| ['$foo' => 'Bar', '$bar' => 'Bar'], | |||
| ['$foo' => 'Bar', '$bar' => 'mixed'], // could be '$bar' => 'Bar' | |||
There was a problem hiding this comment.
This is really annoying to not have anymore :/
I might spend some time on this in the future to bring back.
There was a problem hiding this comment.
I'm sorry, it looked pretty esoteric to me as a way to narrow down types :)
There was a problem hiding this comment.
Maybe we can still do it only for objects, or something like that...
There was a problem hiding this comment.
If it annoys me enough I will find a way to bring it back without a huge performance loss :)
|
I'm kind of travelling right now and could not check - do we know if this improves other big union operations too? E.g. some constant array perf problems also lead to many unions. Or is this a too different case? |
|
@herndlm likely very specific. |
|
@neclimdul this PR has introduced phpstan/phpstan#7550 issue, will you fix it? |
No description provided.