Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Form] Improved performance of ChoiceType and its subtypes#16747
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
e73f28b to0972b9eCompareThere 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.
I think this should be form.property_accessor
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.
Isn't property_accessor is the default name of the service?
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.
form.property_accessor is an alias forproperty_accessor to be used for form related property access stuff:https://github.com/symfony/symfony/blob/2.8/src/Symfony/Bundle/FrameworkBundle/Resources/config/form.xml#L57-L63
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.
I saw that - why?
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.
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.
So I guess the point is that people can override them with different ones? Where's the use case?
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.
So I guess the point is that people can override them with different ones?
It is. For example I had to override the property accessor used by the serializer to have a more perform one for my app: the Symfony one relies on reflection and allow a bunch of conventions.
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.
Ok, updated
0972b9e tod1d5384CompareThere 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.
should it really be static ? Sharing the cache between the ORM and ODM types could be an issue
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.
That's fine, as the (object hash of the) object manager is part of the cache key
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.
but this would just force to load them in memory longer (in tests, this will force choice loaders to stay in memory until the end of the PHPUnit process, which implies keeping the whole Doctrine as well and so on), with no real benefit as we won't need to share the same cache between multiple types.
284ae32 toeacffffCompareTobion commentedNov 30, 2015
👍 Status: Reviewed |
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.
IMO, this should not be static. We need separate caches for the ORM and ODM types
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.
This is not a problem. The object manager hash is included in the cache key, so entities assigned to separate object managers will naturally have different cache keys.
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.
well, see my previous comments (on the hidden discussion): making it static brings no benefit (as several instances will not share the same cache keys), and will leak memory by keeping the cached loaders in memory until the end of the process rather than until the instance is destroyed (which can make a huge difference in tests, where each test gets a new container, and so a different set of cached data)
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.
Updated. There was some reason why I made it static, but I can't remember. We'll find out.
eacffff to0c33d97Comparewebmozart commentedDec 28, 2015
PR updated |
sstok commentedDec 28, 2015
👍 |
1 similar comment
xabbuh commentedDec 28, 2015
👍 |
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.
I suggest keeping the properties in the same order than previously, to avoid messing withgit blame
stof commentedDec 28, 2015
except for my suggestion above, 👍 |
0c33d97 toa0ef101Comparewebmozart commentedDec 30, 2015
Updated |
Tobion commentedDec 30, 2015
Thank you@webmozart. |
…ypes (webmozart)This PR was merged into the 2.7 branch.Discussion----------[Form] Improved performance of ChoiceType and its subtypes| Q | A| ------------- | ---| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -I found out today that, although CachingFactoryDecorator is part of Symfony 2.7, it is not configured to be used in the DI configuration. This simple in-memory cache improved the page load by 50% for one considerably large form with many (~600) choice/entity fields that I was working on today.Also, caching of query builders with parameters was broken, since the parameters are represented by objects. PHP's object hashes were used to calculate the cache keys, hence the cache always missed. I converted parameters to arrays for calculating the cache keys to fix this problem.Commits-------a0ef101 [Form] Improved performance of ChoiceType and its subtypes
nicolas-grekas commentedDec 30, 2015
@webmozart could you please have a look at the failing test here? |
patrick-mcdougle commentedJan 14, 2016
Just a thought, Should we use the decorated service definitions as outlined here:http://symfony.com/doc/current/components/dependency_injection/advanced.html#decorating-services |
I found out today that, although CachingFactoryDecorator is part of Symfony 2.7, it is not configured to be used in the DI configuration. This simple in-memory cache improved the page load by 50% for one considerably large form with many (~600) choice/entity fields that I was working on today.
Also, caching of query builders with parameters was broken, since the parameters are represented by objects. PHP's object hashes were used to calculate the cache keys, hence the cache always missed. I converted parameters to arrays for calculating the cache keys to fix this problem.