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

Lazy create ClassReflection in enum-cases #3059

Closed
wants to merge 3 commits into from

Conversation

staabm
Copy link
Contributor

@staabm staabm commented May 10, 2024

the enum-case object will lazy create the class-reflection when used the first time.
in our slow repro case this means we don't need to create class-reflection for millions of objects over and over, since we are only check isSuperTypeOf.

refs phpstan/phpstan#10979

before this PR

time php bin/phpstan analyze --debug LibraryMasterEnum.php  
23.83s user 0.15s system 99% cpu 24.065 total

after this PR:

time php bin/phpstan analyze --debug LibraryMasterEnum.php  
17.21s user 0.16s system 98% cpu 17.610 total

not perfect yet, but still good progress

@staabm
Copy link
Contributor Author

staabm commented May 11, 2024

compared against the latest 1.11.x for the 10979 case

before

php bin/phpstan analyze --debug LibraryMasterEnum.php  1.99s user 0.13s system 98% cpu 2.145 total

php bin/phpstan analyze --debug LibraryMasterEnum.php  1.98s user 0.13s system 99% cpu 2.127 total

php bin/phpstan analyze --debug LibraryMasterEnum.php  1.96s user 0.13s system 99% cpu 2.106 total

after

php bin/phpstan analyze --debug LibraryMasterEnum.php  1.94s user 0.14s system 99% cpu 2.096 total

php bin/phpstan analyze --debug LibraryMasterEnum.php  1.93s user 0.13s system 99% cpu 2.067 total

php bin/phpstan analyze --debug LibraryMasterEnum.php  1.92s user 0.13s system 99% cpu 2.070 total

blackfire diff

grafik

@@ -79,7 +79,7 @@ public function isSuperTypeOf(Type $type): TrinaryLogic
return $type->isSubTypeOf($this);
}

$parent = new parent($this->getClassName(), $this->getSubtractedType(), $this->getClassReflection());
$parent = new parent($this->getClassName(), $this->getSubtractedType());
Copy link
Member

Choose a reason for hiding this comment

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

The problem with this is loss of precision. ObjectType often has ClassReflection given be the current scope. It makes analysis more precise if there are multiple classes with the same name - it's going to use reflection from the current file for real.

I have an idea: use classReflection if it's been set but don't fetch it from ReflectionProvider.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it seems we are missing test-coverage for the case you mentioned.

just added a new method to get but not create the class-reflection of the base-class

@ondrejmirtes
Copy link
Member

Is it still faster than before?

@staabm
Copy link
Contributor Author

staabm commented May 11, 2024

hmm .. you are right.. the perf benefit nearly went away with the lastest change.

it seems EnumCaseObjectType is slower when the class-reflection is known.

since the perf benefit is not that much, I think we can stop here.

@staabm staabm closed this May 11, 2024
@staabm staabm deleted the no-eager-class-refl branch May 11, 2024 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants