Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Conversation

@bastnic
Copy link
Contributor

@bastnicbastnic commentedJun 26, 2019
edited
Loading

QA
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
LicenseMIT
Doc PRno

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::createFromReflector is heavy, and it's called redundanlty
bySymfony\Component\PropertyInfo\Extractor\PhpDocExtractor for each property
and methods. Avoid that by parsing it only once and then use static cache

This is a quite big performance problem.

Deepin Capture-écran_zone de sélection _20190626142041

@bastnicbastnicforce-pushed thefeature/performance-PropertyInfo-PhpDocExtractor branch 3 times, most recently frombcfdbf1 to865af28CompareJune 26, 2019 12:34
@bendavies
Copy link
Contributor

Caching is normally done with decoration, isn't it?

@bastnic
Copy link
ContributorAuthor

@bendavies maybe ;). I wanted to show that there is indeed a problem and a quickfix implementation.

@teohhanhui
Copy link
Contributor

Should have been fixed by#31452, no?

@bastnic
Copy link
ContributorAuthor

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.

teohhanhui reacted with thumbs up emoji

@bastnic
Copy link
ContributorAuthor

Just seen that it was in 4.3-rc1, so nope not fixed!

Copy link
Member

@dunglasdunglas left a 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
Copy link
ContributorAuthor

Great, thanks@lyrixx and@dunglas for the approvals.

The PR is based on 4.4 because I read somewhere that performance stuff should be a minor, not a patch version. Should I put it on 4.3?

@bastnicbastnicforce-pushed thefeature/performance-PropertyInfo-PhpDocExtractor branch 2 times, most recently fromda89214 to8aabb78CompareJune 27, 2019 11:58
@nicolas-grekasnicolas-grekas added this to thenext milestoneJun 28, 2019
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
@bastnicbastnicforce-pushed thefeature/performance-PropertyInfo-PhpDocExtractor branch from8aabb78 to063e880CompareJune 28, 2019 12:35
@fabpot
Copy link
Member

Thank you@bastnic.

@fabpotfabpot merged commit063e880 intosymfony:4.4Jul 3, 2019
fabpot added a commit that referenced this pull requestJul 3, 2019
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.![Deepin Capture-écran_zone de sélection _20190626142041](https://user-images.githubusercontent.com/84887/60179692-8471c480-981e-11e9-9e3c-3f9c0b83b01b.png)Commits-------063e880 [PropertyInfo] add static cache to ContextFactory
@bendavies
Copy link
Contributor

ah shame this didn't go into 4.3. thanks anyway!

bastnic reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas modified the milestones:next,4.4Oct 27, 2019
@bastnicbastnic deleted the feature/performance-PropertyInfo-PhpDocExtractor branchJanuary 8, 2020 16:32
fabpot added a commit that referenced this pull requestMay 31, 2024
… (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`
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@dunglasdunglasdunglas approved these changes

@xabbuhxabbuhxabbuh left review comments

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@lyrixxlyrixxlyrixx approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

9 participants

@bastnic@bendavies@teohhanhui@fabpot@dunglas@nicolas-grekas@lyrixx@xabbuh@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp