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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, registry.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
1.46 KB
larowlan’s picture

Does this apply to subthemes too? Big savings there too would be great.

Garrett Albright’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7

larowlan, 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:

Plastico ~/Sites/d7git.test/www> sudo -u www drush cc theme-registry ; time sudo -u www drush php-eval "_theme_load_registry('stark', array(), 'phptemplate'); _theme_load_registry('garland', array(), 'phptemplate'); _theme_load_registry('zen', array(), 'phptemplate');"
'theme-registry' cache was cleared                                   [success]
0.521u 0.165s 0:00.77 88.3%	0+0k 0+0io 0pf+0w
Plastico ~/Sites/d7git.test/www> curl http://drupal.org/files/issues/registry_50_0.patch|git apply
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1499  100  1499    0     0   3452      0 --:--:-- --:--:-- --:--:--  8976
Plastico ~/Sites/d7git.test/www> sudo -u www drush cc theme-registry ; time sudo -u www drush php-eval "_theme_load_registry('stark', array(), 'phptemplate'); _theme_load_registry('garland', array(), 'phptemplate'); _theme_load_registry('zen', array(), 'phptemplate');"
'theme-registry' cache was cleared                                   [success]
0.517u 0.169s 0:01.38 48.5%	0+0k 0+0io 0pf+0w

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.

dvessel’s picture

What 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.

dvessel’s picture

Just checked and module activation/deactivation does cause the theme registry to flush. Seems good to me.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Cool, this looks like a nice little improvement!

Committed to 8.x and 7.x.

aspilicious’s picture

Removing tags (needs backport to D7 queue cleanup)

webchick’s picture

Wait. Why are you doing that? :( I want to keep track of D7 issues I've fixed. :(

aspilicious’s picture

Ok Webchick and I fought in irc and I shouldn't have done this.
Secondly I shouldn't had to destroy the Performance tag

dvessel’s picture

I'm not seeing this in the commit history for 7 or 8.

Garrett Albright’s picture

Huh, I just checked… I'm not seeing it either. Maybe webchick hasn't pushed upstream yet?

catch’s picture

Status: Fixed » Needs review
dvessel’s picture

Status: Needs review » Reviewed & tested by the community

Dunno. There's a bunch of commits that came after this one.

catch’s picture

#2: registry_50.patch queued for re-testing.

xjm’s picture

Tagging issues not yet using summary template.

sun’s picture

Still looks good to go for me.

catch’s picture

#2: registry_50.patch queued for re-testing.

catch’s picture

Assigned: Unassigned » webchick

This 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.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Huh. Weird. Sorry, not sure how that happened!

Really Committed and pushed to 7.x and 8.x. ;) Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

kongoji’s picture

FileSize
1.54 KB

Patch #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.