Make theme registry cache collector lazyload

Tagging

Just some quick comments ...

+++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
@@ -38,10 +38,13 @@ class ThemeRegistry extends CacheCollector implements DestructableInterface {
   /**
+   * Whether all modules have already been loaded.
+   */
+  protected $modules_loaded;

@@ -51,32 +54,64 @@ class ThemeRegistry extends CacheCollector implements DestructableInterface {
+    $this->modules_loaded;
+  }

Note: We never assign the variable :) Also, maybe consider to use camelCase.

Overall looks very good! Nice work @lauriii.

1. +++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
   @@ -38,45 +39,90 @@ class ThemeRegistry extends CacheCollector implements DestructableInterface {
      /**
   +   * Whether all modules have already been loaded.
   +   */
   +  protected $modulesLoaded;
   

   I think this needs a @var with the type.

2. +++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
   @@ -38,45 +39,90 @@ class ThemeRegistry extends CacheCollector implements DestructableInterface {
   +        // to function correctly on non-registered theme hooks without triggering
   +        // a call to resolveCacheMiss().
   

   Super minor: 81 characters, could be rewrapped.

3. +++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
   @@ -126,34 +173,35 @@ public function resolveCacheMiss($key) {
   +    $storage = $this->storage;
   

   Just curious, why not getStorage here?

4. +++ b/core/modules/system/src/Tests/Theme/RegistryTest.php
   @@ -31,21 +31,24 @@ class RegistryTest extends KernelTestBase {
   +
   

   Minor: Extra blank line.

5. +++ b/core/modules/system/src/Tests/Theme/RegistryTest.php
   @@ -31,21 +31,24 @@ class RegistryTest extends KernelTestBase {
   +    $this->assertTRUE(\Drupal::cache()->get($cid), 'Cache entry was created.');
   

   Minor: assetTrue

+++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
@@ -58,7 +62,7 @@
-   *   The theme object representing the active theme for this registry.
+   *   The theme object representing the active theme for this registry. ¶

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

+++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
@@ -30,53 +30,90 @@ class ThemeRegistry extends CacheCollector implements DestructableInterface {
    *   The lock backend.
+   * @param \Drupal\Core\Theme\ActiveTheme $theme
+   *   The theme object representing the active theme for this registry.
    * @param array $tags
...
   function __construct($cid, CacheBackendInterface $cache, LockBackendInterface $lock, $tags = array(), $modules_loaded = FALSE) {

that doc seems wrong, I don't see $theme in there.

Yup the changes to that docblock need to be rolled back, @lauriii knows :)

undefined variable $lauriii

As we are updating the type of $completeRegistry, should we not do the same for $persistable ?

@lauriii : Sorry If I am wrong. My point was to document the variable as we did for other.

+++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
@@ -30,16 +30,33 @@ class ThemeRegistry extends CacheCollector implements DestructableInterface {
-  protected $persistable;

@lauriii: Please you can review now.

But as per #31, variable is undefined :)

1. +++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
   @@ -52,31 +70,52 @@ class ThemeRegistry extends CacheCollector implements DestructableInterface {
   +   * Gets the persistable.
   

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

2. +++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
   @@ -52,31 +70,52 @@ class ThemeRegistry extends CacheCollector implements DestructableInterface {
   +        }
   +        // RegistryTest::testRaceCondition() ensures that the cache entry is
   +        // written on the initial construction of the theme registry.
   +        $this->updateCache();
   

   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.

Overall:

+1 from me

Does the issue title/summary need an update now?

Looking good. Nice catch!

I 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()"

It always calls $this->getStorage(), can we optimize to do that only when !isset($this->storage)?

That 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?

1. +++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
   @@ -52,31 +70,52 @@ class ThemeRegistry extends CacheCollector implements DestructableInterface {
   +   * Gets the persistable.
   

   A persistable isn't really anything. Maybe write a one-line summary based on the property's description?

2. +++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
   @@ -52,31 +70,52 @@ class ThemeRegistry extends CacheCollector implements DestructableInterface {
   +      $this->persistable = $this->modulesLoaded && \Drupal::hasRequest() && \Drupal::request()->isMethod('GET');
   

   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.

3. +++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
   @@ -52,31 +70,52 @@ class ThemeRegistry extends CacheCollector implements DestructableInterface {
   +   * @return array
   

   array doesn't really mean much. What's in the array? Even mixed[] would be more explicit and specific.

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

Thanks @csg, setting to needs review so it gets tested.

+++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
@@ -46,37 +65,61 @@ class ThemeRegistry extends CacheCollector implements DestructableInterface {
+  protected function getPersistable() {

What about name it "isPersistable()"?

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

Scenario:

• 50 10 Nodes on the homepage generated with Devel Generate
• Xdebug off
• render caching off

=== 8.0.x..8.0.x compared (5578e29590a73..5578e2f4f233d):

ct  : 162,412|162,412|0|0.0%
wt  : 367,081|367,264|183|0.0%
mu  : 26,823,752|26,823,752|0|0.0%
pmu : 26,990,312|26,990,312|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5578e29590a73&...

=== 8.0.x..2472285-lazy-theme-registry compared (5578e29590a73..5578e32524136):

ct  : 162,412|164,985|2,573|1.6%
wt  : 367,081|380,250|13,169|3.6%
mu  : 26,823,752|27,398,648|574,896|2.1%
pmu : 26,990,312|27,498,200|507,888|1.9%

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?

Here's with caching on though I don't think it really shows much because everything is bypassed.

=== 8.0.x..8.0.x compared (5578e7733b8ce..5578e81d210fd):

ct  : 2,192|2,192|0|0.0%
wt  : 6,135|6,119|-16|-0.3%
mu  : 1,982,952|1,982,952|0|0.0%
pmu : 2,072,728|2,072,728|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5578e7733b8ce&...

=== 8.0.x..2472285-lazy-theme-registry compared (5578e7733b8ce..5578e832a56c5):

ct  : 2,192|2,192|0|0.0%
wt  : 6,135|6,148|13|0.2%
mu  : 1,982,952|1,983,144|192|0.0%
pmu : 2,072,728|2,072,960|232|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5578e7733b8ce&...

Whoops lied only 10 items on the homepage.

Patch re-rolled. All merged automatically during rebase. :)

+   *
+   * @var bool
    */
-  protected $persistable;
+  protected $persistable = NULL;

This should be

  /**
   * (doc description)
   *
   * @var bool|null
   */
  protected $persistable;

No need to initialize with NULL. But the initial null value needs to be documented in @var.

   /**
    * The complete theme registry array.
+   *
+   * @var array
    */
   protected $completeRegistry;

Same here, it should be $var array[]|null.
Why?
- array[] is more specific than just array.
- It can be null.

From the OP:

Theme registry creates the cache on the construct which is very early. Cache record should be created instead when the results are actually being needed.

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:

• EntityViewBuilder::getBuildDefaults().
• ThemeRegistryLoader::findTemplate().
• ThemeManager::render().

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.

Besides #86, the changes proposed in this issue would make ThemeRegistry even more complex than it is now.

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

This is still valid:

    // @todo Implement lazy-loading.
    $this->cacheLoaded = TRUE;

    if ($this->persistable && $cached = $this->cache->get($this->cid)) {
      $this->storage = $cached->data;
    }
    else {
      // If there is no runtime cache stored, fetch the full theme registry,
      // but then initialize each value to NULL. This allows offsetExists()
      // to function correctly on non-registered theme hooks without triggering
      // a call to resolveCacheMiss().
      $this->storage = $this->initializeRegistry();
      foreach (array_keys($this->storage) as $key) {
        $this->persist($key);
      }
      // RegistryTest::testRaceCondition() ensures that the cache entry is
      // written on the initial construction of the theme registry.
      $this->updateCache();
    }

is still in the ThemeRegistry::__construct().

In theory yes, but I think in practice it doesn't really matter.

While the constructor is doing a lot of heavy lifting here, the ThemeRegistry as a whole is lazy-instantiated in \Drupal\Core\Theme\Registry::getRuntime().

There might be some edge cases, but setting a breakpoint in the constructor, it's called through \Drupal\Core\Theme\ThemeManager::render(), immediately before the has/get calls. There are only 3 non-test calls to getRuntime() in core and all are immediately followed up by has() or get() call.

Which is not surprising, as that is the only reason to get the runtime. If it were a standalone service it would be different, but right now, from a performance perspective, there doesn't really seem to be anything to gain here. I'd suggest at least setting it from major to normal or even minor cleanup.

Yes that's a good point, let's bump it to normal. I wonder a bit if we can re-integrate this all into one class, although it's potentially tricky with multiple theme support.