-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[PropertyInfo] Adds static cache to PhpStanExtractor
#54894
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
Conversation
Please target 7.1. We usually don't merge performance improvements into LTS branches. |
src/Symfony/Component/PropertyInfo/Extractor/PhpStanExtractor.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/PropertyInfo/Extractor/PhpStanExtractor.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/PropertyInfo/PhpStan/NameScopeFactory.php
Outdated
Show resolved
Hide resolved
978e580
to
767d355
Compare
4378f20
to
de14980
Compare
Shouldn't we now also implement the |
If so, the |
If all we store is class metadata, we don't need a reset IMHO as the leak is upper bounded, and resetting might effect performance of long running kernels for little memory savings in the end. |
if (null === $docNode) { | ||
return null; | ||
} | ||
|
||
$nameScope = $this->contexts[$class.$declaringClass] ??= $this->nameScopeFactory->create($class, $declaringClass); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$nameScope = $this->contexts[$class.$declaringClass] ??= $this->nameScopeFactory->create($class, $declaringClass); | |
$nameScope = $this->contexts[$class.'/'.$declaringClass] ??= $this->nameScopeFactory->create($class, $declaringClass); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to move this inside the loop, right before the nested foreach?:
$nameScope ??= $this->contexts[$class.'/'.$declaringClass] ??= $this->nameScopeFactory->create($class, $declaringClass);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to move this inside the loop, right before the nested foreach?:
You mean here?
Sounds reasonable to me. It may make sense to skip creating one unless really needed (and since my change is all about re-using a created one, it hardly influences performance).
src/Symfony/Component/PropertyInfo/Extractor/PhpStanExtractor.php
Outdated
Show resolved
Hide resolved
Thank you @mvhirsch. |
I was able to detect a performance penalty when using dozens of traits in even more classes. The
PhpStanExtractor
creates aNameScope
every time, but afaik this can be cached like inPhpDocExtractor
(see #32188).The performance impact is impressive, as it reduces the time needed for my
TestCase
by 30%. See Blackfire profile comparison.