Skip to content

Commit 6b11922

Browse files
committed
feature #18359 [Form] [DoctrineBridge] optimized LazyChoiceList and DoctrineChoiceLoader (HeahDude)
This PR was merged into the 3.1-dev branch. Discussion ---------- [Form] [DoctrineBridge] optimized LazyChoiceList and DoctrineChoiceLoader | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | ~ | License | MIT | Doc PR | ~ Problem ====== Actually we got a circular dependency: | Object | Return |----------|-------------| |`DefaultChoiceListFactory::createListFromLoader()` (decorated) | `LazyChoiceList` with loader and resolved `$value` | `LazyChoiceList::get*` | `DoctrineChoiceLoader` with resolved `$value` `DoctrineChoiceLoader::loadChoiceList()` | (decorated) `DefaultChoiceListFactory` with loaded choices and resolved `$value` `DefaultChoiceListFactory::createListFromChoices()` | `ArrayChoiceList` with `resolved `$value` With this refactoring, the `DoctrineChoiceLoader` is no longer dependant to the factory, the `ChoiceLoaderInterface::loadChoiceList()` must return a `ChoiceListInterface` but should not need a decorated factory while `$value` is already resolved. It should remain lazy IMHO. Solution ====== | Object | Return | |----------|-----------| | `DefaultChoiceListFactory::createListFromLoader()` | `LazyChoiceList` with loader and resolved `$value` | `LazyChoiceList::get*()` | `DoctrineChoiceLoader` with resolved `$value` | `DoctrineChoiceLoader::loadChoiceList()` | `ArrayChoiceList` with resolved `$value`. Since `choiceListFactory` is a private property, this change should be safe regarding BC. To justify this change, I've made some blackfire profiling. You can see my [branch of SE here](https://github.com/HeahDude/symfony-standard/tree/test/optimize-doctrine_choice_loader) and the [all in file test implementation](https://github.com/HeahDude/symfony-standard/blob/test/optimize-doctrine_choice_loader/src/AppBundle/Controller/DefaultController.php). Basically it loads a form with 3 `EntityType` fields with different classes holding 50 instances each. (INIT events are profiled with an empty cache) When | What | Diff (SE => PR) --------|-------|------ INIT (1) | build form (load types) | [see](https://blackfire.io/profiles/compare/061d5d28-15c6-4e01-b8c0-3edc9cb8daf0/graph) INIT (2) | build view (load choices) | [see](https://blackfire.io/profiles/compare/04f142a8-d886-405a-be4d-636ba82d8acd/graph) CACHED | build form (load types) | [see](https://blackfire.io/profiles/compare/293b27b6-aa58-42ae-bafb-655513201505/graph) CACHED | build view (load choices) | [see](https://blackfire.io/profiles/compare/e5b37dfe-cc9e-498f-b98a-7448830ad190/graph) SUBMIT | build form (load types) | [see](https://blackfire.io/profiles/compare/7f3baea9-0d27-46b6-8c24-c577742382dc/graph) SUBMIT | handle request (load choices) | [see](https://blackfire.io/profiles/compare/8644ebfb-4397-495b-8f3d-1a6e1d7f8476/graph) SUBMIT | build view (load values) | [see](https://blackfire.io/profiles/compare/89c3cc7c-ea07-4300-91b3-99004cb58ea1/graph) (1): ![build_form-no_cache](https://cloud.githubusercontent.com/assets/10107633/14136166/b5a85eb8-f661-11e5-8556-3e0dcbfaf404.jpg) (2): ![build_view-no_cache](https://cloud.githubusercontent.com/assets/10107633/14136240/1162f3ee-f662-11e5-834a-1ed1e519dc83.jpg) It can seem like 1 and 2 balance each other but it comes clear when comparing values: | - | Build form | Build view | |-----|---------|--------------| | wall time | -88 ms | +9.71 ms | blocking I/O | -40 ms | +3.67 ms | cpu | -48 ms | +13.4 ms | memory | -4.03 MB | +236 kB | network | -203 B | +2.21 kB Commits ------- 98621f4 [Form] optimized LazyChoiceList 86b2ff1 [DoctrineBridge] optimized DoctrineChoiceLoader
2 parents 32fd187 + 2424982 commit 6b11922

File tree

2 files changed

+18
-9
lines changed

2 files changed

+18
-9
lines changed

Form/ChoiceList/DoctrineChoiceLoader.php

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
namespace Symfony\Bridge\Doctrine\Form\ChoiceList;
1313

1414
use Doctrine\Common\Persistence\ObjectManager;
15+
use Symfony\Component\Form\ChoiceList\ArrayChoiceList;
1516
use Symfony\Component\Form\ChoiceList\ChoiceListInterface;
1617
use Symfony\Component\Form\ChoiceList\Factory\ChoiceListFactoryInterface;
1718
use Symfony\Component\Form\ChoiceList\Loader\ChoiceLoaderInterface;
@@ -60,20 +61,31 @@ class DoctrineChoiceLoader implements ChoiceLoaderInterface
6061
* passed which optimizes the object loading for one of the Doctrine
6162
* mapper implementations.
6263
*
63-
* @param ChoiceListFactoryInterface $factory The factory for creating
64-
* the loaded choice list
6564
* @param ObjectManager $manager The object manager
6665
* @param string $class The class name of the
6766
* loaded objects
6867
* @param IdReader $idReader The reader for the object
6968
* IDs.
69+
* @param ChoiceListFactoryInterface $factory The factory for creating
70+
* the loaded choice list
7071
* @param null|EntityLoaderInterface $objectLoader The objects loader
7172
*/
72-
public function __construct(ChoiceListFactoryInterface $factory, ObjectManager $manager, $class, IdReader $idReader = null, EntityLoaderInterface $objectLoader = null)
73+
public function __construct($manager, $class, $idReader = null, $objectLoader = null, $factory = null)
7374
{
75+
// BC to be removed and replace with type hints in 4.0
76+
if ($manager instanceof ChoiceListFactoryInterface) {
77+
@trigger_error(sprintf('Passing a ChoiceListFactoryInterface to %s is deprecated since version 3.1 and will no longer be supported in 4.0. You should either call "%s::loadChoiceList" or override it to return a ChoiceListInterface.', __CLASS__, __CLASS__));
78+
79+
// Provide a BC layer since $factory has changed
80+
// form first to last argument as of 3.1
81+
$this->factory = $manager;
82+
$manager = $class;
83+
$class = $idReader;
84+
$objectLoader = $factory;
85+
}
86+
7487
$classMetadata = $manager->getClassMetadata($class);
7588

76-
$this->factory = $factory;
7789
$this->manager = $manager;
7890
$this->class = $classMetadata->getName();
7991
$this->idReader = $idReader ?: new IdReader($manager, $classMetadata);
@@ -93,9 +105,7 @@ public function loadChoiceList($value = null)
93105
? $this->objectLoader->getEntities()
94106
: $this->manager->getRepository($this->class)->findAll();
95107

96-
$this->choiceList = $this->factory->createListFromChoices($objects, $value);
97-
98-
return $this->choiceList;
108+
return $this->choiceList = new ArrayChoiceList($objects, $value);
99109
}
100110

101111
/**
@@ -146,7 +156,7 @@ public function loadChoicesForValues(array $values, $value = null)
146156

147157
// Optimize performance in case we have an object loader and
148158
// a single-field identifier
149-
$optimize = null === $value || is_array($value) && $value[0] === $this->idReader;
159+
$optimize = null === $value || is_array($value) && $this->idReader === $value[0];
150160

151161
if ($optimize && !$this->choiceList && $this->objectLoader && $this->idReader->isSingleId()) {
152162
$unorderedObjects = $this->objectLoader->getEntitiesByIds($this->idReader->getIdField(), $values);

Form/Type/DoctrineType.php

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,6 @@ public function configureOptions(OptionsResolver $resolver)
160160
}
161161

162162
$doctrineChoiceLoader = new DoctrineChoiceLoader(
163-
$this->choiceListFactory,
164163
$options['em'],
165164
$options['class'],
166165
$options['id_reader'],

0 commit comments

Comments
 (0)