diogo/symfony
Archived
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

feature #18359 [Form] [DoctrineBridge] optimized LazyChoiceList and DoctrineChoiceLoader (HeahDude)

Browse Source 
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
This commit is contained in:
Fabien Potencier 2016-04-28 12:16:40 +02:00
commit 0c5b83ab95
4 changed files with 154 additions and 47 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php
View File

@ -12,6 +12,7 @@
namespace Symfony\Bridge\Doctrine\Form\ChoiceList;
use Doctrine\Common\Persistence\ObjectManager;
use Symfony\Component\Form\ChoiceList\ArrayChoiceList;
use Symfony\Component\Form\ChoiceList\ChoiceListInterface;
use Symfony\Component\Form\ChoiceList\Factory\ChoiceListFactoryInterface;
use Symfony\Component\Form\ChoiceList\Loader\ChoiceLoaderInterface;
@ -60,20 +61,31 @@ class DoctrineChoiceLoader implements ChoiceLoaderInterface
* passed which optimizes the object loading for one of the Doctrine
* mapper implementations.
*
* @param ChoiceListFactoryInterface $factory The factory for creating
* the loaded choice list
* @param ObjectManager $manager The object manager
* @param string $class The class name of the
* loaded objects
* @param IdReader $idReader The reader for the object
* IDs.
* @param ChoiceListFactoryInterface $factory The factory for creating
* the loaded choice list
* @param null|EntityLoaderInterface $objectLoader The objects loader
*/
public function __construct(ChoiceListFactoryInterface $factory, ObjectManager $manager, $class, IdReader $idReader = null, EntityLoaderInterface $objectLoader = null)
public function __construct($manager, $class, $idReader = null, $objectLoader = null, $factory = null)
{
// BC to be removed and replace with type hints in 4.0
if ($manager instanceof ChoiceListFactoryInterface) {
@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__));
// Provide a BC layer since $factory has changed
// form first to last argument as of 3.1
$this->factory = $manager;
$manager = $class;
$class = $idReader;
$objectLoader = $factory;
}
$classMetadata = $manager->getClassMetadata($class);
$this->factory = $factory;
$this->manager = $manager;
$this->class = $classMetadata->getName();
$this->idReader = $idReader ?: new IdReader($manager, $classMetadata);
@ -93,9 +105,7 @@ class DoctrineChoiceLoader implements ChoiceLoaderInterface
? $this->objectLoader->getEntities()
: $this->manager->getRepository($this->class)->findAll();
$this->choiceList = $this->factory->createListFromChoices($objects, $value);
return $this->choiceList;
return $this->choiceList = new ArrayChoiceList($objects, $value);
}
/**
@ -146,7 +156,7 @@ class DoctrineChoiceLoader implements ChoiceLoaderInterface
// Optimize performance in case we have an object loader and
// a single-field identifier
$optimize = null === $value || is_array($value) && $value[0] === $this->idReader;
$optimize = null === $value || is_array($value) && $this->idReader === $value[0];
if ($optimize && !$this->choiceList && $this->objectLoader && $this->idReader->isSingleId()) {
$unorderedObjects = $this->objectLoader->getEntitiesByIds($this->idReader->getIdField(), $values);

1
src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php
View File

@ -160,7 +160,6 @@ abstract class DoctrineType extends AbstractType
}
$doctrineChoiceLoader = new DoctrineChoiceLoader(
$this->choiceListFactory,
$options['em'],
$options['class'],
$options['id_reader'],

92
src/Symfony/Component/Form/ChoiceList/LazyChoiceList.php
View File

@ -45,9 +45,18 @@ class LazyChoiceList implements ChoiceListInterface
/**
* @var ChoiceListInterface|null
*
* @deprecated Since 3.1, to be remove in 4.0. Cache the choice list in the {@link ChoiceLoaderInterface} instead.
*/
private $loadedList;
/**
* @var bool
*
* @deprecated Flag used for BC layer since 3.1. To be removed in 4.0.
*/
private $loaded = false;
/**
* Creates a lazily-loaded list using the given loader.
*
@ -70,11 +79,23 @@ class LazyChoiceList implements ChoiceListInterface
*/
public function getChoices()
{
if (!$this->loadedList) {
$this->loadedList = $this->loader->loadChoiceList($this->value);
if ($this->loaded) {
// We can safely invoke the {@link ChoiceLoaderInterface} assuming it has the list
// in cache when the lazy list is already loaded
if ($this->loadedList !== $this->loader->loadChoiceList($this->value)) {
@trigger_error(sprintf('Caching the choice list in %s is deprecated since 3.1 and will not happen in 4.0. Cache the list in the %s instead.', __CLASS__, ChoiceLoaderInterface::class));
}
return $this->loadedList->getChoices();
}
// BC
$this->loadedList = $this->loader->loadChoiceList($this->value);
$this->loaded = true;
return $this->loadedList->getChoices();
// In 4.0 keep the following line only:
// return $this->loader->loadChoiceList($this->value)->getChoices()
}
/**
@ -82,11 +103,22 @@ class LazyChoiceList implements ChoiceListInterface
*/
public function getValues()
{
if (!$this->loadedList) {
$this->loadedList = $this->loader->loadChoiceList($this->value);
if ($this->loaded) {
// Check whether the loader has the same cache
if ($this->loadedList !== $this->loader->loadChoiceList($this->value)) {
@trigger_error(sprintf('Caching the choice list in %s is deprecated since 3.1 and will not happen in 4.0. Cache the list in the %s instead.', __CLASS__, ChoiceLoaderInterface::class));
}
return $this->loadedList->getValues();
}
// BC
$this->loadedList = $this->loader->loadChoiceList($this->value);
$this->loaded = true;
return $this->loadedList->getValues();
// In 4.0 keep the following line only:
// return $this->loader->loadChoiceList($this->value)->getValues()
}
/**
@ -94,11 +126,22 @@ class LazyChoiceList implements ChoiceListInterface
*/
public function getStructuredValues()
{
if (!$this->loadedList) {
$this->loadedList = $this->loader->loadChoiceList($this->value);
if ($this->loaded) {
// Check whether the loader has the same cache
if ($this->loadedList !== $this->loader->loadChoiceList($this->value)) {
@trigger_error(sprintf('Caching the choice list in %s is deprecated since 3.1 and will not happen in 4.0. Cache the list in the %s instead.', __CLASS__, ChoiceLoaderInterface::class));
}
return $this->loadedList->getStructuredValues();
}
// BC
$this->loadedList = $this->loader->loadChoiceList($this->value);
$this->loaded = true;
return $this->loadedList->getStructuredValues();
// In 4.0 keep the following line only:
// return $this->loader->loadChoiceList($this->value)->getStructuredValues();
}
/**
@ -106,11 +149,22 @@ class LazyChoiceList implements ChoiceListInterface
*/
public function getOriginalKeys()
{
if (!$this->loadedList) {
$this->loadedList = $this->loader->loadChoiceList($this->value);
if ($this->loaded) {
// Check whether the loader has the same cache
if ($this->loadedList !== $this->loader->loadChoiceList($this->value)) {
@trigger_error(sprintf('Caching the choice list in %s is deprecated since 3.1 and will not happen in 4.0. Cache the list in the %s instead.', __CLASS__, ChoiceLoaderInterface::class));
}
return $this->loadedList->getOriginalKeys();
}
// BC
$this->loadedList = $this->loader->loadChoiceList($this->value);
$this->loaded = true;
return $this->loadedList->getOriginalKeys();
// In 4.0 keep the following line only:
// return $this->loader->loadChoiceList($this->value)->getOriginalKeys();
}
/**
@ -118,11 +172,16 @@ class LazyChoiceList implements ChoiceListInterface
*/
public function getChoicesForValues(array $values)
{
if (!$this->loadedList) {
return $this->loader->loadChoicesForValues($values, $this->value);
if ($this->loaded) {
// Check whether the loader has the same cache
if ($this->loadedList !== $this->loader->loadChoiceList($this->value)) {
@trigger_error(sprintf('Caching the choice list in %s is deprecated since 3.1 and will not happen in 4.0. Cache the list in the %s instead.', __CLASS__, ChoiceLoaderInterface::class));
}
return $this->loadedList->getChoicesForValues($values);
}
return $this->loadedList->getChoicesForValues($values);
return $this->loader->loadChoicesForValues($values, $this->value);
}
/**
@ -130,10 +189,15 @@ class LazyChoiceList implements ChoiceListInterface
*/
public function getValuesForChoices(array $choices)
{
if (!$this->loadedList) {
return $this->loader->loadValuesForChoices($choices, $this->value);
if ($this->loaded) {
// Check whether the loader has the same cache
if ($this->loadedList !== $this->loader->loadChoiceList($this->value)) {
@trigger_error(sprintf('Caching the choice list in %s is deprecated since 3.1 and will not happen in 4.0. Cache the list in the %s instead.', __CLASS__, ChoiceLoaderInterface::class));
}
return $this->loadedList->getValuesForChoices($choices);
}
return $this->loadedList->getValuesForChoices($choices);
return $this->loader->loadValuesForChoices($choices, $this->value);
}
}

82
src/Symfony/Component/Form/Tests/ChoiceList/LazyChoiceListTest.php
View File

@ -11,6 +11,7 @@
namespace Symfony\Component\Form\Tests\ChoiceList;
use Symfony\Component\Form\ChoiceList\ArrayChoiceList;
use Symfony\Component\Form\ChoiceList\LazyChoiceList;
/**
@ -26,7 +27,7 @@ class LazyChoiceListTest extends \PHPUnit_Framework_TestCase
/**
* @var \PHPUnit_Framework_MockObject_MockObject
*/
private $innerList;
private $loadedList;
/**
* @var \PHPUnit_Framework_MockObject_MockObject
@ -37,20 +38,21 @@ class LazyChoiceListTest extends \PHPUnit_Framework_TestCase
protected function setUp()
{
$this->innerList = $this->getMock('Symfony\Component\Form\ChoiceList\ChoiceListInterface');
$this->loadedList = $this->getMock('Symfony\Component\Form\ChoiceList\ChoiceListInterface');
$this->loader = $this->getMock('Symfony\Component\Form\ChoiceList\Loader\ChoiceLoaderInterface');
$this->value = function () {};
$this->list = new LazyChoiceList($this->loader, $this->value);
}
public function testGetChoicesLoadsInnerListOnFirstCall()
public function testGetChoiceLoadersLoadsLoadedListOnFirstCall()
{
$this->loader->expects($this->once())
$this->loader->expects($this->exactly(2))
->method('loadChoiceList')
->with($this->value)
->will($this->returnValue($this->innerList));
->will($this->returnValue($this->loadedList));
$this->innerList->expects($this->exactly(2))
// The same list is returned by the loader
$this->loadedList->expects($this->exactly(2))
->method('getChoices')
->will($this->returnValue('RESULT'));
@ -58,14 +60,39 @@ class LazyChoiceListTest extends \PHPUnit_Framework_TestCase
$this->assertSame('RESULT', $this->list->getChoices());
}
public function testGetValuesLoadsInnerListOnFirstCall()
/**
* @group legacy
*/
public function testGetChoicesUsesLoadedListWhenLoaderDoesNotCacheChoiceListOnFirstCall()
{
$this->loader->expects($this->once())
$this->loader->expects($this->at(0))
->method('loadChoiceList')
->with($this->value)
->will($this->returnValue($this->innerList));
->willReturn($this->loadedList);
$this->innerList->expects($this->exactly(2))
$this->loader->expects($this->at(1))
->method('loadChoiceList')
->with($this->value)
->willReturn(new ArrayChoiceList(array('a', 'b')));
// The same list is returned by the lazy choice list
$this->loadedList->expects($this->exactly(2))
->method('getChoices')
->will($this->returnValue('RESULT'));
$this->assertSame('RESULT', $this->list->getChoices());
$this->assertSame('RESULT', $this->list->getChoices());
}
public function testGetValuesLoadsLoadedListOnFirstCall()
{
$this->loader->expects($this->exactly(2))
->method('loadChoiceList')
->with($this->value)
->will($this->returnValue($this->loadedList));
// The same list is returned by the loader
$this->loadedList->expects($this->exactly(2))
->method('getValues')
->will($this->returnValue('RESULT'));
@ -73,14 +100,15 @@ class LazyChoiceListTest extends \PHPUnit_Framework_TestCase
$this->assertSame('RESULT', $this->list->getValues());
}
public function testGetStructuredValuesLoadsInnerListOnFirstCall()
public function testGetStructuredValuesLoadsLoadedListOnFirstCall()
{
$this->loader->expects($this->once())
$this->loader->expects($this->exactly(2))
->method('loadChoiceList')
->with($this->value)
->will($this->returnValue($this->innerList));
->will($this->returnValue($this->loadedList));
$this->innerList->expects($this->exactly(2))
// The same list is returned by the loader
$this->loadedList->expects($this->exactly(2))
->method('getStructuredValues')
->will($this->returnValue('RESULT'));
@ -88,14 +116,15 @@ class LazyChoiceListTest extends \PHPUnit_Framework_TestCase
$this->assertSame('RESULT', $this->list->getStructuredValues());
}
public function testGetOriginalKeysLoadsInnerListOnFirstCall()
public function testGetOriginalKeysLoadsLoadedListOnFirstCall()
{
$this->loader->expects($this->once())
$this->loader->expects($this->exactly(2))
->method('loadChoiceList')
->with($this->value)
->will($this->returnValue($this->innerList));
->will($this->returnValue($this->loadedList));
$this->innerList->expects($this->exactly(2))
// The same list is returned by the loader
$this->loadedList->expects($this->exactly(2))
->method('getOriginalKeys')
->will($this->returnValue('RESULT'));
@ -116,15 +145,17 @@ class LazyChoiceListTest extends \PHPUnit_Framework_TestCase
public function testGetChoicesForValuesUsesLoadedList()
{
$this->loader->expects($this->once())
$this->loader->expects($this->exactly(3))
->method('loadChoiceList')
->with($this->value)
->will($this->returnValue($this->innerList));
// For BC, the same choice loaded list is returned 3 times
// It should only twice in 4.0
->will($this->returnValue($this->loadedList));
$this->loader->expects($this->never())
->method('loadChoicesForValues');
$this->innerList->expects($this->exactly(2))
$this->loadedList->expects($this->exactly(2))
->method('getChoicesForValues')
->with(array('a', 'b'))
->will($this->returnValue('RESULT'));
@ -136,6 +167,7 @@ class LazyChoiceListTest extends \PHPUnit_Framework_TestCase
$this->assertSame('RESULT', $this->list->getChoicesForValues(array('a', 'b')));
}
// To be remove in 4.0
public function testGetValuesForChoicesForwardsCallIfListNotLoaded()
{
$this->loader->expects($this->exactly(2))
@ -149,15 +181,17 @@ class LazyChoiceListTest extends \PHPUnit_Framework_TestCase
public function testGetValuesForChoicesUsesLoadedList()
{
$this->loader->expects($this->once())
$this->loader->expects($this->exactly(3))
->method('loadChoiceList')
->with($this->value)
->will($this->returnValue($this->innerList));
// For BC, the same choice loaded list is returned 3 times
// It should only twice in 4.0
->will($this->returnValue($this->loadedList));
$this->loader->expects($this->never())
->method('loadValuesForChoices');
$this->innerList->expects($this->exactly(2))
$this->loadedList->expects($this->exactly(2))
->method('getValuesForChoices')
->with(array('a', 'b'))
->will($this->returnValue('RESULT'));