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.

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

Faster ConstantArrayType->isSuperTypeOf() #2086

Merged
merged 1 commit into from
Dec 11, 2022

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Dec 11, 2022

23% speedup of phpstan/phpstan#8504

This file mmarton/phpstan-issue8146@master/src/DataFixtures/LocationFixtures.php from phpstan/phpstan#8146 really slows PHPStan down, because it contains a lot of literal arrays.


grafik


$isValueSuperType = $this->valueTypes[$i]->isSuperTypeOf($type->getOffsetValueType($keyType));
if ($isValueSuperType->no()) {
return TrinaryLogic::createNo();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return TrinaryLogic::createNo();
return $isValueSuperType;

should be possible, no? or any other reason why you didn't do that?

Copy link
Contributor Author

@staabm staabm Dec 11, 2022

Choose a reason for hiding this comment

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

createNo is cheap and I like seeing on the first sight without applying any logics what the return type is.

but its the same thing, you are right.

@ondrejmirtes ondrejmirtes merged commit e4a5e98 into phpstan:1.9.x Dec 11, 2022
@ondrejmirtes
Copy link
Member

Thank you!

@staabm staabm deleted the fast-const-array-supertype branch December 11, 2022 21:06
@staabm
Copy link
Contributor Author

staabm commented Dec 11, 2022

@ondrejmirtes we could apply a similar early return NO on NO here:

$results[] = $templateType->isValidVariance($this->types[$i], $ancestor->types[$i]);

I have no concrete perf relevant case to measure it. wdyt?

@ondrejmirtes
Copy link
Member

Only if it shows up in Blackfire :) This is gonna run the same number of times as there are @template above a class... so not that many :)

@staabm
Copy link
Contributor Author

staabm commented Dec 11, 2022

ok.

profiling https://github.com/mmarton/phpstan-issue8146/blob/master/src/DataFixtures/LocationFixtures.php I can see the top consumer is now PhpFileCleaner

grafik

this makes me think of #1290 which deletes said PhpFileCleaner class

@ondrejmirtes
Copy link
Member

Nope, that's completely unrelated to the arrays, it just means there's a lot of dependencies in vendor and otherwise the project (analysed files) is relatively small.

#1290 is going to be slower than the current solution with PhpFileCleaner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants