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

[FrameworkBundle] Add cache configuration for PropertyInfo#31452

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

Merged

Conversation

@alanpoulain
Copy link
Contributor

@alanpoulainalanpoulain commentedMay 9, 2019
edited
Loading

QA
Branch?4.3
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?
Tests pass?yes
Fixed ticketsn/a
LicenseMIT
Doc PRn/a

The PropertyInfoExtractor was not cached by default but the class to do it was surprisingly here, unused.

I've added the configuration to enable it by default when it's not the development environment.

I haven't added the warmup part because there are too much arguments for the methods (class, property, context).

It will be a big boost for the performance! For this code:

$book =$this->serializer->deserialize('{"id":3,"reviews":[{"id": 7, "body": "This book is fantastic!", "rating": 9, "letter": "A", "publicationDate": "2019"}],"isbn":"978-0-5533-9243-2","title":"Fool\'s Assassin","description":"A famous saga","author":"Robin Hobb","publicationDate":"2014"}', Book::class,'json');$this->serializer->serialize($book,'json');

We obtain this:

image

The Blackfire comparison is here:https://blackfire.io/profiles/compare/2c746d26-320a-4aab-80ef-7276c2e92b96/graph

teohhanhui, lyrixx, maxhelias, loicbourg, damienalexandre, and er1z reacted with heart emojiKoc, dunglas, teohhanhui, lyrixx, Korbeil, welcoMattic, RSalo, andrew-demb, and er1z reacted with rocket emoji
@dunglas
Copy link
Member

Usually, we don't accept performance improvements (even huge ones like this) as bug fixes. Maybe could it be included in the next 4.3 beta (it's already late, but it's a huge improvement ; ping @symfony/deciders), but definitely not in 4.2.

It looks like a introduced this class a while ago... and forgot to open the follow up PR with the FrameworkBundle wiring 😕. Thanks for finding this.

@alanpoulain
Copy link
ContributorAuthor

The test failures seem unrelated.

@nicolas-grekasnicolas-grekas added this to the4.3 milestoneMay 11, 2019
@alanpoulainalanpoulainforce-pushed thecache-configuration-property-info branch fromc04e598 to7290c4aCompareMay 11, 2019 10:22
@alanpoulainalanpoulain changed the base branch from4.2 to4.3May 11, 2019 10:23
@alanpoulain
Copy link
ContributorAuthor

Rebased for 4.3.

@alanpoulain
Copy link
ContributorAuthor

Should I take account of@dunglas's comment:

IIRC, it's preferred to use XML for the service definition and to remove it in the extension is appropriate, to allow IDEs like PHPStorm to detect the services. Not sure if it's really matter here has it's the same interface that is implemented, it's more for consistency.

And do the same forClassMetadataFactory (I don't think it's a BC break)?

@nicolas-grekas
Copy link
Member

Yes please.

@alanpoulainalanpoulainforce-pushed thecache-configuration-property-info branch frome685d6f to05bb11cCompareMay 11, 2019 13:25
@alanpoulain
Copy link
ContributorAuthor

Changes done.

@fabpotfabpotforce-pushed thecache-configuration-property-info branch from05bb11c to17f6225CompareMay 13, 2019 06:44
@fabpot
Copy link
Member

Thank you@alanpoulain.

@fabpotfabpot merged commit17f6225 intosymfony:4.3May 13, 2019
fabpot added a commit that referenced this pull requestMay 13, 2019
…Info (alanpoulain)This PR was squashed before being merged into the 4.3 branch (closes#31452).Discussion----------[FrameworkBundle] Add cache configuration for PropertyInfo| Q             | A| ------------- | ---| Branch?       | 4.3| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? || Tests pass?   | yes| Fixed tickets | n/a| License       | MIT| Doc PR        | n/aThe PropertyInfoExtractor was not cached by default but the class to do it was surprisingly here, unused.I've added the configuration to enable it by default when it's not the development environment.I haven't added the warmup part because there are too much arguments for the methods (class, property, context).It will be a big boost for the performance! For this code:```php$book = $this->serializer->deserialize('{"id":3,"reviews":[{"id": 7, "body": "This book is fantastic!", "rating": 9, "letter": "A", "publicationDate": "2019"}],"isbn":"978-0-5533-9243-2","title":"Fool\'s Assassin","description":"A famous saga","author":"Robin Hobb","publicationDate":"2014"}', Book::class, 'json');$this->serializer->serialize($book, 'json');```We obtain this:![image](https://user-images.githubusercontent.com/10920253/57487994-2874d000-72b2-11e9-92a2-28b14a038194.png)The Blackfire comparison is here:https://blackfire.io/profiles/compare/2c746d26-320a-4aab-80ef-7276c2e92b96/graphCommits-------17f6225 [FrameworkBundle] Add cache configuration for PropertyInfo
@fabpotfabpot mentioned this pull requestMay 22, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@fabpotfabpotfabpot approved these changes

@dunglasdunglasdunglas approved these changes

@lyrixxlyrixxAwaiting requested review from lyrixx

@srozesrozeAwaiting requested review from sroze

@xabbuhxabbuhAwaiting requested review from xabbuh

+1 more reviewer

@maxheliasmaxheliasmaxhelias approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

6 participants

@alanpoulain@dunglas@nicolas-grekas@fabpot@maxhelias@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp