Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Theme registry icreates the cache on the construct which is very early. Cache record should be created instead when the results are actually being needed.
Proposed resolution
Create getter methods for all the parameters that are wanted to cache so that they can implement logic for cache misses.
Remaining tasks
-
User interface changes
-
API changes
-
Beta phase evaluation
Issue category | Task because issue is to improve performance |
---|---|
Issue priority | Major because it contains performance improvements |
Prioritized changes | The main goal of this issue is performance improvements which is considered to be prioritized change. |
Disruption | Could be disruptive for contributed because it includes a API change. |
Comment | File | Size | Author |
---|---|---|---|
#76 | 2472285-76.patch | 16.85 KB | opdavies |
#61 | interdiff.txt | 1.59 KB | lauriii |
#61 | make_theme_registry-2472285-61.patch | 16.84 KB | lauriii |
#57 | make_theme_registry-2472285-57.patch | 16.7 KB | lauriii |
#57 | interdiff.txt | 6.4 KB | lauriii |
Comments
Comment #1
Fabianx CreditAttribution: Fabianx for Acquia commentedTagging
Comment #2
lauriiiI think this issue is more than just changing the cid to be not set in constructor. Theme registry is not lazy loading at the moment and I think that should be done here.
Comment #3
lauriiiLets see how many tests I'm able to break with this!
Comment #5
lauriiiComment #7
lauriiiComment #8
dawehnerJust some quick comments ...
Note: We never assign the variable :) Also, maybe consider to use camelCase.
Comment #11
lauriiiComment #12
lauriiiMissed the patch file :P
Comment #14
lauriiiComment #15
lauriiiComment #18
star-szrOverall looks very good! Nice work @lauriii.
I think this needs a @var with the type.
Super minor: 81 characters, could be rewrapped.
Just curious, why not getStorage here?
Minor: Extra blank line.
Minor: assetTrue
Comment #19
lauriiiThanks for the review @cottser!
Comment #20
star-szrEvil whitespace invades!
This could probably benefit from a beta evaluation and issue summary update, I think it's making API changes if I'm not mistaken.
Comment #21
lauriiiComment #22
lauriiiComment #23
lauriiiComment #24
lauriiiComment #25
lauriiiAnother option would be to still give the cid as a parameter for the class and then set it to a temporary property. Then it could be set as cid on getCid call.
Comment #26
lauriiiComment #27
dawehnerthat doc seems wrong, I don't see $theme in there.
Comment #29
star-szrYup the changes to that docblock need to be rolled back, @lauriii knows :)
Comment #30
lauriiiComment #31
dawehnerundefined variable $lauriii
Comment #32
joshi.rohit100As we are updating the type of $completeRegistry, should we not do the same for $persistable ?
Comment #33
lauriii@joshi.rohit100: no, $persistable has to be set NULL so it can be tested here:
Comment #34
joshi.rohit100@lauriii : Sorry If I am wrong. My point was to document the variable as we did for other.
Comment #35
lauriii@joshi.rohit100 good catch! Sorry for misunderstanding your first commit. If you want, feel free to fix that so I can review it :)
Comment #36
joshi.rohit100@lauriii: Please you can review now.
Comment #37
lauriiiThanks! Looks good. Setting as RTBC per #31
Comment #38
joshi.rohit100But as per #31, variable is undefined :)
Comment #39
catchCould use a better comment. "Gets whether the theme registry is safe to persist?"
Also while it's not introduced by this patch, I'm wondering whether we might want to remove the GET-only requirement from here - that would allow the cache entry to be used on form submissions.
This comment is out of date. We now ensure the cache gets written when a theme registry key is requested, not when the class is constructed.
Comment #40
lauriii#1011614: Theme registry can grow too large for MySQL max_allowed_packet and memcache default slab size it seems like it has been limited to GET since the theme registry cache class has been created.
Comment #41
Fabianx CreditAttribution: Fabianx for Acquia commentedOverall:
+1 from me
Comment #42
star-szrDoes the issue title/summary need an update now?
Comment #43
lauriiiYes it does.
Comment #44
lauriiiComment #45
lauriiiI created a follow up for #39.1 #2493579: Make theme registry persistable also on POST requests
Comment #46
lauriiiFixed #39.2
Comment #47
pfrenssenLooking good. Nice catch!
Comment #48
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedI looked at the patch more closely and I think we introduce a run-time overhead into get() and has():
- It always calls $this->getStorage(), can we optimize to do that only when !isset($this->storage)?
- Same for has() and resolveCacheMiss()
Could also be called "loadStorage()"
Comment #49
XanoThat check already exists within
::getStorage()
. Is the overhead really that much, and will reducing it warrant introducing repeated checks in different parts of the code?A persistable isn't really anything. Maybe write a one-line summary based on the property's description?
Can we inject these dependencies? The class is only instantiated hard-coded in \Drupal\Core\Theme\Registry, which itself is only instantiated as a service or by tests, so adding dependencies there wouldn't be a BC break.
array
doesn't really mean much. What's in the array? Evenmixed[]
would be more explicit and specific.Comment #50
lauriiiXano: so is this what you want for 2? Injecting request_stack for ThemeRegistry.
Tests will still fail.
Comment #52
csg CreditAttribution: csg at Cheppers commentedI added the request_stack parameter to core/modules/system/src/Test/Themes/RegistryTest.php, there are no other occurrences of creating an instance of the ThemeRegistry class.
Comment #53
star-szrThanks @csg, setting to needs review so it gets tested.
Comment #54
dawehnerWhat about name it "isPersistable()"?
Comment #56
catchWith persistable, we should get rid of the GET requirement. Since we disable render caching on POST requests, but may end up rendering parts of a page for form submissions, we want the theme registry cached there if possible.
Comment #57
lauriiiComment #58
joelpittetScenario:
5010 Nodes on the homepage generated with Devel Generatehttp://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5578e29590a73&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5578e29590a73&...
Should I have a different scenario to see the improvement or does this need some work?
Comment #59
joelpittetHere's with caching on though I don't think it really shows much because everything is bypassed.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5578e7733b8ce&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5578e7733b8ce&...
Comment #60
joelpittetWhoops lied only 10 items on the homepage.
Comment #61
lauriiiThis looks a lot better:
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=557c2635273b3&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=557c2635273b3&...
Comment #65
lauriiiI guess the profiling results in the previous comment are not very good :P there is some problem on the rendering during the profiling.
Comment #75
mgiffordComment #76
opdaviesPatch re-rolled. All merged automatically during rebase. :)
Comment #84
donquixote CreditAttribution: donquixote as a volunteer commentedThis should be
No need to initialize with NULL. But the initial null value needs to be documented in @var.
Comment #85
donquixote CreditAttribution: donquixote as a volunteer commentedSame here, it should be
$var array[]|null
.Why?
- array[] is more specific than just array.
- It can be null.
Comment #86
donquixote CreditAttribution: donquixote as a volunteer commentedFrom the OP:
I checked where new instances of ThemeRegistry are created.
The only instance outside of tests was Registry::getRuntime().
Registry->getRuntime() is called in 3 places:
In all those places, the ->has() method is executed shortly after.
This means, the "lazy initialization" will gain us nothing.
The constructor is not "very early", because the object is only created shortly before it is needed.
This "external laziness" is actually more elegant than the internal laziness that this issue is trying to achieve :)
I suggest to close this as wontfix.
Comment #87
donquixote CreditAttribution: donquixote as a volunteer commentedBesides #86, the changes proposed in this issue would make ThemeRegistry even more complex than it is now.