Skip to content

Commit 019625d

Browse files
fabpotfabpot
committed
merged branch bschussek/options_performance (PR #5004)
Commits ------- 24b764e [Form] Fixed issues mentioned in the PR 9216816 [Form] Turned Twig filters into tests 310f985 [Form] Added a layer of 2.0 BC methods to FormView and updated UPGRADE and CHANGELOG 5984b18 [Form] Precalculated the closure for deciding whether a choice is selected (PHP +30ms, Twig +30ms) 5dc3c39 [Form] Moved the access to templating helpers out of the choice loop for performance reasons (PHP +100ms) 0ef9acb [Form] Moved the method isChoiceSelected() to the ChoiceView class (PHP +150ms) 8b72766 [Form] Tweaked the generation of option tags for performance (PHP +200ms, Twig +50ms) 400c95b [Form] Replace methods in ChoiceView by public properties (PHP +100ms, Twig +400ms) d072f35 [Form] The properties of FormView are now accessed directly in order to increase performance (PHP +200ms, Twig +150ms) Discussion ---------- [Form] Made FormView and ChoiceView properties public for performance reasons Bug fix: no Feature addition: no Backwards compatibility break: **yes** Symfony2 tests pass: yes Fixes the following tickets: - Todo: - This PR changes the access to properties of `FormView` and `ChoiceView` objects from getters to direct property accesses. On [my example form](http://advancedform.gpserver.dk/app_dev.php/taxclasses/1) this improves rendering performance for **300ms** with PHP templates and **550ms** with Twig on my local machine. Unfortunately, this breaks BC both with 2.0 and with the current master in Form Types and PHP templates. Twig templates are not affected by this change. 2.0: ``` $formView->set('my_var', 'foobar'); $formView->get('my_var'); $formView->getChild('childName'); $formView['childName']; ``` master: ``` $formView->setVar('my_var', 'foobar'); $formView->getVar('my_var'); $formView->get('childName'); $formView['childName']; ``` this PR: ``` $formView->vars['my_var'] = 'foobar'; $formView->vars['my_var']; $formView->children['childName']; $formView['childName']; ``` Should we add methods to keep BC with 2.0? The second part of this PR contains improvements to the rendering of choice fields. These gain another **~500ms** for PHP templates and **80ms** for Twig. These improvements are BC, unless you overwrote the block "choice_widget_options" in your form themes which then needs to be adapted. **Update:** The PR now includes a BC layer for 2.0. --------------------------------------------------------------------------- by stof at 2012-07-21T11:37:41Z @bschussek couldn't we keep the getters and setters for BC even if the rendering accesses the public properties directly ? --------------------------------------------------------------------------- by bschussek at 2012-07-21T11:52:33Z @stof A BC layer for 2.0 is now included. People who upgraded to master already unfortunately need to adapt their code. --------------------------------------------------------------------------- by sstok at 2012-07-21T12:40:57Z :+1:
2 parents 6c256b0 + 24b764e commit 019625d

File tree

63 files changed

+703
-871
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

63 files changed

+703
-871
lines changed

‎UPGRADE-2.1.md

Lines changed: 63 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -224,30 +224,23 @@
224224
`buildViewBottomUp()` in `FormTypeInterface` and `FormTypeExtensionInterface`.
225225
Furthermore, `buildViewBottomUp()` was renamed to `finishView()`. At last,
226226
all methods in these types now receive instances of `FormBuilderInterface`
227-
and `FormViewInterface` where they received instances of `FormBuilder` and
228-
`FormView` before. You need to change the method signatures in your
229-
form types and extensions as shown below.
227+
where they received instances of `FormBuilder` before. You need to change the
228+
method signatures in your form types and extensions as shown below.
230229
231230
Before:
232231
233232
```
234233
use Symfony\Component\Form\FormBuilder;
235-
use Symfony\Component\Form\FormView;
236234
237235
public function buildForm(FormBuilder $builder, array $options)
238-
public function buildView(FormView $view, FormInterface $form)
239-
public function buildViewBottomUp(FormView $view, FormInterface $form)
240236
```
241237
242238
After:
243239
244240
```
245241
use Symfony\Component\Form\FormBuilderInterface;
246-
use Symfony\Component\Form\FormViewInterface;
247242
248243
public function buildForm(FormBuilderInterface $builder, array $options)
249-
public function buildView(FormViewInterface $view, FormInterface $form, array $options)
250-
public function finishView(FormViewInterface $view, FormInterface $form, array $options)
251244
```
252245
253246
* The method `createBuilder` was removed from `FormTypeInterface` for performance
@@ -383,41 +376,6 @@
383376
If address is an object in this case, the code given in "Before"
384377
works without changes.
385378
386-
* The methods in class `FormView` were renamed to match the naming used in
387-
`Form` and `FormBuilder`. The following list shows the old names on the
388-
left and the new names on the right:
389-
390-
* `set`: `setVar`
391-
* `has`: `hasVar`
392-
* `get`: `getVar`
393-
* `all`: `getVars`
394-
* `addChild`: `add`
395-
* `getChild`: `get`
396-
* `getChildren`: `all`
397-
* `removeChild`: `remove`
398-
* `hasChild`: `has`
399-
400-
The new method `addVars` was added to make the definition of multiple
401-
variables at once more convenient.
402-
403-
The method `hasChildren` was deprecated. You should use `count` instead.
404-
405-
Before:
406-
407-
```
408-
$view->set('help', 'A text longer than six characters');
409-
$view->set('error_class', 'max_length_error');
410-
```
411-
412-
After:
413-
414-
```
415-
$view->addVars(array(
416-
'help' => 'A text longer than six characters',
417-
'error_class' => 'max_length_error',
418-
));
419-
```
420-
421379
* Form and field names must now start with a letter, digit or underscore
422380
and only contain letters, digits, underscores, hyphens and colons.
423381
@@ -1069,6 +1027,67 @@
10691027
<?php echo $view['form']->block('widget_attributes') ?>
10701028
```
10711029
1030+
* The following methods in class `FormView` were deprecated and will be
1031+
removed in Symfony 2.3:
1032+
1033+
* `set`
1034+
* `has`
1035+
* `get`
1036+
* `all`
1037+
* `getVars`
1038+
* `addChild`
1039+
* `getChild`
1040+
* `getChildren`
1041+
* `removeChild`
1042+
* `hasChild`
1043+
* `hasChildren`
1044+
* `getParent`
1045+
* `hasParent`
1046+
* `setParent`
1047+
1048+
You should access the public properties `vars`, `children` and `parent`
1049+
instead.
1050+
1051+
Before:
1052+
1053+
```
1054+
$view->set('help', 'A text longer than six characters');
1055+
$view->set('error_class', 'max_length_error');
1056+
```
1057+
1058+
After:
1059+
1060+
```
1061+
$view->vars = array_replace($view->vars, array(
1062+
'help' => 'A text longer than six characters',
1063+
'error_class' => 'max_length_error',
1064+
));
1065+
```
1066+
1067+
Before:
1068+
1069+
```
1070+
echo $view->get('error_class');
1071+
```
1072+
1073+
After:
1074+
1075+
```
1076+
echo $view->vars['error_class'];
1077+
```
1078+
1079+
Before:
1080+
1081+
```
1082+
if ($view->hasChildren()) { ...
1083+
```
1084+
1085+
After:
1086+
1087+
```
1088+
if (count($view->children)) { ...
1089+
```
1090+
10721091
### Validator
10731092
10741093
* The methods `setMessage()`, `getMessageTemplate()` and

‎src/Symfony/Bridge/Doctrine/Tests/Form/Type/EntityTypeTest.php

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ public function testSetDataToUninitializedEntityWithNonRequired()
124124
'property' => 'name'
125125
));
126126

127-
$this->assertEquals(array(1 => new ChoiceView('1', 'Foo'), 2 => new ChoiceView('2', 'Bar')), $field->createView()->getVar('choices'));
127+
$this->assertEquals(array(1 => new ChoiceView('1', 'Foo'), 2 => new ChoiceView('2', 'Bar')), $field->createView()->vars['choices']);
128128
}
129129

130130
public function testSetDataToUninitializedEntityWithNonRequiredToString()
@@ -140,7 +140,7 @@ public function testSetDataToUninitializedEntityWithNonRequiredToString()
140140
'required' => false,
141141
));
142142

143-
$this->assertEquals(array(1 => new ChoiceView('1', 'Foo'), 2 => new ChoiceView('2', 'Bar')), $field->createView()->getVar('choices'));
143+
$this->assertEquals(array(1 => new ChoiceView('1', 'Foo'), 2 => new ChoiceView('2', 'Bar')), $field->createView()->vars['choices']);
144144
}
145145

146146
public function testSetDataToUninitializedEntityWithNonRequiredQueryBuilder()
@@ -159,7 +159,7 @@ public function testSetDataToUninitializedEntityWithNonRequiredQueryBuilder()
159159
'query_builder' => $qb
160160
));
161161

162-
$this->assertEquals(array(1 => new ChoiceView('1', 'Foo'), 2 => new ChoiceView('2', 'Bar')), $field->createView()->getVar('choices'));
162+
$this->assertEquals(array(1 => new ChoiceView('1', 'Foo'), 2 => new ChoiceView('2', 'Bar')), $field->createView()->vars['choices']);
163163
}
164164

165165
/**
@@ -503,7 +503,7 @@ public function testOverrideChoices()
503503

504504
$field->bind('2');
505505

506-
$this->assertEquals(array(1 => new ChoiceView('1', 'Foo'), 2 => new ChoiceView('2', 'Bar')), $field->createView()->getVar('choices'));
506+
$this->assertEquals(array(1 => new ChoiceView('1', 'Foo'), 2 => new ChoiceView('2', 'Bar')), $field->createView()->vars['choices']);
507507
$this->assertTrue($field->isSynchronized());
508508
$this->assertSame($entity2, $field->getData());
509509
$this->assertSame('2', $field->getClientData());
@@ -533,7 +533,7 @@ public function testGroupByChoices()
533533
'Group1' => array(1 => new ChoiceView('1', 'Foo'), 2 => new ChoiceView('2', 'Bar')),
534534
'Group2' => array(3 => new ChoiceView('3', 'Baz')),
535535
'4' => new ChoiceView('4', 'Boo!')
536-
), $field->createView()->getVar('choices'));
536+
), $field->createView()->vars['choices']);
537537
}
538538

539539
public function testDisallowChoicesThatAreNotIncluded_choicesSingleIdentifier()

‎src/Symfony/Bridge/Twig/Extension/FormExtension.php

Lines changed: 50 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313

1414
use Symfony\Bridge\Twig\TokenParser\FormThemeTokenParser;
1515
use Symfony\Bridge\Twig\Form\TwigRendererInterface;
16-
use Symfony\Component\Form\FormViewInterface;
16+
use Symfony\Component\Form\Extension\Core\View\ChoiceView;
1717
use Symfony\Component\Form\Extension\Csrf\CsrfProvider\CsrfProviderInterface;
1818

1919
/**
@@ -62,15 +62,13 @@ public function getTokenParsers()
6262
public function getFunctions()
6363
{
6464
return array(
65-
'form_enctype' => new \Twig_Function_Method($this, 'renderer->renderEnctype', array('is_safe' => array('html'))),
66-
'form_widget' => new \Twig_Function_Method($this, 'renderer->renderWidget', array('is_safe' => array('html'))),
67-
'form_errors' => new \Twig_Function_Method($this, 'renderer->renderErrors', array('is_safe' => array('html'))),
68-
'form_label' => new \Twig_Function_Method($this, 'renderer->renderLabel', array('is_safe' => array('html'))),
69-
'form_row' => new \Twig_Function_Method($this, 'renderer->renderRow', array('is_safe' => array('html'))),
70-
'form_rest' => new \Twig_Function_Method($this, 'renderer->renderRest', array('is_safe' => array('html'))),
71-
'csrf_token' => new \Twig_Function_Method($this, 'renderer->renderCsrfToken'),
72-
'_form_is_choice_group' => new \Twig_Function_Method($this, 'renderer->isChoiceGroup', array('is_safe' => array('html'))),
73-
'_form_is_choice_selected' => new \Twig_Function_Method($this, 'renderer->isChoiceSelected', array('is_safe' => array('html'))),
65+
'form_enctype' => new \Twig_Function_Method($this, 'renderer->renderEnctype', array('is_safe' => array('html'))),
66+
'form_widget' => new \Twig_Function_Method($this, 'renderer->renderWidget', array('is_safe' => array('html'))),
67+
'form_errors' => new \Twig_Function_Method($this, 'renderer->renderErrors', array('is_safe' => array('html'))),
68+
'form_label' => new \Twig_Function_Method($this, 'renderer->renderLabel', array('is_safe' => array('html'))),
69+
'form_row' => new \Twig_Function_Method($this, 'renderer->renderRow', array('is_safe' => array('html'))),
70+
'form_rest' => new \Twig_Function_Method($this, 'renderer->renderRest', array('is_safe' => array('html'))),
71+
'csrf_token' => new \Twig_Function_Method($this, 'renderer->renderCsrfToken'),
7472
);
7573
}
7674

@@ -84,6 +82,48 @@ public function getFilters()
8482
);
8583
}
8684

85+
/**
86+
* {@inheritdoc}
87+
*/
88+
public function getTests()
89+
{
90+
return array(
91+
'selectedchoice' => new \Twig_Test_Method($this, 'isSelectedChoice'),
92+
);
93+
}
94+
95+
/**
96+
* Returns whether a choice is selected for a given form value.
97+
*
98+
* Unfortunately Twig does not support an efficient way to execute the
99+
* "is_selected" closure passed to the template by ChoiceType. It is faster
100+
* to implement the logic here (around 65ms for a specific form).
101+
*
102+
* Directly implementing the logic here is also faster than doing so in
103+
* ChoiceView (around 30ms).
104+
*
105+
* The worst option tested so far is to implement the logic in ChoiceView
106+
* and access the ChoiceView method directly in the template. Doing so is
107+
* around 220ms slower than doing the method call here in the filter. Twig
108+
* seems to be much more efficient at executing filters than at executing
109+
* methods of an object.
110+
*
111+
* @param ChoiceView $choice The choice to check.
112+
* @param string|array $selectedValue The selected value to compare.
113+
*
114+
* @return Boolean Whether the choice is selected.
115+
*
116+
* @see ChoiceView::isSelected()
117+
*/
118+
public function isSelectedChoice(ChoiceView $choice, $selectedValue)
119+
{
120+
if (is_array($selectedValue)) {
121+
return false !== array_search($choice->value, $selectedValue, true);
122+
}
123+
124+
return $choice->value === $selectedValue;
125+
}
126+
87127
/**
88128
* {@inheritdoc}
89129
*/

‎src/Symfony/Bridge/Twig/Form/TwigRendererEngine.php

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
namespace Symfony\Bridge\Twig\Form;
1313

1414
use Symfony\Component\Form\AbstractRendererEngine;
15-
use Symfony\Component\Form\FormViewInterface;
15+
use Symfony\Component\Form\FormView;
1616

1717
/**
1818
* @author Bernhard Schussek <bschussek@gmail.com>
@@ -40,9 +40,9 @@ public function setEnvironment(\Twig_Environment $environment)
4040
/**
4141
* {@inheritdoc}
4242
*/
43-
public function renderBlock(FormViewInterface $view, $resource, $block, array $variables = array())
43+
public function renderBlock(FormView $view, $resource, $block, array $variables = array())
4444
{
45-
$cacheKey = $view->getVar(self::CACHE_KEY_VAR);
45+
$cacheKey = $view->vars[self::CACHE_KEY_VAR];
4646

4747
$context = $this->environment->mergeGlobals($variables);
4848

@@ -71,12 +71,12 @@ public function renderBlock(FormViewInterface $view, $resource, $block, array $v
7171
* @see getResourceForBlock()
7272
*
7373
* @param string $cacheKey The cache key of the form view.
74-
* @param FormViewInterface $view The form view for finding the applying themes.
74+
* @param FormView $view The form view for finding the applying themes.
7575
* @param string $block The name of the block to load.
7676
*
7777
* @return Boolean True if the resource could be loaded, false otherwise.
7878
*/
79-
protected function loadResourceForBlock($cacheKey, FormViewInterface $view, $block)
79+
protected function loadResourceForBlock($cacheKey, FormView $view, $block)
8080
{
8181
// The caller guarantees that $this->resources[$cacheKey][$block] is
8282
// not set, but it doesn't have to check whether $this->resources[$cacheKey]
@@ -105,19 +105,19 @@ protected function loadResourceForBlock($cacheKey, FormViewInterface $view, $blo
105105
}
106106

107107
// Check the default themes once we reach the root view without success
108-
if (!$view->hasParent()) {
108+
if (!$view->parent) {
109109
for ($i = count($this->defaultThemes) - 1; $i >= 0; --$i) {
110110
$this->loadResourcesFromTheme($cacheKey, $this->defaultThemes[$i]);
111111
// CONTINUE LOADING (see doc comment)
112112
}
113113
}
114114

115115
// Proceed with the themes of the parent view
116-
if ($view->hasParent()) {
117-
$parentCacheKey = $view->getParent()->getVar(self::CACHE_KEY_VAR);
116+
if ($view->parent) {
117+
$parentCacheKey = $view->parent->vars[self::CACHE_KEY_VAR];
118118

119119
if (!isset($this->resources[$parentCacheKey])) {
120-
$this->loadResourceForBlock($parentCacheKey, $view->getParent(), $block);
120+
$this->loadResourceForBlock($parentCacheKey, $view->parent, $block);
121121
}
122122

123123
// EAGER CACHE POPULATION (see doc comment)

‎src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -86,15 +86,14 @@
8686

8787
{% block choice_widget_options %}
8888
{% spaceless %}
89-
{% for index, choice in options %}
90-
{% if _form_is_choice_group(choice) %}
91-
<optgroup label="{{ index|trans({}, translation_domain) }}">
92-
{% for nested_choice in choice %}
93-
<option value="{{ nested_choice.value }}"{% if _form_is_choice_selected(form, nested_choice) %} selected="selected"{% endif %}>{{ nested_choice.label|trans({}, translation_domain) }}</option>
94-
{% endfor %}
89+
{% for group_label, choice in options %}
90+
{% if choice is iterable %}
91+
<optgroup label="{{ group_label|trans({}, translation_domain) }}">
92+
{% set options = choice %}
93+
{{ block('choice_widget_options') }}
9594
</optgroup>
9695
{% else %}
97-
<option value="{{ choice.value }}"{% if _form_is_choice_selected(form, choice) %} selected="selected"{% endif %}>{{ choice.label|trans({}, translation_domain) }}</option>
96+
<option value="{{ choice.value }}"{% if choice is selectedchoice(value) %} selected="selected"{% endif %}>{{ choice.label|trans({}, translation_domain) }}</option>
9897
{% endif %}
9998
{% endfor %}
10099
{% endspaceless %}

‎src/Symfony/Bridge/Twig/Tests/Extension/FormExtensionDivLayoutTest.php

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
use Symfony\Bridge\Twig\Tests\Extension\Fixtures\StubTranslator;
1919
use Symfony\Bridge\Twig\Tests\Extension\Fixtures\StubFilesystemLoader;
2020
use Symfony\Component\Form\FormView;
21+
use Symfony\Component\Form\Extension\Core\View\ChoiceView;
2122
use Symfony\Component\Form\Tests\AbstractDivLayoutTest;
2223

2324
class FormExtensionDivLayoutTest extends AbstractDivLayoutTest
@@ -105,6 +106,39 @@ public function testThemeBlockInheritanceUsingExtend()
105106
);
106107
}
107108

109+
public function isSelectedChoiceProvider()
110+
{
111+
// The commented cases should not be necessary anymore, because the
112+
// choice lists should assure that both values passed here are always
113+
// strings
114+
return array(
115+
// array(true, 0, 0),
116+
array(true, '0', '0'),
117+
array(true, '1', '1'),
118+
// array(true, false, 0),
119+
// array(true, true, 1),
120+
array(true, '', ''),
121+
// array(true, null, ''),
122+
array(true, '1.23', '1.23'),
123+
array(true, 'foo', 'foo'),
124+
array(true, 'foo10', 'foo10'),
125+
array(true, 'foo', array(1, 'foo', 'foo10')),
126+
127+
array(false, 10, array(1, 'foo', 'foo10')),
128+
array(false, 0, array(1, 'foo', 'foo10')),
129+
);
130+
}
131+
132+
/**
133+
* @dataProvider isSelectedChoiceProvider
134+
*/
135+
public function testIsChoiceSelected($expected, $choice, $value)
136+
{
137+
$choice = new ChoiceView($choice, $choice . ' label');
138+
139+
$this->assertSame($expected, $this->extension->isSelectedChoice($choice, $value));
140+
}
141+
108142
protected function renderEnctype(FormView $view)
109143
{
110144
return (string) $this->extension->renderer->renderEnctype($view);

0 commit comments

Comments
 (0)