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

Made ParentClass reflection resolving lazier #2584

Merged
merged 2 commits into from
Aug 25, 2023

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Aug 25, 2023

before this PR we looked up the whole parent class hierarchy, even though we work thru the results one by one and early exit on success. this means we are doing more reflection then necessary.

perf comparison

with 1.10.x@6754c2308

mstaab@NB-COMPLEX-61 MINGW64 /c/dvl/Workspace/phpstan-src-staabm (1.10.x)
$ php bin/phpstan clear-result-cache -c ../motiontm/app/partner/phpstan.neon.dist -q && time php bin/phpstan analyse -c ../motiontm/app/partner/phpstan.neon.dist -q

real    0m39.625s
user    0m0.031s
sys     0m0.046s

mstaab@NB-COMPLEX-61 MINGW64 /c/dvl/Workspace/phpstan-src-staabm (1.10.x)
$ php bin/phpstan clear-result-cache -c ../motiontm/app/partner/phpstan.neon.dist -q && time php bin/phpstan analyse -c ../motiontm/app/partner/phpstan.neon.dist -q

real    0m42.209s
user    0m0.000s
sys     0m0.046s

mstaab@NB-COMPLEX-61 MINGW64 /c/dvl/Workspace/phpstan-src-staabm (1.10.x)
$ php bin/phpstan clear-result-cache -c ../motiontm/app/partner/phpstan.neon.dist -q && time php bin/phpstan analyse -c ../motiontm/app/partner/phpstan.neon.dist -q

real    0m40.986s
user    0m0.000s
sys     0m0.031s

with this PR:

mstaab@NB-COMPLEX-61 MINGW64 /c/dvl/Workspace/phpstan-src-staabm (lazy-class-parent-reflection)
$ php bin/phpstan clear-result-cache -c ../motiontm/app/partner/phpstan.neon.dist -q && time php bin/phpstan analyse -c ../motiontm/app/partner/phpstan.neon.dist -q

real    0m39.760s
user    0m0.062s
sys     0m0.015s

mstaab@NB-COMPLEX-61 MINGW64 /c/dvl/Workspace/phpstan-src-staabm (lazy-class-parent-reflection)
$ php bin/phpstan clear-result-cache -c ../motiontm/app/partner/phpstan.neon.dist -q && time php bin/phpstan analyse -c ../motiontm/app/partner/phpstan.neon.dist -q

real    0m40.682s
user    0m0.000s
sys     0m0.015s

mstaab@NB-COMPLEX-61 MINGW64 /c/dvl/Workspace/phpstan-src-staabm (lazy-class-parent-reflection)
$ php bin/phpstan clear-result-cache -c ../motiontm/app/partner/phpstan.neon.dist -q && time php bin/phpstan analyse -c ../motiontm/app/partner/phpstan.neon.dist -q

real    0m38.821s
user    0m0.000s
sys     0m0.046s

its not a huge win, but I think the improvemnt adds up as more worker processes are started, as every worker has some overhead in doing too much reflection.


we could even consider marking getParents() as @deprecated, to make sure other 3rd party rules/extension don't do overaggressive reflection?

@staabm
Copy link
Contributor Author

staabm commented Aug 25, 2023

the actual diff is in dbee4b5

27f265c is only re-shuffling the code to improve readability

@ondrejmirtes ondrejmirtes merged commit 2d58174 into phpstan:1.10.x Aug 25, 2023
411 of 413 checks passed
@ondrejmirtes
Copy link
Member

Thank you, this makes total sense.

I wouldn't go as far as deprecating the other method, it's okay to use if if you don't early-return from a foreach.

@staabm staabm deleted the lazy-class-parent-reflection branch August 25, 2023 11:32
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