On a Drupal 6 site I'm profiling, the theme registry takes up to 3.5mb of memory on each request.

An extreme version of what can happen is this lightbox2 issue #409354: Performance issues with large number imagecache presets and content types - where dynamic theme functions are leading to max_allowed_packet being exceeded - and because this cache is requested on every request, that means a full registry rebuild and attempted cache_set() every time. memcache also has a 1mb gzipped limit for cache objects and similar problems have been reported in #958930: Performance of 6.x-1.7 still degraded compared to 6.x-1.5.

So this patch adds a new function theme_get_hook(), which maintains a static and persistent cache of theme registry entries by hook name, and falls back to the global theme registry cache when a new one is found.

On a default Drupal 7 install with the standard profile, the theme registry takes up 350kb of memory. With this patch, it's 65kb on the first request to the front page. After posting a node, viewing a node, viewing a user profile then visiting the front page again, it was 144kb - still half. I'd imagine the savings to be a lot bigger once you have several contrib modules installed - since rarely is every single part of every single hook_theme() going to be used across a site.

This doesn't actually fix the problem when the theme registry cache gets too big to actually cache (although it would remove the worst of the performance implications since we wouldn't be trying to load the full registry every request), but that's a different problem and harder to solve.

The main drawback here is there will be a number of cache_set() while the smaller cache is being built. The 145kb cache entry I built has 65 entries. However these aren't rebuilds, just requesting the already cached full registry and adding parts of it to the runtime cache.

Did some click around testing while profiling but haven't run simpletests on this. Once this passes I'll do a D6 backport and see how much impact it has on that 3.5mb cache entry.

CommentFileSizeAuthor
#88 theme_registry.patch8.72 KBcatch
#87 theme_registry-1011614-87.patch9.47 KBpillarsdotnet
#85 theme_registry-1011614-85.patch9.44 KBpillarsdotnet
#81 theme_registry.patch8.72 KBcatch
#79 theme_registry_12_0.patch8.72 KBcatch
#77 theme_registry_12.patch8.71 KBcatch
#68 theme_registry.patch8.72 KBcatch
#58 Desk 1_083.png97.58 KBcatch
#58 theme_registry.patch15.68 KBcatch
#57 theme_registry.patch15.8 KBcatch
#55 diff.diff15.26 KBcatch
#52 theme_registry_pressflow_D6.patch9.46 KBcatch
#52 before-d6.png81.99 KBcatch
#52 Desk 1_071.png82.46 KBcatch
#49 theme_registry_D7.patch13.22 KBcatch
#41 registry.patch13.22 KBcatch
#37 theme_registry.patch13.65 KBcatch
#36 theme_registry.patch14.1 KBcatch
#34 theme_registry.patch14.83 KBcatch
#33 theme_registry.patch12.04 KBcatch
#33 cache_set.txt10.88 KBcatch
#33 registry.txt1.01 KBcatch
#33 runtime.txt24.54 KBcatch
#30 theme_registry.patch11.86 KBcatch
#29 theme_registry.patch11.88 KBcatch
#27 theme_registry.patch11.89 KBcatch
#26 theme_registry_runtime_again.patch9.22 KBcatch
#24 theme_registry_runtime.patch8.71 KBcatch
#20 theme_registry_runtime.patch8.66 KBcatch
#19 theme_registry.patch7.1 KBcatch
#18 theme_registry.patch4.48 KBcatch
#15 theme_registry_ArrayAccess.patch4.57 KBcatch
#13 theme_registry_ArrayAccess.patch4.57 KBcatch
#13 theme_registry_ArrayAccess_D6.patch4.78 KBcatch
#13 Desk 1_051.png141.98 KBcatch
#13 Desk 1_052.png136.99 KBcatch
#12 theme_registry_arrayAccess.patch3.72 KBcatch
#10 registry_array_access.patch2.35 KBcatch
#5 theme_registry_cache_1011614_D6.patch3.57 KBcatch
#4 theme_registry_cache_1011614.patch7.23 KBcatch
#1 theme_registry_cache.patch6.86 KBcatch
theme_registry_cache.patch6.9 KBcatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

FileSize
6.86 KB

Stray dsm().

Discussed briefly with chx in irc, he doesn't like the number of writes this might cause, we can get around this by doing a similar thing to the system paths caching in path.inc - write the cache at the end of the page request instead of each time. Since the theme registry is a lot more static than variables etc., there's not the same opportunity for race conditions so this ought to work fine. However that's not in this patch yet.

Status: Needs review » Needs work

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

catch’s picture

Oh another question from chx was "won't the cache eventually reach the same size as the full registry once all theme functions are called?" (paraphrasing). I don't think it will on the vast majority of sites for the following reasons:

* If you use an admin theme, then theme functions only called on admin pages won't be cached for your main site theme, and theme functions only called on non-admin pages are never going to be cached for your admin theme. The theme registry is per-theme. This is covers a very large number of Drupal installs (and will be considerably more with Drupal 7 now that Seven is in core), and it'll mean there'll definitely be a memory saving for both kinds of pages on those sites (probably a dramatic one for admin pages, less so for user-facing pages but it's hard to tell).

* If you use any kind of user theme switching, a mobile theme, og_theme, spaces or similar, then again those themes are only going to be used in particular contexts for the site - so will likely only need a subset of the theme registry.

* A lot of modules provide theme functions for blocks, where that block might never be used (or is only used in one theme), or like that lightbox2 issue might be registering theme functions dynamically for other site objects where they're not necessarily all going to be called depending on configuration settings.

* Any situation where you're overriding core functionality (for example page manager or views using hook_menu_alter() to replace pages like node/n or taxonomy/term/n)., the theme function for that page will still be registered, but it's never going to be called.

Going to look at the test failures now.

catch’s picture

Status: Needs work » Needs review
FileSize
7.23 KB

Missed one case of $hooks[$hook]; for theme suggestions, theme API test now passes, assuming this will sort out the other failures.

catch’s picture

And an initial Drupal 6 backport, this doesn't handle the possibility of a registry build function other than the default yet so only for testing purposes really.

Anonymous’s picture

subscribe.

sun’s picture

This makes a lot of sense. The only part I'm concerned about is the subsequent/iterative updating of the theme registry hook cache.

First of all, the current code seems to attempt to update the cache for every single theme_get_hook() invocation (and cache miss). We should investigate usage of a shutdown callback or something to execute the update query only once.

Second, this change basically introduces at least one additional write operation (much more without aforementioned fix) for potentially every single and "unique" page on a site after flushing caches. For high-performance cache backends, that's probably not an issue, but I wonder about the performance implications for regular SQL/database cache backends.

catch’s picture

Yeah we'll need to put the caching at the end of the request, drupal_page_footer() is where most of those things currently go.

On the write operation, this is true, any page that uses a theme hook that's not already been cached will trigger an extra write. The main question is how many of those are there likely to be immediately after a cold start.

sun’s picture

Note that drupal_page_footer() is only invoked for HTML pages, but for example, not for AJAX requests, but those can invoke theme functions, too. Likely needs to be part of drupal_exit() instead, or, ahem, actually both, since they are substitutes and we don't seem to have a commonly shared helper hook/facility yet... :-/

catch’s picture

FileSize
2.35 KB

Berdir pointed me to his github fork using ArrayAccess as a way to split the schema cache.

The exact implementation of that won't work as well for here, schemas are rarely requested for more than a handful of tables per request, for the theme registry there are a lot of calls to theme() per request so it's a different usage pattern, even though the goals are similar.

However the basic idea looks like something we could use here, and would mean all the logic of building the theme registry over time could be done in the class, and there wouldn't be any need to change l() and other places that rely on being able to check something in the full registry array.

Here's an initial patch that just converts the registry itself to an ArrayAccess object. 8.x loads a page correctly with the patch applied, if it passes tests then that's a good point to start from for adding the caching changes. This will also make it easier to backport, since there'll be zero changes to theme() or anything else.

Patch is needs review only to avoid me having to run the full test suite locally.

Status: Needs review » Needs work

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

catch’s picture

Status: Needs work » Needs review
FileSize
3.72 KB

Here it is with the same caching strategy as #4.

ArrayAccess is really well suited to this - we're even able to use the destructor to set the cache at the end of the request instead of messing about with hook_page_footer(), and the only function that gets changed in all of core to implement this is theme_get_registry().

I'm attaching screenshots from a nearly standard install (+testing, -overlay), this is just after hitting the front page a couple of times with and without the patch - neither the full theme registry is realistically large enough (it is more than 3-4mb on at least a couple of sites I've used), nor is the runtime registry large enough (this is just one page which won't exercise all theme functions actually in use on the site), but it gives an idea.

Once this passes tests (theme tests pass but I didn't do a full run) I'll do the Drupal 6 backport then try this on a real site with a large theme registry to see how much difference it actually makes.

In irc, chx suggested using ArrayObject instead of ArrayAccess, but I don't see a nice way to pass the arguments into the constructor with that.

catch’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
FileSize
136.99 KB
141.98 KB
4.78 KB
4.57 KB

Better comments on the 8.x patch, applies cleanly to 7.x, also initial 6.x version (6.x version is untested), and the xhprof screenshots I failed to upload before.

Status: Needs review » Needs work

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

catch’s picture

Status: Needs work » Needs review
FileSize
4.57 KB

Hmm, theme tests still pass locally, new patch with minor cleanup anyway.

yched’s picture

This looks sweet, and might definitely help over at #1040790: _field_info_collate_fields() memory usage.

I'm wondering if there would be a way to abstract it a little :

+++ b/includes/theme.inc
@@ -563,6 +542,113 @@ function _theme_build_registry($theme, $base_theme, $theme_engine) {
+      $completeRegistry = _theme_build_registry($this->theme, $this->base_theme, $this->theme_engine);

If this was just $completeRegistry = $this->completeRegistry();, then it looks like the whole construct could be easily reused by other registries by subclassing and overriding __construct() and completeRegistry() ?

Powered by Dreditor.

yched’s picture

Also :

- do we really need to cache_set() in both resolveRegistry() and __destruct() ?
- $completeRegistry is camelCased ? :-)

catch’s picture

FileSize
4.48 KB

Abstracting it so other caches like this can re-use it would be great. I haven't yet split this out into it's own class to inherit from, may do that next patch iteration.
Right now only offsetExists() and offsetGet() are generic, __destruct() is nearly generic (possibly we could add an 'isCacheable()' method then it would be). That's probably enough to make it worth it. ArrayObjectCache? in cache.inc?

Possible candidates for using ArrayObject instead of their current implementations:

- drupal_get_path() - system path cache and maybe parts of the current static caching.
- module_implements() cache (uses exactly this pattern except it's baked in)
- schema cache - see berdir's sandbox and the core isssue, slightly different pattern needed.
- variable cache.
- system_get_info()
- system_list()
- _field_info_collate_fields()
I'm sure there's more.

resolveCacheMiss() is setting cache for the complete theme registry if there's a cache miss there - it is definitely worth having the whole monster cached since rebuilding it from scratch is very expensive and we don't want to do that each time there's a miss on the runtime cache. This is different from __destruct() that sets the runtime registry cache. If this isn't clear from the patch I should add more comments.

I realised __construct() can take any arguments you like without throwing an E_STRICT warning, so my only objection to chx's suggestion of using ArrayObject is removed and this new patch extends ArrayObject instead of implementing ArrayAccess. This drops two methods that weren't neede and gives us some other nice things (which aren't needed here but might be in other systems).

Also fixed the camel case and washed my mouth out with soapy water.

catch’s picture

FileSize
7.1 KB

Here it is as an API, was able to clean it up quite a bit (partly due to making it an API, partly just from reading through it again).

This patch adds an interface to cache.inc - CacheArrayObjectInterface - this defines three methods that are needed.

Then there is a default class CacheArrayObject extends ArrayObject implements CacheArrayObjectInterface, that is not quite useful by itself but very nearly.

ThemeRegistry now only extends CacheArrayObject and overrides three methods.

catch’s picture

One more iteration.

I think it's necessary for theme_get_registry() to return the full array instead of the ArrayObject - that way you can dpm(theme_get_registry()); and see what's really in there.

So this patch adds theme_get_registry_runtime() and a couple of other necessary helpers, which are used internally (and could be used by contrib modules that are just doing something like contextual.module does).

This makes the patch bigger, but it's now strictly an API addition.

For some other caches like the schema which provides a way to get the value of an array key as well as the whole thing, we wouldn't need to add the extra helpers and could just use the ArrayObject or not internally.

This means if you have a contrib module that calls theme_get_registry() on every request, you won't see a memory saving (but this could always be patched), however unless I missed something the change should be completely transparent now.

Status: Needs review » Needs work

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

catch’s picture

Status: Needs work » Needs review
FileSize
8.71 KB

This ought to do it.

Status: Needs review » Needs work

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

catch’s picture

Status: Needs work » Needs review
FileSize
9.22 KB

Hmm, everything fine locally, trying once more.

catch’s picture

FileSize
11.89 KB

More cleanup.

Minimal stampede/race condition prevention in CacheArrayObject::__destruct()

Instead of new _runtime() functions in theme.inc I added an optional parameter to theme_get_registry(), this is a lot less code duplication and the function currently takes no arguments so it's not too bad. We may eventually want to default the parameter to FALSE in Drupal 8, but I think this would be fine for both 7 and 8 at the moment.

Added lots of docs.

Now using NULL instead of FALSE to indicated 'does not exist' - since it's more semantic, and I can't think of a single place in core where we have a large array like this and use NULL as a meaningful value for the top level array, whereas we do differentiate between FALSE and NULL quite often. If you really need to differentiate between NULL and not set at all, you can override the methods and still use this.

Status: Needs review » Needs work

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

catch’s picture

Status: Needs work » Needs review
FileSize
11.88 KB

contextual module got left out, apologies again for repeated patch fail.

Here's a list of current issues that could probably be simplified by this, there may be more than these. Disclaimer: I admit to opening a fair number of these issues in the first place.

#1080964: Be more sparing with locale cache clears
#1040790: _field_info_collate_fields() memory usage
#987768: [PP-1] Optimize variable caching
#402896: Introduce DrupalCacheArray and use it for drupal_get_schema()
#1102252: Caching of defaults
#853864: views_get_default_view() - race conditions and memory usage

catch’s picture

FileSize
11.86 KB

Justin reviewed in irc.

Anonymous’s picture

if the test bot comes back happy, then i'm happy with this patch. not RTBC'ing until other people have reviewed.

yched’s picture

Status: Needs work » Needs review
+++ b/includes/cache.inc
@@ -506,3 +506,154 @@ class DrupalDatabaseCache implements DrupalCacheInterface {
+ * CacheArrayObject.a

Leftover "a".

+++ b/includes/cache.inc
@@ -506,3 +506,154 @@ class DrupalDatabaseCache implements DrupalCacheInterface {
+  /**
+   * Whether the array can be written to cache this request.
+   *
+   * @return
+   *   Boolean to indicate cacheability of the arrayObject.
+   */
+  public function cacheable();

"Whether the array can be written to cache [for ?] this request"
We're not talking about caching the request, are we ?

I'd also nitpick about the method name, I find it a bit misleading (we're dealing with caches, so a non-cacheable cache sounds puzzling).

If I get this correctly, this is about whether there were some runtime cache misses+resolutions that should be written back to the persistent cache, right ?

Also, for the specific ThemeRegistry implementation, shouldn't the module_load_all() check happen at the time you try to resolve the miss in resolveCacheMiss(), rather than in cacheable() (called at the end in __destruct()) ? I'd think what matters is whether the runtime resolution is "well informed" at the moment you made it ? When __destruct() runs, all modules might have been loaded, yet $this->cache_add could contain resolutions that were made while not all modules were loaded, and should therefore not be persisted.

--> proposal : rather than cacheable(), have a persist($offset, $value) method that adds to $this->cache_add - or even have this baked in CacheArrayObject::offsetSet() through a third boolean parameter, defaulting to TRUE ?
resolveCacheMiss() decides whether it should persist the runtime resolution (module_load_all() check), and __destruct() writes $this->cache_add if it's non empty ?

Also, $this->cache_add could probably only store the array of offsets to persist, rather than duplicate the values as well ?

+++ b/includes/theme.inc
@@ -277,23 +291,34 @@ function _theme_registry_callback($callback = NULL, array $arguments = array())
+    $cache = cache_get("theme_registry:$theme->name", 'cache');

@@ -313,6 +338,48 @@ function drupal_theme_rebuild() {
+    $this->cid = 'theme_registry:runtime:' . $theme->name;

The theme_get_registry($complete) trick relies on the fact that there are two separate cache bins ('theme_registry:' . $theme->name, 'theme_registry:runtime:' . $theme->name). Would there be a place to make that explicit in the comments somewhere ?

+++ b/includes/theme.inc
@@ -313,6 +338,48 @@ function drupal_theme_rebuild() {
+ * complete theme registry is loade and used to update the run-time cache.

typo, "loaded"

Powered by Dreditor.

catch’s picture

FileSize
24.54 KB
1.01 KB
10.88 KB
12.04 KB

Here's some rough testing for how this looks on a real-ish site (in this case, Drupal 8 with all optional modules enabled). That's a simple site that's not really the target for this patch, but if it's OK for that, then it should be good for sites with a lot more modules installed.

The full theme registry with all those modules installed comes in at 186 items. Each item isn't exactly the same size but it'll average out.

I added logging with file_put_contents() to CacheArrayObject::__construct() and cache_set(). The __construct() logging recorded which page request was causing a cache set, and how many items were in the cache when it was set, along with which theme. The cache_set() logging recorded all cache sets with cid and request path. Then I ran drush cc all, hit the front page, then clicked around a bit as user/1.

Attaching both files here.

Summary:

- After clicking around several paths that are owned by different modules, Bartik had 114 items in the runtime registry, and Seven had 93. They both grew about 15-25 items at a time, bartik slowed down massively after 95 items - picking up ten then 4-5 items per page, suggesting it was beginning to top out at least somewhat. With this simple site and no contrib modules that's around 50% smaller for the theme registry. I'll do a similar test on a very large D6 site soon once I've got that backport working.

- From a completely cold start, both the module_implements() and theme registry caches are getting updated on most requests (10 or so pages visited).

- the cache_set() calls for these are not much compared to the cache_set() calls for the menu cache - which is often 6-8 items per request.

However one bit of particularly bad news at least for reducing the size of the theme registry:

- I dumped the runtime registry cache once it'd been built, and there are lots of cached items for hook suggestions (usually array($suggestion => NULL)). This happens due to the caching of non-existence of items in the ArrayObject. I'm not sure this is a surmountable issue for the theme registry so it may turn out this wasn't a good issue to start with, but for other large caches it's going to be fine (field info, schema etc.).

Here's a sample:

[html__node__3] =>
    [html__node__%] =>

This is closely related to #956520: Available theme hook suggestions are only exposed when there are preprocess functions defined and #653622: Make the '__' pattern for theme suggestions easier to use - there are multiple ways that theme suggestions can happen, some are explicitly registered, some are based on patterns, and there are obviously infinite combinations.

It might be possible to do something tricky like cache registered items globally, but cache non-registered items per-page - that'd give you small two small cache objects for each page instead of one big one, the per-page cache should be extremely small and could also have a sanity limit, and this would mean the numbers above look a lot better since the main runtime registry is going to be even smaller.

While doing this testing I found a couple more issues in the patch, so I'm uploading all three logging files and the updated patch, CNR only to ensure tests still pass, but this is CNW for now.

Side note - there's a lot of setting of the variable cache in cache_set.txt and I can't see why there would be, needs investigating.

I may break the API out into its own issue, or system_get_info() / schema cache are good candidates without the same pitfalls.

catch’s picture

FileSize
14.83 KB

Crossposted with yched's review, this patch doesn't deal with any of those points yet.

I agree with persist(), that's nice.

cacheable() - this could include caches that we never want to set on POST requests or if there's a session (maybe), it's not only supposed to mean that there's stuff to cache, but using persist() for that logic seems sensible to me.

Storing keys to cache only - yeah this might be doable.

--

I wrote a quick implementation of a global cache for registered theme hooks and a per-path cache for unregistered theme hooks (suggestions) - 'cos it turns out to be easy to write (ThemeCache has to override ___destruct() and some extra logic gets added to __construct()).

I logged count($cache) again, this time to one file called registered.txt (the global cache), and another unregistered.txt (per-page cache). This is what it looks like, slightly different click path to before but very similar:

registered.txt - this is cumulatively built across the site, each item is an array from the registry:

count: 35 uri: / theme: bartik
count: 46 uri: /node/6 theme: bartik
count: 12 uri: /node/add?render=overlay theme: seven
count: 35 uri: /node/add/article?render=overlay theme: seven
count: 40 uri: /node/add/article?render=overlay&render=overlay theme: seven
count: 47 uri: /admin/modules?render=overlay theme: seven
count: 51 uri: /admin/config?render=overlay theme: seven
count: 61 uri: /admin/content?render=overlay theme: seven
count: 50 uri: /forum theme: bartik
count: 52 uri: /aggregator theme: bartik

unregistered.txt - each item is just NULL - this is per page (but cumulative should suggestions change on one page).


count: 34 uri: / theme: bartik
count: 34 uri: /node/6 theme: bartik
count: 34 uri: /node/5 theme: bartik
count: 8 uri: /node/add?render=overlay theme: seven
count: 12 uri: /node/add/article?render=overlay theme: seven
count: 17 uri: /node/add/article?render=overlay&render=overlay theme: seven
count: 34 uri: /node/7 theme: bartik
count: 12 uri: /admin/modules?render=overlay theme: seven
count: 8 uri: /admin/config?render=overlay theme: seven
count: 8 uri: /admin/reports?render=overlay theme: seven
count: 11 uri: /admin/content?render=overlay theme: seven
count: 11 uri: /admin/content/comment?render=overlay theme: seven
count: 27 uri: /forum theme: bartik
count: 19 uri: /misc/forum-default.png theme: bartik
count: 24 uri: /aggregator theme: bartik
count: 26 uri: /aggregator/sources theme: bartik

So the registered cache gets set 10 times now - compared with 16 for the per-page cache - that's a larger item so it's good if there are less sets. The cumulative cache doesn't get above 61 items for either theme (out of 180 total registered).

And the unregistered per-page cache, the worst example is 34 unimplemented theme suggestions - this is going to be a similar size to the path alias cache or smaller. I don't think it's safe to cache it any less granular than per page since we have nids used for suggestions.

That makes this somewhat viable still but I'll probably leave it there for a while, get yched's feedback into the base class, then try a different system that doesn't have infinite possible array keys to check for so there's a more straightforward implementation to look at.

Status: Needs review » Needs work

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

catch’s picture

FileSize
14.1 KB

New version that should deal with all the points in yched's review (adds a set() method alongside persist()).

Except the $complete trick, I wasn't sure where best to document this in theme_get_registry() - that is mostly documented in the ThemeRegistry class.

Passes theme tests locally.

catch’s picture

FileSize
13.65 KB

chx reviewed some more in irc. This patch moves CacheArrayObject to bootstrap.inc and drops the interface, methods are documented in the class itself now. Otherwise no change.

yched’s picture

+++ b/includes/bootstrap.inc
@@ -2832,6 +2832,162 @@ function drupal_static_reset($name = NULL) {
+  public function persist($offset, $value, $request_only = FALSE) {
+    parent::offsetSet($offset, $value);
+    $this->add_keys[] = $offset;
+  }

$request_only is not used, the offset is unconditionally added to $this->add_keys ?

+ nitpicking : I was more thinking of "persist" as strictly "add to persistent cache (aka cache_set())", i.e promote a clear terminology separation between "request cache" and "persistent cache". Something like :

public function offsetSet($offset, $value, $persist = TRUE) {
  parent::offsetSet($offset, $value);
  if ($persist) {
    $this->add_keys[] = $offset;
  }
}

(not sure if we're allowed to extend the signature of a function that belongs to the interface, though ?)

+++ b/includes/theme.inc
@@ -313,6 +339,81 @@ function drupal_theme_rebuild() {
+  public function resolveCacheMiss($offset) {
+    // If there is a cache miss and the registry is requested before
+    // all modules have been loaded, ensure that there is no chance
+    // of a partial registry being written back to cache.
+    if (!isset($this->persistable)) {
+      $this->persistable = module_load_all(NULL);
+    }
...
+    $this->persist($offset, $value, $this->persistable);
+  }

I guess that's minor, and the case might not happen in real request flows, but some resolutions might not be persistable (all modules not loaded yet), while some later ones might be (all modules were loaded meanwhile).
The module_load_all() is inexpensive (reading a static), and could be made for each resolution individually ? (or at least while empty($this->persistable), no need to check once it's TRUE - but that would only make $this->persistable replicate module_load_all()'s static, I'm not sure that's even worth it)

Powered by Dreditor.

catch’s picture

@yched: yes the default implementation ignores $request_only - if you wanted to support that you'd have to override the function. Same as resolveCacheMiss() doesn't actually resolve a cache miss.

We could completely remove these methods from the class, put the interface back, and leave them there, but there will be some things that are always cacheable - for example the schema probably is. Once there are a couple more implementations of this we might be able to find a generic pattern for those methods. This was the original reason I made the interface but as time has gone on, the class has got more generic.

The module_load_all() check only mirrors the one in _theme_load_registry() - if the full registry is loaded and that isn't persistable, then neither is anything we use in ThemeRegistry - and the module_load_all() static checking only happens on misses, so I'm fine doing that every time. The only other thing would be to share a static between _theme_load_registry() and here - and then check this (or do something fugly like per #don't_cache_me in the theme registry when it can't be cached ;), but I'm not sure that's worth it for what should be a very rare edge case.

We'll get a strict warning if we try to extend the signature of offsetSet() - so it needs to be a new function even if it's not called persist().

Fabianx’s picture

Subscribe

catch’s picture

FileSize
13.22 KB

Took $request_only and $value out of persist(), tests still pass. I'm going to get this implemented in drupal_get_schema() next so there's a second implementation to review, not planning to tweak this any more here unless something new comes up in review.

andypost’s picture

Great approach! Not sure how this affects a performance but saves a lot RAM. Also this should be extended on field's definition cache to resolve #1040790: _field_info_collate_fields() memory usage

catch’s picture

There's a working patch at #402896: Introduce DrupalCacheArray and use it for drupal_get_schema() for the schema cache now, ArrayObject hunk is the same there as here. Only needs to override one method of the base class although there are other changes to make it work in general.

I won't have time to work on the field info cache until next week some time, but that's probably up next.

pillarsdotnet’s picture

omercioglu’s picture

sub

podarok’s picture

#41 under D7.2 with shift making WSOD without errors at all

pillarsdotnet’s picture

with shift

Pardon my ignorance, but what does that mean?

podarok’s picture

#41 does not apply cleanly for 7.2 so I used hands and put the code into 7.2 with a small shift in lines 8)
D8 is changed enough against D7 for the patch are not applying cleanly

catch’s picture

FileSize
13.22 KB

Here's a D7 patch that applies cleanly.

@podarok are you getting a 500? 200 with empty content? Could you check PHP/apache error logs?

D6 backport coming up soon, this can't get into D6 proper since it requires PHP5 but I'm hoping to port it to Pressflow

podarok’s picture

2catch
It was a fast test - I`ll try to test a speed and memory with a patch for my "huge" project
(800+ fieldapi types(bundles) with 20-60 fields in each (~15000 fields at all with 2000+ uniq fields (some fields are shared between bundles)))
Page, that I want to display - it is a views page with selectors based on field_filters
Page displayed in 10-11 seconds and the main trouble - memory.

So I`ll try this patch again
with patch
Memory used at: devel_boot()=2.62 MB, devel_shutdown()=176.25 MB, PHP peak=191.75 MB.

Overall Summary
Total Incl. Wall Time (microsec): 12,457,746 microsecs
Total Incl. CPU (microsecs): 12,211,197 microsecs
Total Incl. MemUse (bytes): 181,395,376 bytes
Total Incl. PeakMemUse (bytes): 197,612,060 bytes
Number of Function Calls: 45,854

without patch
Memory used at: devel_boot()=3.12 MB, devel_shutdown()=175.96 MB, PHP peak=191.5 MB.

Overall Summary
Total Incl. Wall Time (microsec): 12,473,317 microsecs
Total Incl. CPU (microsecs): 12,187,739 microsecs
Total Incl. MemUse (bytes): 181,755,296 bytes
Total Incl. PeakMemUse (bytes): 197,586,256 bytes
Number of Function Calls: 41,384

No WSOD with @catch Your D7 patch!
All working

andypost’s picture

@podarok Suppose your case is not affected by theme-registry bottleneck... So better to try #402896: Introduce DrupalCacheArray and use it for drupal_get_schema()
A lot of fields cause a big schema cache & field cache

In your case you can get better memory stats with this approach at #1040790: _field_info_collate_fields() memory usage

catch’s picture

Here's a working D6 patch.

I'm also attaching before/after screenshots from xhprof - testing the patch on a large Drupal 6 site.

Before the patch, memory usage from _theme_load_registry() is 3.2mb

With the patch, and after hitting 6-7 pages to warm the runtime cache a bit, it comes to 360kb, even if that doubles or triples after the cache has been building for a while, it is still a significant memory saving.

Before:
before-d6.png

After:
Desk 1_071.png

Peter Bowey’s picture

Refer #52

The enclosed *patch* @ http://drupal.org/files/issues/theme_registry_pressflow_D6.patch
causes this error (using Pressflow 6.22)

Warning: mysqli_real_escape_string() expects parameter 1 to be mysqli, null given in db_escape_string()

I rolled the given patch out! Now I see no errors...

catch’s picture

Status: Needs review » Needs work

Just thought of a better implementation here:

The current patch keeps a global (per-theme) runtime cache of used theme hooks.

Because theme() allows for invalid theme hooks to be passed in (suggestions etc.), it also keeps a per-page cache if unimplemented theme hooks so that these aren't a cache miss. This keeps the memory usage down but at the cost of adding a new per-page cache.

Pretty sure we can skip that entirely, by keeping a full list of array_keys(theme_get_registry()) in the runtime cache, with the value initialized to an empty array. offsetExists() will always return true or false correctly for that array without any resolveCacheMiss() logic being invoked. offsetGet() however can check if !empty() and if it's empty fill in the array value from the complete registry same as we're already doing now in resolveCacheMiss().

The list of implemented-but-unused theme hooks may be longer than the list of per-page-unimplemented-theme-hooks, but it's not going to be astronomical, and it won't contain values so should be tiny. Compared to the current approach we're looking at most an extra 100kb or so, and still mb savings compared to core, while removing per-page caching entirely.

Will get a patch up for that tomorrow.

@peter bowey, the issue you're hitting is that with the Drupal 6 database layer, the mysqli connection object is being destroyed on shutdown before the CacheArrayObject() so the db isn't available. This isn't an issue with memcache, I'll likely commit a workaround to the Pressflow branch either this week or early next week.

catch’s picture

Status: Needs work » Needs review
FileSize
15.26 KB

Confirmed the caching works with this approach but didn't run tests on this one yet.

The ThemeRegistry class looks a lot more verbose, it may be possible to trim that - I only wanted to override ->set(), but without overriding __destruct() too my overridden ->set() doesn't appear to be called. Will post profile data later and a bit more background. Even if we can't this is saving a lot of writes now and should be very close to previous memory savings. No changes outside the class (except I updated the other stuff based on the schema cache issue but that had to happen anyway).

Status: Needs review » Needs work

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

catch’s picture

Status: Needs work » Needs review
FileSize
15.8 KB

This should make those two tests pass.

theme_links() unit test could only be a unit test because theme_get_registry() uses a static cache instead of drupal_static(), so not really a real unit test. Changed to web test case.

Dashboard module calls drupal_initialize_theme() in hook_enable(), module_load_all(NULL) returns true bit it was getting a partial registry. I fixed this by not caching the registry if $_SERVER['REQUEST_METHOD'] != GET, which makes some sense anyway, but will look into it a bit more because while it's a bit weird initializing the theme in hook_enable(), it's also weird that you can end up with a partial theme registry doing that.

catch’s picture

FileSize
15.68 KB
97.58 KB

Here's some cleanup, I've not been able make the class less verbose but functionally this is much better than previous approaches.

xhprof screenshot for a vanilla D8 site with the patch - theme registry usage is 100kb with the patch (compared to 400kb without it). Will try to post a more realistic example tomorrow (using that same D6 site).

catch’s picture

Once this is in the next step would be working on #1188084: Try to avoid loading every specified include file during theme registry rebuilds. (probably for D8 only), loading all files specified in hook_theme() can easily take 4mb or so, if we can skip that this would be a lot of memory freed - although depends how CPU-intensive the replacement is.

Peter Bowey’s picture

@catch

Many thanks for following this Drupal [theme registry] concept through!

I am following the incremental improvement very closely (most of all in relation to changes for Pressflow 6.22 -> https://launchpad.net/pressflow)

See https://code.launchpad.net/~catch-drupal/pressflow/cachearrayobject

I note the last change for pressflow was [2011-07-01 11:54:27] for http://bazaar.launchpad.net/~catch-drupal/pressflow/cachearrayobject/rev....

I am hoping that the time will come when this works with APC or Xcache 'bins', as apposed to depending on Memcache methods.

catch’s picture

The D6 site I've been testing this with, after clicking around several pages, had a runtime registry of 549kb with the latest version of this from launchpad, this is compared to 3.2mb without.

I'd expect it to grow a bit more with usage, but reckon we'll end up with 50-80% savings on many sites - especially those using admin themes.

Peter Bowey’s picture

Refer #61
@catch
I think it is time I re-tested [real-time on a semi-production site] this Pressflow 6.22 update +'emailed Patch'.

I will report back, after I roll-in same backups + safeguards to preserve some of my own 'sanity' :)

Peter Bowey’s picture

#refer 61

Not quite :)
With Pressflow 6.22

Error logs:

Warning: mysqli_real_escape_string() expects parameter 1 to be mysqli, null given in db_escape_string() (line 388 of /var/www/virtual/peterbowey.com.au/includes/database.mysqli.inc).

Notice: Undefined property: SchemaCache::$cid in SchemaCache->destruct() (line 67 of /var/www/virtual/peterbowey.com.au/modules/schema_cache/schema_cache.module).
dixon_’s picture

subscribing

franz.glauber’s picture

sub

franz’s picture

sub

xjm’s picture

Tagging issues not yet using summary template.

catch’s picture

FileSize
8.72 KB

#402896: Introduce DrupalCacheArray and use it for drupal_get_schema() is in. Here's a re-roll. Passes theme tests locally but didn't run the rest.

Another issue for background from the memcache queue, opened this week: #1257768: memcache set not working for theme registry cache.

catch’s picture

Title: Use a partial theme registry cache during runtime » Theme registry can grow too large for MySQL max_allowed_packet and memcache default slab size
Category: task » bug

Moving this to a bug. The default value for max_allowed_packet on MySQL is 1MB, see http://dev.mysql.com/doc/refman/5.5/en/packet-too-large.html max slab size for memcache is 1MB too.

I posted a patch to document this for cache_set() at #1261846: Document 1MB maximum size limit for cache_set().

This patch does not remove the massive cache item, just stops it being needed every request. I don't think we can usefully remove it altogether for Drupal 7 without a (smallish, but still...) API change, however I'll post an 8.x issue to start attacking this part of things soon - the same problem exists with the schema and other registries (and the same rough API change as well).

Alan Evans’s picture

Has there been any discussion of the feasibility of sharding cached objects? So, if any object approaches the 1MB limit, put a warning in watchdog and automatically break it up into parts that can be reassembled on cache_get? (That's just a very basic outline - there are complexities and still places where this could break, hence the warning).

The idea behind always warning is that there should be some record of sharding happening so that it could be flagged up and mitigated in the code that's causing the big cache object where possible.

There is obviously overhead to consider in the sharding logic itself.

Mainly just wondering if this has been already considered and rejected...

Doesn't sound like anything that could be patched in MySQL/memcached themselves, as the limits are there for a reason.

catch’s picture

There's a patch to the Drupal memcache project here to add a watchdog message if a cache set fails and the item is also greater than 1mb #435694: >1M data writes incompatible with memcached (memcached -I 32M -m32).

I'd be wary of doing more than this since in terms of automated sharding since you can enable compression in memcache, and store cache items that uncompressed might be several megabytes. Same as MySQL allowed packet size can be increased as well. So if we hard coded the 1mb limit in core anywhere we might actually make things worse.

Alan Evans’s picture

I doubt that we'd want to hardcode it for sure - it'd be optional and even the size threshold could be configurable.

As I imagine this working, when you set something in cache, you'd need to check the size of the object you're storing and split it up, storing at the same time a marker (under, say, cache key "shards:my-cache-key") which contains information about the individual shards stored. This would mean that every cache get would have to check against the "shards" registry, but we'd have to assume sharded objects would be relatively rare. If a shard record is found, reassemble the bits and return that. That's then an additional cache backend lookup for every single cache get, regardless of whether it was sharded or needed to be. Writing this down now, that doesn't sound likely to be acceptable.

Alternatively you could elect to store some kind of "magic marker" under the sharded cache key itself which just points onward to where the actual shards are stored, but I can't really decide which is the lesser evil: magic values or the previous additional storage option. At the moment I'd lean toward magic values being the less wasteful.

I haven't thought through implications for multi gets.

Apologies if I'm barking up entirely the wrong trees ... seemed worth exploring a bit though. And, point taken, *if* this idea is any use at all, it might not belong in the core cache API.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Nice work folks. Its ready.

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Performance, +Needs issue summary update, +memory, +Needs backport to D7

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

bibo’s picture

Subscribing.

catch’s picture

Status: Needs work » Needs review
FileSize
8.71 KB

Cache API changes broke this. Re-rolled.

chx’s picture

Status: Needs review » Needs work

UGH private? protected, please. webchick did convince me in Denver that protected does help developers by hiding stuff they dont need to change but private makes my life harder if I happen to extend your class.

catch’s picture

Status: Needs work » Needs review
FileSize
8.72 KB

Re-rolled with protected.

Status: Needs review » Needs work

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

catch’s picture

Status: Needs work » Needs review
FileSize
8.72 KB

Test bot doesn't like my fuzz.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

thanks for reroll. back to rtbc.

catch’s picture

Assigned: catch » webchick

Since this needs backport and it's my patch, adding to webchick's queue.

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

I got a little bit turned around in the discussion above, but it seems like it was mostly kicking around the idea of a DrupalCacheArray pattern itself, which got implemented over in #402896: Introduce DrupalCacheArray and use it for drupal_get_schema() instead, and then subsequent discussion about this patch was just as an implementation of that pattern for the theme registry.

I read through the code comments of the latest patch and they seem pretty clear what's going on. The one thing I was curious about is what happens before/after this patch when you call theme('something_that_doesnt_exist'). And it seems the answer in both cases is "nothing" (I temporarily hacked index.php with theme('hello')) so that seems fine for a backport as well.

Committed and pushed to 8.x, however this doesn't apply to 7.x for some reason. Moving back to "patch (to be ported)".

pillarsdotnet’s picture

Status: Patch (to be ported) » Needs review
FileSize
9.44 KB

Status: Needs review » Needs work

The last submitted patch, theme_registry-1011614-85.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
9.47 KB

Changed the remaining cache() calls back to _cache_get_object().

catch’s picture

FileSize
8.72 KB

In D7 we should just use cache_*() functions instead of _cache_get_object(). Re-rolled for that, no other changes.

catch’s picture

#88: theme_registry.patch queued for re-testing.

catch’s picture

Assigned: webchick » Unassigned

Un-assigning webchick.

catch’s picture

Status: Needs review » Reviewed & tested by the community

This was a straight re-roll, most of which was done by pillarsdotnet, so I'm moving it back to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks!

Status: Fixed » Closed (fixed)

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

Alex Andrascu’s picture

Wich one of these patches can be safely applied to D6(.22) / Pressflow ?
Thanks

pillarsdotnet’s picture

None. Feel free to roll a backport.

catch’s picture

Just saw these comments. This relies on PHP 5.2, so it can never be backported to 6.x proper, but I'm maintaining a github fork with it at https://github.com/tag1consulting/pressflow6/tree/theme_registry_592008