Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Draft: Intersect optimization for large unions #1471

Merged
merged 3 commits into from Jun 24, 2022
Merged

Draft: Intersect optimization for large unions #1471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a3ebe9
Optimize inner loop of intersect union search
neclimdul Jun 23, 2022
517bc8f
Optimize type specification intersect logic
neclimdul Jun 23, 2022
9abc52c
Adjust test results
ondrejmirtes Jun 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/Analyser/TypeSpecifier.php
View file
Open in desktop
Expand Up @@ -342,9 +342,8 @@ public function specifyTypesInCondition(
}

if ($context->true()) {
$type = TypeCombinator::intersect($scope->getType($expr->right), $scope->getType($expr->left));
$leftTypes = $this->create($expr->left, $type, $context, false, $scope, $rootExpr);
$rightTypes = $this->create($expr->right, $type, $context, false, $scope, $rootExpr);
$leftTypes = $this->create($expr->left, $exprRightType, $context, false, $scope, $rootExpr);
$rightTypes = $this->create($expr->right, $exprLeftType, $context, false, $scope, $rootExpr);
return $leftTypes->unionWith($rightTypes);
} elseif ($context->false()) {
return $this->create($expr->left, $exprLeftType, $context, false, $scope, $rootExpr)->normalize($scope)
Expand Down
6 changes: 4 additions & 2 deletions src/Type/TypeCombinator.php
View file
Open in desktop
Expand Up @@ -638,11 +638,13 @@ public static function intersect(Type ...$types): Type
$topLevelUnionSubTypes = [];
$innerTypes = $type->getTypes();
usort($innerTypes, $sortTypes);
$slice1 = array_slice($types, 0, $i);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

Just out of curiosity, why creating intermediary variables here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherweise these arrays get sliced within the loop over and over again

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you're right, for some reason I haven't seen the foreach loop ! Well done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, please send a microoptimization PR, then we'll discuss the rest.

$slice2 = array_slice($types, $i + 1);
foreach ($innerTypes as $innerUnionSubType) {
$topLevelUnionSubTypes[] = self::intersect(
$innerUnionSubType,
...array_slice($types, 0, $i),
...array_slice($types, $i + 1),
...$slice1,
...$slice2,
);
}

Expand Down
4 changes: 2 additions & 2 deletions tests/PHPStan/Analyser/TypeSpecifierTest.php
View file
Open in desktop
Expand Up @@ -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'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really annoying to not have anymore :/

I might spend some time on this in the future to bring back.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, it looked pretty esoteric to me as a way to narrow down types :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can still do it only for objects, or something like that...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it annoys me enough I will find a way to bring it back without a huge performance loss :)

[],
],
[
Expand Down Expand Up @@ -1067,7 +1067,7 @@ public function dataCondition(): array
),
[
'$foo' => 'array<string, mixed>',
'array_filter($foo, \'is_string\', ARRAY_FILTER_USE_KEY)' => 'array<string, mixed>',
'array_filter($foo, \'is_string\', ARRAY_FILTER_USE_KEY)' => 'array', // could be 'array<string, mixed>'
],
[],
],
Expand Down