Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
dunglas commentedMay 10, 2019
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. |
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
alanpoulain commentedMay 10, 2019
The test failures seem unrelated. |
c04e598 to7290c4aComparealanpoulain commentedMay 11, 2019
Rebased for 4.3. |
alanpoulain commentedMay 11, 2019
Should I take account of@dunglas's comment:
And do the same for |
nicolas-grekas commentedMay 11, 2019
Yes please. |
e685d6f to05bb11cComparealanpoulain commentedMay 11, 2019
Changes done. |
05bb11c to17f6225Comparefabpot commentedMay 13, 2019
Thank you@alanpoulain. |
…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:The Blackfire comparison is here:https://blackfire.io/profiles/compare/2c746d26-320a-4aab-80ef-7276c2e92b96/graphCommits-------17f6225 [FrameworkBundle] Add cache configuration for PropertyInfo
Uh oh!
There was an error while loading.Please reload this page.
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:
We obtain this:
The Blackfire comparison is here:https://blackfire.io/profiles/compare/2c746d26-320a-4aab-80ef-7276c2e92b96/graph