[Form] Improved performance of ChoiceType and its subtypes #16747

webmozart · 2015-11-29T20:03:53Z

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.

Tobion · 2015-11-29T20:29:42Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/form.xml

+
+        <service id="form.choice_list_factory.property_access" class="Symfony\Component\Form\ChoiceList\Factory\PropertyAccessDecorator" public="false">
+            <argument type="service" id="form.choice_list_factory.default"/>
+            <argument type="service" id="property_accessor"/>


I think this should be form.property_accessor

Isn't property_accessor is the default name of the service?

form.property_accessor is an alias for property_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

I saw that - why?

So I guess the point is that people can override them with different ones? Where's the use case?

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.

Ok, updated

stof · 2015-11-29T21:37:10Z

src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php

@@ -32,6 +32,11 @@
 abstract class DoctrineType extends AbstractType
 {
    /**
+     * @var DoctrineChoiceLoader[]
+     */
+    private static $choiceLoaders = array();


should it really be static ? Sharing the cache between the ORM and ODM types could be an issue

That's fine, as the (object hash of the) object manager is part of the cache key

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.

Tobion · 2015-11-30T21:11:23Z

👍

Status: Reviewed

stof · 2015-12-05T10:57:10Z

src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php

     */
-    protected $registry;
+    private static $choiceLoaders = array();


IMO, this should not be static. We need separate caches for the ORM and ODM types

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.

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)

Updated. There was some reason why I made it static, but I can't remember. We'll find out.

webmozart · 2015-12-28T10:11:55Z

PR updated

sstok · 2015-12-28T11:58:08Z

👍

xabbuh · 2015-12-28T12:13:33Z

👍

stof · 2015-12-28T12:34:54Z

src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php

     */
-    private $choiceLoaders = array();
+    private $choiceListFactory;


I suggest keeping the properties in the same order than previously, to avoid messing with git blame

stof · 2015-12-28T12:36:10Z

except for my suggestion above, 👍

webmozart · 2015-12-30T10:12:19Z

Updated

Tobion · 2015-12-30T15:04:32Z

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 · 2015-12-30T19:14:20Z

@webmozart could you please have a look at the failing test here?
https://travis-ci.org/symfony/symfony/jobs/99478577

patrick-mcdougle · 2016-01-14T15:30:39Z

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

carsonbot added the Status: Needs Review label Nov 29, 2015

webmozart added Form Bug labels Nov 29, 2015

webmozart force-pushed the choice-performance branch from e73f28b to 0972b9e Compare November 29, 2015 20:06

Tobion reviewed Nov 29, 2015
View reviewed changes

webmozart force-pushed the choice-performance branch from 0972b9e to d1d5384 Compare November 29, 2015 21:07

stof reviewed Nov 29, 2015
View reviewed changes

webmozart force-pushed the choice-performance branch 2 times, most recently from 284ae32 to eacffff Compare November 30, 2015 20:31

carsonbot added Status: Reviewed and removed Status: Needs Review labels Nov 30, 2015

stof reviewed Dec 5, 2015
View reviewed changes

webmozart force-pushed the choice-performance branch from eacffff to 0c33d97 Compare December 28, 2015 10:11

stof reviewed Dec 28, 2015
View reviewed changes

[Form] Improved performance of ChoiceType and its subtypes

a0ef101

webmozart force-pushed the choice-performance branch from 0c33d97 to a0ef101 Compare December 30, 2015 10:11

Tobion merged commit a0ef101 into symfony:2.7 Dec 30, 2015

This was referenced Jan 14, 2016

Release v2.7.9 #17367

Merged

Release v2.8.2 #17368

Merged

xabbuh mentioned this pull request Jan 19, 2016

[Form] Query builder is run for each entity within each instance of a form within a collection #8658

Closed

fabpot mentioned this pull request Feb 3, 2016

Release v3.0.2 #17665

Merged

HeahDude mentioned this pull request Mar 29, 2016

[Form] added CallbackChoiceLoader and refactored ChoiceType's children #18332

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form] Improved performance of ChoiceType and its subtypes #16747

[Form] Improved performance of ChoiceType and its subtypes #16747

webmozart commented Nov 29, 2015

Tobion Nov 29, 2015

webmozart Nov 30, 2015

xabbuh Nov 30, 2015

webmozart Nov 30, 2015

Tobion Nov 30, 2015

webmozart Nov 30, 2015

theofidry Nov 30, 2015

webmozart Nov 30, 2015

stof Nov 29, 2015

webmozart Nov 30, 2015

stof Dec 5, 2015

Tobion commented Nov 30, 2015

stof Dec 5, 2015

webmozart Dec 17, 2015

stof Dec 17, 2015

webmozart Dec 28, 2015

webmozart commented Dec 28, 2015

sstok commented Dec 28, 2015

xabbuh commented Dec 28, 2015

stof Dec 28, 2015

stof commented Dec 28, 2015

webmozart commented Dec 30, 2015

Tobion commented Dec 30, 2015

nicolas-grekas commented Dec 30, 2015

patrick-mcdougle commented Jan 14, 2016

[Form] Improved performance of ChoiceType and its subtypes #16747

[Form] Improved performance of ChoiceType and its subtypes #16747

Conversation

webmozart commented Nov 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion commented Nov 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webmozart commented Dec 28, 2015

sstok commented Dec 28, 2015

xabbuh commented Dec 28, 2015

Choose a reason for hiding this comment

stof commented Dec 28, 2015

webmozart commented Dec 30, 2015

Tobion commented Dec 30, 2015

nicolas-grekas commented Dec 30, 2015

patrick-mcdougle commented Jan 14, 2016