Skip to content

[Form] Added support for caching choice lists based on options #30994

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 21, 2020

Conversation

HeahDude
Copy link
Contributor

@HeahDude HeahDude commented Apr 7, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets ~
License MIT
Doc PR symfony/symfony-docs#13182

Currently, the CachingFactoryDecorator is responsible for unnecessary memory usage, anytime a choice option is set with a callback option defined as an anonymous function or a loader, then a new hash is generated for the choice list, while we may expect the list to be reused once "finally" configured in a form type or choice type extension.

A simple case is when using one of the core intl choice types in a collection:

// ...
class SomeFormType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder
            ->add('some_choices', ChoiceType::class, [
                // before: cached choice list (unnecessary overhead)
                // after: no cache (better perf)
                'choices' => $someObjects,
                'choice_value' => function (?object $choice) { /* return some string */ },
            ])

            // see below the nested effects
            ->add('nested_fields', CollectionType::class, [
                'entry_type' => NestedFormType::class,
            ])
    // ...
}

// ...
class NestedFormType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder
            // ...
            ->add('some_other_choices', ChoiceType::class, [
                // before: cached choice list for every entry because we define a new closure instance for each field
                // after: no cache, a bit better for the same result, but much better if it were not nested in a collection
                'choices' => $someOtherObjects,
                'choice_value' => function (?object $otherChoice) { /* return some string */ },
            ])

            ->add('some_loaded_choices', ChoiceType::class, [
                // before: cached but for every entry since every field will have its
                //         own instance of loader, generating a new hash 
                // after: no cache, same pro/cons as above
                'choice_loader' => new CallbackChoiceLoader(function() { /* return some choices */}),
                // or 
                'choice_loader' => new SomeLoader(),
            ])

            ->add('person', EntityType::class, [
                // before: cached but for every entry, because we define extra `choice_*` option
                // after: no cache, same pro/cons as above
                'class' => SomeEntity::class,
                'choice_label' => function (?SomeEntity $choice) { /* return some label */},
            ])

            // before: cached for every entry, because the type define some "choice_*" option
            // after: cached only once, better perf since the same loader is used for every entry
            ->add('country', CountryType::class)

            // before: cached for every entry, because the type define some "choice_*" option
            // after: no cache, same pro/cons as above
            ->add('locale', LocaleType::class, [
                'preferred_choices' => [ /* some preferred locales */ ],
                'group_by' => function (?string $locale, $label) { /* return some group */ },
            ])
// ...

In such cases, we would expect every entries to use the same cached intl choice list, but not, as many list and views as entries will be kept in cache. This is even worse if some callback options like choice_label, choice_value, choice_attr, choice_name, preferred_choices or group_by are used.
This PR helps making cache explicit when needed and deprecate drop unexpected implicit caching of choice list for most simple cases responsible of unnecessary overhead.

The result is better performance just by upgrading to 5.1 \o/.
But to solve the cases above when cache is needed per options, one should now use the new ChoiceList static methods to wrap option values, which is already done internally in this PR.

use Symfony\Component\Form\ChoiceList\ChoiceList;
// ...
class NestedFormType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder
            // explicitly shared cached choice lists between entries

            ->add('some_other_choices', ChoiceType::class, [
                'choices' => $someOtherObjects,
                'choice_value' => ChoiceList::value($this, function (?object $otherChoice) {
                    /* return some string */ 
                }),
            ]),

            ->add('some_loaded_choices', ChoiceType::class, [
                'choice_loader' => ChoiceList::lazy($this, function() {
                    /* return some choices */
                })),
                // or
                'choice_loader' => ChoiceList::loader($this, new SomeLoader()),
            ]),

            ->add('person', EntityType::class, [
                'class' => SomeEntity::class,
                'choice_label' => ChoiceList::label($this, function (?SomeEntity $choice) {
                    /* return some label */
                },
            ])

            // nothing to do :)
            ->add('country', CountryType::class)

            ->add('locale', LocaleType::class, [
                'preferred_choices' => ChoiceList::preferred($this, [ /* some preferred locales */ ]),
                'group_by' => ChoiceList::groupBy($this, function (?string $locale, $label) {
                    /* return some group */
                }),
            ])
// ...

I've done some nice profiling with Blackfire and the simple example above in a fresh website skeleton and only two empty entries as initial data, then submitting an empty form. That gives the following results:

  • Rendering the form - Before vs After

Screenshot 2020-02-16 at 9 24 58 PM

  • Rendering the form - Before vs After with ChoiceList helpers

Screenshot 2020-02-16 at 9 26 51 PM

  • Submitting the form - Before vs After

Screenshot 2020-02-16 at 9 28 01 PM

  • Submitting the form - Before vs After with ChoiceList helpers

Screenshot 2020-02-16 at 9 29 10 PM


TODO:

  • Docs
  • More profiling
  • Add some tests

#EUFOSSA

@fabpot
Copy link
Member

fabpot commented Jul 8, 2019

@HeahDude What's the status of this PR? Are you willing to finish it?

@HeahDude HeahDude force-pushed the form/choice_lists-caching branch from 81ce543 to dd39486 Compare November 23, 2019 15:38
@HeahDude HeahDude requested a review from xabbuh as a code owner November 23, 2019 15:38
@HeahDude HeahDude force-pushed the form/choice_lists-caching branch 3 times, most recently from 12b57e8 to 8b517e0 Compare November 24, 2019 20:07
@HeahDude
Copy link
Contributor Author

This one is ready for another round of reviews.

@HeahDude HeahDude force-pushed the form/choice_lists-caching branch 3 times, most recently from c379ca7 to b8ff42d Compare November 25, 2019 17:56
@HeahDude HeahDude force-pushed the form/choice_lists-caching branch 2 times, most recently from c09ad47 to 2e0116e Compare November 27, 2019 19:49
@HeahDude HeahDude force-pushed the form/choice_lists-caching branch 4 times, most recently from b29cf28 to f4a2b42 Compare December 7, 2019 20:12
@HeahDude HeahDude force-pushed the form/choice_lists-caching branch from bbb4797 to b25973c Compare February 16, 2020 15:50
*
* @author Jules Pietri <jules@heahprod.com>
*/
final class ChoiceList
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I see the point of this class. I'd suggest removing it and inlining the instantiations instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the user land part of this feature, everything else is about optimising the internals, many advantages to me:

  • easier to document/learn, only one class instead of many (I also add the ChoiceFilter in [Form] Added a "choice_filter" option to ChoiceType #35733 and we may add more in the future like a ChoiceLabelAttr I'got locally)

  • better discovery since every factory method has its own specific phpdoc instead of the generic constructor in the abstract class

  • better auto complete, there are a lot of classes using the Choice word already:
    Screenshot 2020-02-17 at 7 08 15 PM

    VS

    Screenshot 2020-02-17 at 7 06 07 PM

I agree we could inline in our own usage in this PR to save a few static calls, but I wouldn't either since the types are some kind of documentation and given the Blackfire profiles I got with the current implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can see this class as a ChoiceList builder or configurator

* @param mixed $option Any pseudo callable, array, string or bool to define a choice list option
* @param mixed|null $vary Dynamic data used to compute a unique hash when caching the option
*/
final public function __construct($formType, $option, $vary = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why final?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not? This class is responsible for clearing the cache of all options, they should not override/extends it. I see no use case currently, we have everything we need here with those three args.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a design needed by the CachingFactoryDecorator no matter the option.

/**
* @return mixed
*/
final public function getOption()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd remove the final keywords - @internal is enough to express our intend, no need to be extreme

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The property is private and there is no need for extension.

@@ -139,5 +197,6 @@ public function reset()
{
$this->lists = [];
$this->views = [];
Cache\AbstractStaticOption::reset();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? if this is static, it shouldn't need to be reset between requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's consistent with the lists and the views, the options used to build them may be defined per request

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks for your answers :)

@HeahDude
Copy link
Contributor Author

Thank you for the review <3. I've updated the description to better explain the common scenarii and the after/before effects.
I've also done more profiling and added some nice screenshots :).
Docs PR is coming!

@HeahDude
Copy link
Contributor Author

Doc PR opened symfony/symfony-docs#13182.

@fabpot
Copy link
Member

fabpot commented Feb 21, 2020

Thank you @HeahDude.

@fabpot fabpot merged commit 269c4a2 into symfony:master Feb 21, 2020
@HeahDude HeahDude deleted the form/choice_lists-caching branch February 21, 2020 16:51
fabpot added a commit that referenced this pull request Mar 16, 2020
…eahDude)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[Form] Added a "choice_filter" option to ChoiceType

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | yes
| Tickets       | Fix #32657
| License       | MIT
| Doc PR        | symfony/symfony-docs#13223

Finally opening this PR for a very old branch, based on both #34550 (merged) and #30994 (merged).

~Until #30994 is merged, this PR should better be reviewed by commits. Thanks!~

Commits
-------

ed2c312 [Form] Added a "choice_filter" option to ChoiceType
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Mar 24, 2020
…oiceType` options (HeahDude)

This PR was squashed before being merged into the master branch (closes #13182).

Discussion
----------

[Form] added the new `ChoiceList` class to configure `ChoiceType` options

Documentation for symfony/symfony#30994.

Commits
-------

9fc18e7 [Form] added the new `ChoiceList` class to configure `ChoiceType` options
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@fabpot fabpot mentioned this pull request May 5, 2020
xabbuh added a commit that referenced this pull request May 20, 2022
… (HeahDude)

This PR was squashed before being merged into the 4.4 branch.

Discussion
----------

[Form] Fix same choice loader with different choice values

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | #44655
| License       | MIT
| Doc PR        | ~

It appears that deprecating the caching in the `LazyChoiceList` (cf #18359) was a mistake. The bug went under the radar because in practice every choice field has its own loader instance.
However, optimizations made in #30994 then revealed the flaw (cf #42206) as the loaders were actually shared across many fields.
While working on a fix I ended up implementing something similar to what's proposed in #44655.

I'll send a PR for 5.4 as well.

Commits
-------

65cbf18 [Form] Fix same choice loader with different choice values
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants