The theme registry is maintained separately for each theme, however the bulk of the work done in _theme_build_registry() is the same regardless of theme.
This patch caches the theme registry for modules - one entry regardless of current theme, then once that's built, any number of themes are able to re-use that when building their own registry.
With the standard core install we're already building two theme registries - one for Bartik and one for Seven. There are different setups (domain access, og_theme) where a lot more themes might be in use.
A full theme registry build for one theme takes anywhere between 300-900ms and between 6mb and 23mb of memory depending on the site and whether an opcode cache is installed (although so many .admin.inc files are loaded by the registry building that it is often cache misses for opcode caches anyway).
The patch reduces this to ~50ms and ~500kb of memory for the second (and any subsequent themes) - in this case I cleared the theme registry, hit the front page with Bartik, then hit 'add content' which loads in Seven.
I'm attaching three files - a full rebuild in Bartik with the patch applied, a rebuild in Seven with the patch applied (note there are only two calls to _theme_process_registry() instead of 29 for that one), and an unpatched theme rebuild from a client site on Drupal 6 with over 200 modules installed - this code hasn't changed all that much since Drupal 6 so that gives an idea with a more complex site.
Theme rebuilds don't happen that often, but there are a couple of reasons this is worth optimizing:
1. A new Drupal user is going to rebuild both the Bartik and Seven theme registries within a few seconds of installing Drupal. While the time taken is variable (depends on opcode caches etc.) it's good for first impressions of Drupal if it's not horribly slow when you first start clicking around. This is the same thing as optimizing the installer.
Patch passes theme tests locally. Marking for backport since the change is entirely internal to _theme_build_registry().
2. If a site is serving a large number of requests per second, there can easily be a stampede on the theme registry since it's a single cache entry that's required to serve any page. This should make the stampedes cheaper at least, we may also want to add an explicit lock as well but the patch doesn't do this.
3. I know of a certain Drupal 7 install that allows Drupal themes to be created via the UI, this involves rebuilding the theme registry for one theme quite often, that particular use case should be improved.
Comment | File | Size | Author |
---|---|---|---|
#22 | registry_50_1.patch | 1.54 KB | kongoji |
#2 | registry_50.patch | 1.46 KB | catch |
theme_registry_rebuild_d6.png | 124.08 KB | catch | |
registry_seven.png | 90.51 KB | catch | |
registry_bartik.png | 108.63 KB | catch |
Comments
Comment #2
catchComment #3
larowlanDoes this apply to subthemes too? Big savings there too would be great.
Comment #4
Garrett Albright CreditAttribution: Garrett Albright commentedlarowlan, yes, there should be no difference whether a theme is a subtheme or not from the perspective of this patch.
FWIW, I didn't notice a great speed gain when I tested like this:
That being said, the logic seems sound, particularly for sites which use Domain Access or another method to run multiple "sites" from the same Drupal core and database where each has its own theme.
Comment #5
dvessel CreditAttribution: dvessel commentedWhat happens when you switch themes and in-between you enable or disable a module? I'm not certain if it is a real problem but I checked the timestamps in the database while switching themes and it didn't cause a rebuild of the registry. If the module activation/deactivation doesn't cause a flush of the registry, you'll end up in a bad state.
Comment #6
dvessel CreditAttribution: dvessel commentedJust checked and module activation/deactivation does cause the theme registry to flush. Seems good to me.
Comment #7
webchickCool, this looks like a nice little improvement!
Committed to 8.x and 7.x.
Comment #8
aspilicious CreditAttribution: aspilicious commentedRemoving tags (needs backport to D7 queue cleanup)
Comment #9
webchickWait. Why are you doing that? :( I want to keep track of D7 issues I've fixed. :(
Comment #10
aspilicious CreditAttribution: aspilicious commentedOk Webchick and I fought in irc and I shouldn't have done this.
Secondly I shouldn't had to destroy the Performance tag
Comment #11
dvessel CreditAttribution: dvessel commentedI'm not seeing this in the commit history for 7 or 8.
Comment #12
Garrett Albright CreditAttribution: Garrett Albright commentedHuh, I just checked… I'm not seeing it either. Maybe webchick hasn't pushed upstream yet?
Comment #13
catchComment #14
dvessel CreditAttribution: dvessel commentedDunno. There's a bunch of commits that came after this one.
Comment #15
catch#2: registry_50.patch queued for re-testing.
Comment #16
xjmTagging issues not yet using summary template.
Comment #17
sunStill looks good to go for me.
Comment #18
catch#2: registry_50.patch queued for re-testing.
Comment #19
catchThis is my patch and it needs backport, since webchick already committed it once, assigning to her so it's clear I won't commit it to 8.x myself.
Comment #20
webchickHuh. Weird. Sorry, not sure how that happened!
Really Committed and pushed to 7.x and 8.x. ;) Thanks!
Comment #22
kongoji CreditAttribution: kongoji commentedPatch #2 works also against Drupal 6, but on update.php generates a fatal error because can't find cache_get function.
I needed to add a
function_exists
to handle also the update.php scenario.