-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Conversation
src/Symfony/Component/Form/ChoiceList/Factory/CachingFactoryDecorator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Form/ChoiceList/Factory/Cache/AbstractStaticCallback.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Form/ChoiceList/Factory/Cache/AbstractStaticCallback.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Form/ChoiceList/Factory/Cache/ChoiceLoader.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Form/ChoiceList/Factory/Cache/ChoiceLoader.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Form/ChoiceList/Factory/Cache/ChoiceLoader.php
Outdated
Show resolved
Hide resolved
@HeahDude What's the status of this PR? Are you willing to finish it? |
81ce543
to
dd39486
Compare
12b57e8
to
8b517e0
Compare
This one is ready for another round of reviews. |
c379ca7
to
b8ff42d
Compare
c09ad47
to
2e0116e
Compare
b29cf28
to
f4a2b42
Compare
db73bca
to
89a4b7c
Compare
89a4b7c
to
bbb4797
Compare
bbb4797
to
b25973c
Compare
* | ||
* @author Jules Pietri <jules@heahprod.com> | ||
*/ | ||
final class ChoiceList |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 aChoiceLabelAttr
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:
VS
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why final?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this 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 :)
Thank you for the review <3. I've updated the description to better explain the common scenarii and the after/before effects. |
Doc PR opened symfony/symfony-docs#13182. |
Thank you @HeahDude. |
…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
…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
… (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
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:
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
orgroup_by
are used.This PR helps making cache explicit when needed and
deprecatedrop 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.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:
ChoiceList
helpersChoiceList
helpersTODO:
#EUFOSSA