Problem/Motivation
ElementInfoManager::buildInfo() gets cached plugin definitions and processes all plugins by creating instances, checking for FormElementInterface
, adding '#type' to each item, and invoking hook_element_info_alter()
.
Historically this was considered to be pretty lightweight (?) so running on every page was not the end of the world, as well as allowing themes to alter element_info. However, themes cannot do this anymore as ModuleHandler->alter() is not going to invoke anything in a theme.
Proposed resolution
Cache the processed result and return that from ElementInfoManager::buildInfo(). Keep two layers of caching, plugin definitions, and full altered element info.
From some profiling, doing this processing takes ~20ms - and that's running on every request!
Here is the difference between a cached and uncached run of that element info:
Overall Summary | |
---|---|
Total Incl. Wall Time (microsec): | 21,439 microsecs |
Total Incl. CPU (microsecs): | 21,430 microsecs |
Total Incl. MemUse (bytes): | 2,578,648 bytes |
Total Incl. PeakMemUse (bytes): | 2,635,856 bytes |
Number of Function Calls: | 2,224 |
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#33 | interdiff-2368975-33.txt | 679 bytes | damiankloip |
#33 | 2368975-33.patch | 4.6 KB | damiankloip |
#28 | interdiff-2368975-28.txt | 450 bytes | damiankloip |
#28 | 2368975-28.patch | 4.64 KB | damiankloip |
#1 | 2368975.patch | 1.93 KB | damiankloip |
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedHere is the patch to add the cache layer in.
Comment #2
damiankloip CreditAttribution: damiankloip commentedComment #6
damiankloip CreditAttribution: damiankloip commentedLet's see how this gets on instead.
Basically, this worked before when it was running on every page:
Which to me seems like a definite hack :) I have moved it directly into the FormBuilder, we could add a process callback if we wanted to?
Comment #8
catchGiven we have a plugin in core that's making use of the lack of caching, and 46ms looks to be quite a big regression from 7.x, bumping to critical.
Comment #9
damiankloip CreditAttribution: damiankloip commentedLet's try this. Should fix form unit tests.
Comment #11
dawehnerNote: We should make it clear in
\Drupal\Core\Render\Element\ElementInterface::getInfo()
that this hook is not executed on runtime.
In general I wonder whether we had that kind of caching back in d7? I guess it was less problematic back then, because initializing
hooks is much easier than initializing a plugin.
Yeah doing anything dynamic is a bad idea, that is true.
Comment #12
damiankloip CreditAttribution: damiankloip commentedWhat would make this clearer would be if it was a static method IMO. Elements is really not a good fit for plugins, but anyway...
Comment #13
martin107 CreditAttribution: martin107 commentedLocally I cannot reproduce the fails in FilterHtmlImageSecureTest or ElementInfoManagerTest !!! ( via browser testing ).
So I am about to restest.
Not sure what in head may have changed.
Comment #16
damiankloip CreditAttribution: damiankloip commentedYes, thanks for testings. I think these are probably test env specific issues :/ Looking in to.
Comment #17
martin107 CreditAttribution: martin107 commentedFWIW - I was using php5.510 - when those tests passed.
Comment #18
dawehnerGood point.
Comment #19
damiankloip CreditAttribution: damiankloip commentedLet's try this.
Comment #20
damiankloip CreditAttribution: damiankloip commentedComment #21
damiankloip CreditAttribution: damiankloip commentedSorry, initial benchmarks were off a bit I think. It's more like ~20ms. Update IS.
Comment #22
damiankloip CreditAttribution: damiankloip commentedComment #23
jibranTBH I don't like this change. Can we ask form maintainer for the opinion?
Comment #24
catchWe shouldn't set it dynamically.
Do you mean don't both initializing rather than initializing to NULL? If so might as well leave it unitialized.
Comment #25
damiankloip CreditAttribution: damiankloip commentedJibran, you prefer a hack in the form element plugin instead?
The form builder should definitely take control of the action for the form. Not an element plugin :)
Comment #26
jibran@damiankloip no. It is in prepareForm it should be ok. Can we remove the NULL initialization? Thanks for the explanation.
Comment #27
damiankloip CreditAttribution: damiankloip commentedYeah. I am OK with that, and it seems catch is too.
Rerolling :)
Comment #28
damiankloip CreditAttribution: damiankloip commentedComment #29
dawehnerI think this is perfect
Comment #30
tim.plunkett+1
Comment #31
damiankloip CreditAttribution: damiankloip commentedGreat!
Comment #32
alexpottThe removal of the createInstance method is an out-of-scope unrelated change.
Comment #33
damiankloip CreditAttribution: damiankloip commentedok, here we go. Created #2371811: remove createInstance() method form ElementInfoManager for that.
Comment #34
jibranBack to RTBC.
Comment #35
alexpottThis issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed b0c49c8 and pushed to 8.0.x. Thanks!