Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[PropertyInfo] add static cache to ContextFactory#32188
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[PropertyInfo] add static cache to ContextFactory#32188
Uh oh!
There was an error while loading.Please reload this page.
Conversation
bcfdbf1 to865af28Comparebendavies commentedJun 26, 2019
Caching is normally done with decoration, isn't it? |
bastnic commentedJun 26, 2019
@bendavies maybe ;). I wanted to show that there is indeed a problem and a quickfix implementation. |
teohhanhui commentedJun 26, 2019
Should have been fixed by#31452, no? |
bastnic commentedJun 26, 2019
Not that I'm aware of. The cache added in the PR seems to be at property level. But for each property/method extraction you need to parse the original file. That's what i'm trying to avoid. |
bastnic commentedJun 26, 2019
Just seen that it was in 4.3-rc1, so nope not fixed! |
dunglas left a comment
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.
LGTM! I don’t think that a decorator is needed for a local cache like this one. There is no reasons to not use such metadata cache!
bastnic commentedJun 27, 2019
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
da89214 to8aabb78CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
ContextFactory::createFromReflector is heavy, and it's called redundanltyby Symfony\Component\PropertyInfo\Extractor\PhpDocExtractor for each propertyand methods. Avoid that by parsing it only once and then use static cache
8aabb78 to063e880CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
fabpot commentedJul 3, 2019
Thank you@bastnic. |
This PR was merged into the 4.4 branch.Discussion----------[PropertyInfo] add static cache to ContextFactory| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | no (performance...)| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#32073,api-platform/api-platform#1159| License | MIT| Doc PR | noThe issue is very very well described here#32073, and was also discussed a few weeks ago with@dunglas hereapi-platform/api-platform#1159.`ContextFactory::createFromReflector` is heavy, and it's called redundanltyby `Symfony\Component\PropertyInfo\Extractor\PhpDocExtractor` for each propertyand methods. Avoid that by parsing it only once and then use static cacheThis is a quite big performance problem.Commits-------063e880 [PropertyInfo] add static cache to ContextFactory
bendavies commentedJul 3, 2019
ah shame this didn't go into 4.3. thanks anyway! |
… (mvhirsch)This PR was squashed before being merged into the 7.2 branch.Discussion----------[PropertyInfo] Adds static cache to `PhpStanExtractor`| Q | A| ------------- | ---| Branch? | 7.2| Bug fix? | no (performance)| New feature? | no <!-- please update src/**/CHANGELOG.md files -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Issues | <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead -->| License | MITI was able to detect a performance penalty when using dozens of traits in even more classes. The `PhpStanExtractor ` creates a `NameScope` every time, but afaik this can be cached like in `PhpDocExtractor` (see#32188).The performance impact is impressive, as it reduces the time needed for my `TestCase` by 30%. See [Blackfire profile comparison](https://blackfire.io/profiles/compare/9eb78784-fa68-4721-9c87-f2be52da2d63/graph).<!--Replace this notice by a description of your feature/bugfix.This will help reviewers and should be a good start for the documentation.Additionally (seehttps://symfony.com/releases): - Always add tests and ensure they pass. - Bug fixes must be submitted against the lowest maintained branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against the latest branch. - For new features, provide some code snippets to help understand usage. - Changelog entry should followhttps://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry - Never break backward compatibility (seehttps://symfony.com/bc).-->Commits-------9697351 [PropertyInfo] Adds static cache to `PhpStanExtractor`
Uh oh!
There was an error while loading.Please reload this page.
The issue is very very well described here#32073, and was also discussed a few weeks ago with@dunglas hereapi-platform/api-platform#1159.
ContextFactory::createFromReflectoris heavy, and it's called redundanltyby
Symfony\Component\PropertyInfo\Extractor\PhpDocExtractorfor each propertyand methods. Avoid that by parsing it only once and then use static cache
This is a quite big performance problem.