[Form] WIP Improved performance of form building #4882
Conversation
|
Maybe ResolvedFormType |
|
I really like ResolvedFormType. That's the naming method I took for my tag parser that handes the same conceptual issue. |
|
ResolvedFormType sounds very clear. |
|
Yes |
|
I like |
|
👍 |
| * For the full copyright and license information, please view the LICENSE | ||
| * file that was distributed with this source code. | ||
| */ | ||
|
|
There was a problem hiding this comment.
maybe you can mark this test as a "benchmark" group so it can be excluded easily
/**
* @group benchmark
*/
phpunit.xml.dist already does exclude this group
<groups>
<exclude>
<group>benchmark</group>
</exclude>
</groups>
There was a problem hiding this comment.
We only add to the benchmark group test that are really slow. That's not the case here.
| @@ -6,13 +6,14 @@ | |||
|
|
|||
| <parameters> | |||
| <parameter key="form.extension.class">Symfony\Component\Form\Extension\DependencyInjection\DependencyInjectionExtension</parameter> | |||
| <parameter key="form.registry.class">Symfony\Component\Form\FormRegistry</parameter> | |||
| <parameter key="form.factory.class">Symfony\Component\Form\FormFactory</parameter> | |||
| <parameter key="form.type_guesser.validator.class">Symfony\Component\Form\Extension\Validator\ValidatorTypeGuesser</parameter> | |||
| </parameters> | |||
|
|
|||
| <services> | |||
| <!-- FormFactory --> | |||
|
This looks good to me |
| foreach ($form->getConfig()->getTypes() as $type) { | ||
| $types[] = $type->getName(); | ||
| for ($type = $form->getConfig()->getType(); null !== $type; $type = $type->getParent()) { | ||
| array_unshift($types, $type->getName()); |
There was a problem hiding this comment.
This appears to duplicate Form::getTypes(). Is there any reason you're not just calling that?
There was a problem hiding this comment.
Yes, Form::getTypes() is deprecated.
Commits ------- cd7835d [Form] Cached the form type hierarchy in order to improve performance 2ca753b [Form] Fixed choice list hashing in DoctrineType 2bf4d6c [Form] Fixed FormFactory not to set "data" option if not explicitely given 7149d26 [Form] Removed invalid PHPDoc text Discussion ---------- [Form] WIP Improved performance of form building Bug fix: no Feature addition: no Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: - Todo: **Update the Silex extension** This PR is work in progress and up for discussion. It increases the performance of FormFactory::createForm() on a specific, heavy-weight form from **0.848** to **0.580** seconds. Before, the FormFactory had to traverse the hierarchy and calculate the default options of each FormType everytime a form was created of that type. Now, FormTypes are wrapped within instances of a new class `ResolvedFormType`, which caches the parent type, the type's extensions and its default options. The updated responsibilities: `FormFactory` is a registry and proxy for `ResolvedFormType` objects, `FormType` specifies how a form can be built on a specific layer of the type hierarchy (e.g. "form", or "date", etc.) and `ResolvedFormType` *does the actual building* across all layers of the hierarchy (by delegating to the parent type, which delegates to its parent type etc.). --------------------------------------------------------------------------- by schmittjoh at 2012-07-12T18:25:40Z Maybe ResolvedFormType --------------------------------------------------------------------------- by jmather at 2012-07-13T02:56:38Z I really like ResolvedFormType. That's the naming method I took for my tag parser that handes the same conceptual issue. --------------------------------------------------------------------------- by axelarge at 2012-07-13T05:25:00Z ResolvedFormType sounds very clear. This change is great and I desperately hope to see more of this kind --------------------------------------------------------------------------- by Baachi at 2012-07-13T06:41:26Z Yes `ResolvedFormType` sounds good :) :+1: --------------------------------------------------------------------------- by fabpot at 2012-07-13T07:11:33Z I like `ResolvedFormType` as well. --------------------------------------------------------------------------- by henrikbjorn at 2012-07-13T07:46:48Z :+1: `ResolvedFormType` :shipit: --------------------------------------------------------------------------- by stof at 2012-07-13T18:01:51Z This looks good to me
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| public function resolveType(FormTypeInterface $type) |
There was a problem hiding this comment.
IMO resolveType shouldn't be part of the interface and the logic should be included in addType.
Resolving a type but not adding it to the registry that you called it on makes no sense, does it?
With just addType and getType, it is more clear.
There was a problem hiding this comment.
I agree that this interface probably isn't the best place, but I didn't want to add a FormTypeResolverInterface just for that simple purpose. It definitely doesn't belong into addType, since adding and getting should be synchronous. If you add A, you want to get back A, not B.
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: Update the Silex extension
This PR is work in progress and up for discussion. It increases the performance of FormFactory::createForm() on a specific, heavy-weight form from 0.848 to 0.580 seconds.
Before, the FormFactory had to traverse the hierarchy and calculate the default options of each FormType everytime a form was created of that type.
Now, FormTypes are wrapped within instances of a new class
ResolvedFormType, which caches the parent type, the type's extensions and its default options.The updated responsibilities:
FormFactoryis a registry and proxy forResolvedFormTypeobjects,FormTypespecifies how a form can be built on a specific layer of the type hierarchy (e.g. "form", or "date", etc.) andResolvedFormTypedoes the actual building across all layers of the hierarchy (by delegating to the parent type, which delegates to its parent type etc.).