Abstract RenderCache into a separate service that is capable of cache redirects in a non-render array-specific way

#2479459: BlockRepository::getVisibleBlocksPerRegion() does an uncached entity query on every request would be a possible use case for this.

So maybe something like this?

Bumping to 8.1.x due to feature request nature. Triaging majors.

I agree, this is by no means major.

The Workspace module would also benefit from this if we'd be able to make the entity storage derive its persistent cache keys from the cache contexts of the request.

Just for reference, this was discussed in #2784921-164: Add Workspaces experimental module, point 12.

Going to take this PoC and clean it up a little to add to Group. If it works well there, I'll let you know.

My current plan is to create a ContextCacheFactory service which has a get method that takes the name of a cache backend service. That would then return a polished version ContextCache from Daniel's proof of concept, wrapping said cache backend so it works with cache contexts and redirects.

Or does anyone have a better suggestion?

Okay so naming things is always hard :) Will start of with VariationCache or CacheContextAwareCache as I recall a core test recently being renamed from BubblingSomething to CacheContextSomething.

The factory I had in mind would be declared as such:

  variation_cache_factory:
    class: Drupal\Core\Cache\VariationCacheFactory
    arguments: ['@cache_factory']

Then you could "create" a cache context aware version of any cache backend like so:

  variation_cache.static:
    class: Drupal\Core\Cache\VariationCacheBackendInterface
    factory: variation_cache_factory:get
    arguments: [static]

VariationCacheFactory::get($bin) would simply invoke CacheFactory::get($bin) and wrap its return value in a VariationCache object.

Please note: For this to work nicely with the recently added entity.memory_cache service, it will need to be properly declared as a cache backend. See how I handled this for now in Group:

  # Recently, core introduced a new static cache that does not serialize its
  # data, unlike the cache.static service. This entity.memory_cache service has
  # two major drawbacks: First of all, it is named confusingly as it can work
  # for almost anything, not just entities. Secondly, it is not declared the
  # way cache backend services should be declared, meaning it cannot make
  # proper use of cache tags. In order to fix this, we declare the cache and
  # its factory the right way, albeit prefixed with group_ so that we do not
  # collide with core if it ever fixes this.
  cache.backend.group_memory_no_serialize:
    class: Drupal\group\Cache\MemoryCacheFactory
  cache.group_static_no_serialize:
    class: Drupal\Core\Cache\CacheBackendInterface
    tags:
      - { name: cache.bin, default_backend: cache.backend.group_memory_no_serialize }
    factory: cache_factory:get
    arguments: [group_static_no_serialize]

But there are other issues that (sort of) handle that:

• #2973286: [PP-1] Clean up MemoryCache and MemoryBackend naming issues
• #3016690: Audit usages of cache.static

Shall I create a dedicated issue that handles the above?

Just reflecting that there's some code here, I keep thinking this is a duplicate of the issue with a patch on it.

Here's the relevant part of the huge patch found here: #3041087: Update the new permission layer to be alterable.

Ignoring the obvious fact that the classes are namespaced for Group right now, it should give you a general idea of the approach I've taken. Oh and the best part is: It actually seems to be working :)

If you're looking for a concrete example, scour the massive patch in the other issue for the ChainGroupPermissionCalculator class.

Let's see what this does while I'm away for lunch. Should largely work, but I need to do some further investigation whether I got all bases covered and the following line in ::set() needs love as the old CID-comparing didn't take cache tags into account:

    if ($elements['#cache'] !== $pre_bubbling_elements['#cache']) {

Forgot to actually inject the new cache factory.

For working on such a complex issue I sure am making a lot of rookie mistakes here...

Oh for the love of...

Sorry for all the noise.

518 test failures :o

Well, at least we have something to work with now :)

Really nice work. The patch itself looks pretty good. Didn't play around with it yet, though.

Noticed one thing:

+++ b/core/lib/Drupal/Core/Cache/VariationCache.php
@@ -0,0 +1,171 @@
+      return FALSE;

It seems the callers of this method do not account for this case.

pretty good

Wow, I guess today is understatement day. What I meant to say instead: really impressive (!) and surprisingly elegant for such a complex topic

Sorry for the glibness...

Re #28 Which return value would that be? It's not entirely obvious from the context.

This should fix some tests, I also type-hinted the cache keys and cleaned up the cache key comparing logic to do exactly the same as before through use of a public method on the variation cache.

Re #30: I was talking about VariationCache::createCacheId():

+++ b/core/lib/Drupal/Core/Cache/VariationCache.php
@@ -0,0 +1,180 @@
+      $cid = $this->createCacheID($redirect_cache->data['keys'], $redirect_cache->data['cacheable_metadata']);
...
+    return $this->cacheBackend->get($cid);
...
+    $cid = $this->createCacheID($keys, $cacheable_metadata);
+    $this->cacheBackend->set($cid, $data, $this->maxAgeToExpire($cacheable_metadata->getCacheMaxAge()), $cacheable_metadata->getCacheTags());
...
+  protected function createCacheID(array $keys, CacheableMetadata &$cacheable_metadata) {
...
+      return FALSE;

Here it is with some more context

I've refactored parts of RenderCache to account for the changed approach to how it should call the cache. It was currently using the same cache keys for both the redirect and the actual cache item.

Also made sure the test case calls the cache correctly, no longer tries to test a method that was removed and some other minor test fixes. Hopefully this will make fewer tests fail now.

Re #33 ah, you're right. That was for a short-circuit at the top of RenderCache::get() and ::set(). It seems to be unused now but maybe that served a purpose: If we can't create a cache ID, we shouldn't try to get or set the cache entry.

I should update VariationCache to include checks for whether the cache ID is not FALSE so the behavior remains unchanged. The only difference being that this won't short-circuit RenderCache as early as it used to because it now first needs to call the cache.

Over a 100 failures fewer 🎉

@tstoeckler Looking at the test failures, it seems what you were on about in #28 is causing a lot of failures. So we definitely need to refactor that part too. But I'm out of time this week.

Attached is a patch that should fix some more misuse of the new cache.

Fixed a few more test cases and fixed the render cache calling the backend when it didn't use to.

The VariationCache::createCacheId() method now always expects valid keys and no longer returns FALSE as that was behavior unique to the RenderCache (keys might be missing) and cache backends should be able to rely on the fact that they're being called correctly.

Introduced RenderCache::isElementCacheable() to reintroduce the short-circuiting behavior to RenderCache.

Added some more test fixes and added ::delete() and ::validate() to the variation cache while also changing ::set() to only set a redirect if need be.

It does seem like an awful lot of core tests call the render cache bin directly rather than going through RenderCache, making this a tough nut to crack.

Well if we're going to make silly typos, of course tests are gonna fail. Let's see what happens next.

Wait, so I reran the tests without the change from the interdiff? What a nice way to spend my time.

Found another culprit! We used to store the cache redirect by the pre-bubbling CID and now I simply passed the keys and the cacheability of the post-bubbling element, effectively caching it under the post-bubbling CID.

I realized we can actually store the redirect indefinitely, so I did exactly that but did ensure we use the pre-bubbling contexts (and thus CID).

Hmm only 5 fixes :/ Back to the drawing board I guess.

So the things we were caching got influenced by the cache context folding, which is no longer the case. To fix that, I made sure the potentially updated cache tags and max-age from the actual variation cache got merge back into the cache object's #cache data when retrieving a render array from the cache.

🎉🎉🎉

I still have some ideas as to what might be wrong with the last few tests, but this is some amazing progress already.

Fixed a regression regarding the rendered cache tag and a bug where I would only update the cache keys for the redirect if the redirect got updated.

Make sure the cache tags we retrieve from the cache have a proper incremental index after filtering out the 'rendered' tag as some tests were doing exact matches. You could argue that PluginTest should have used assertEquals() instead of assertEqual() as I believe the former ignores numeric indexes and would thus not have caused a test fail.

Also set all of the combined cache tags and max-age on the cache redirect like the old system used to. I first figured we could cache the redirect indefinitely, but it seems the tests disagree. I can think of one edge case where this would make sense, so trying things out with the old logic.

I think I know what broke TrackerTest (and potentially others). RenderCache::set() used to update $elements cache tags because it folded the cache contexts and got $elements by reference. The new version doesn't, but properly folds the cache contexts only when setting the cache item.

So the X-Drupal-Cache-Tags header no longer contains cache tags that were only added after cache contexts were folded. The X-Drupal-Cache-Contexts header remains unaffected.

I don't know about you, but I find it rather dirty that a cache's set method affects the thing it's caching for the rest of the runtime. That said, the whole #cache property needing to be a live copy of the cache metadata -even though the actual cache should be responsible for anything cache related- is the real culprit here.

I'm thinking of not allowing ::set() to affect the object it's caching, but to add the cache tags it finds during cache context folding directly to the response. That should allow the tests to pass, without needing every variation cache invocation to account for what might happen when they call ::set().

It should also fix most remaining test fails. The only thing it won't do is affect the #cache metadata during the current runtime. I don't know how much code depends on that being updated as you go, but I'm hoping none as it's a rather nasty behavior to expect in the first place.

Edit: The question then becomes whether we should also add the tags from ::get() to the response rather than back to the render array. Will investigate that last (after fixing ::set()).

I've removed the render cache (ab)use from DynamicPageCacheSubscriber to see what that does. I've also fixed a test case.

The idea here is that we should not set the cache's optimized tags in RenderCache during ::get() or ::set(). So if this subscriber thingy works out well, the last move would be to update FinishResponseSubscriber to consistently set X-Drupal-Cache-Tags and X-Drupal-Cache-Contexts.

I say consistently because now it shows you the contexts after optimization, but cache tags before optimization unless the cache tags came from optimization in RenderCache and bubbled up to the response. Which makes no sense at all because anything coming from other places such as route access checks are not properly represented.

So my next patch will aim to set X-Drupal-Cache-Tags to the response's tags plus the ones that come out of cache context optimization and the X-Drupal-Cache-Contexts to the optimized contexts (remains unchanged).

Setting the cache's tags on the render array during ::get() did fix 285 test failures but I'm hoping the above will do the same.

This should remove the clutter from the changes in the last patch.

This should fix the other fails, although we might need tor evisit the tests for EntityResourceBase (jsonapi) in a follow-up because its logic wasn't updated to only account for one cache entry, unlike rest's EntityResourceBase.

Great work here! I had totally forgotten that Dynamic Page Cache currently (ab)uses the render cache redirect system, that alone makes this issue such a great cleanup, regardless of further possibilities. The subscriber looks so much more readable with the patch, really nice.

+++ b/core/modules/dynamic_page_cache/src/EventSubscriber/DynamicPageCacheSubscriber.php
@@ -194,10 +172,23 @@ public function onResponse(FilterResponseEvent $event) {
-    $this->renderCache->set($response_as_render_array, $this->dynamicPageCacheRedirectRenderArray);

This line used to cause redirects to be stored because the cache contexts would almost always differ. We don't have that now, so the variation cache tries to find stuff varying by the contexts of the last response that was saved.

So it used to benefit not only from the RenderCache's cache context resolution, but also its redirect resolution, meaning everything stored under the 'response' cache key would eventually vary by the maximum amount of cache contexts that ever bubbled up to the response.

So because the response doesn't always vary by the same contexts, we need a new type of caching wrapper that also handles redirects the way RenderCache does (and then feed RenderCache that cache wrapper).

I've removed the self-healing strategy from RenderCache and put it in VariationCache as other systems relied on it as well (see #67). This makes RenderCache a really clean wrapper around the variation cache.

I expect this to break a lot more things and there are some todos here:

1. Remove the extra cache tags logic from RenderCache::get() again.
2. Rewrite the long ass text about self-healing in RenderCache::set() and move it somewhere else where it makes sense now
3. Write tests for VariationCache and move all tests related to redirects and self-healing into that test
4. Rewrite FinishResponseSubscriber (see #59)

Edit: Also need to remove sharesCacheKey() as it no longer serves a purpose

Wow, that went far better than expected. These small fixes should reduce failure noise even more.

Some weird errors here:

• demo_umami_content's UninstallDefaultContentTest installs banner blocks, but relies on its config/optional folder to actually place them. This no longer seems to happen as the front page doesn't contain said block's text any more. I do not see how changing the caching system would affect this. Unless it is served a stale cache of the front page before checking for blocks and the placement of blocks should have cleared the front page cache?
• NodeBlockFunctionalTest expects a page with a block that varies by user to be cacheable (which it should not!) and used to get these results. However, every check from line 150 forward should always return UNCACHEABLE. Unless there is some placeholdering going on that I'm unaware of and is now broken?

I did notice DynamicPageCacheIntegrationTest is failing because the self-healing being moved into the variation cache. It used to HIT because RenderCache didn't need a first redirect to find the cache ID, i.e.: it would construct it itself because render arrays know more about their cacheability than they should during cache gets. Now, the self-healing has moved to an earlier phase.

Old scenario:

1. Ask for route foo first which varies by nothing special
2. Response is stored under response:foo:GET
3. Ask for route bar which varies by animal
4. Self-healing! Redirect is stored under response:bar:GET
5. Response is stored under response:bar:GET:pig
6. Visit route foo again, render cache thinks the cache ID is response:foo:GET and finds it
7. Visit route bar again, render cache thinks the cache ID is response:bar:GET and finds a redirect to response:bar:GET:pig
8. Render cache queries response:bar:GET:pig and finds the response

New scenario:

1. Ask for route foo first which varies by nothing special
2. Redirect is stored under 'response'
3. Response is stored under response:foo:GET
4. Ask for route bar which varies by animal
5. Self-healing! Redirect is still stored under 'response' but updated to also account for 'animal'
6. Response is stored under response:bar:GET:pig
7. Visit route foo again, variation cache queries 'response' and finds a redirect to response:foo:GET:pig
8. Variation cache queries response:foo:GET:pig and finds nothing
9. Visit route foo again, variation cache queries 'response' and finds a redirect to response:bar:GET:pig
10. Render cache queries response:bar:GET:pig and finds the response

So as you can see the only reason this worked (and was more efficient) under RenderCache was because we would tell RenderCache what contexts to use during ::get().

In order to maintain this more efficient approach of only storing redirects for those variations that deviate from the original, we would have to rewrite VariationCache to always ask for the "original cacheability" during ::get() and ::set(), which would make the solution less elegant.

So it's either:

• Maintain clean API and take a performance hit because DynamicPageCache now varies by the sum of all possible cache contexts used
• Rewrite variation cache to always ask for original cache metadata so that we can do more efficient redirects

I'll try the second to see how many remaining test fails are solved that way, but it might be less elegant :(

Come to think of it, RenderCache was only more efficient on the first tier of caching.

Suppose you have a render array that outputs your "housing situation".

1. It initially varies by housing type: apartment or house
2. If it's a house it varies by garden: no-garden or garden
3. if it has a garden it varies by house orientation: east, south, west or north

Output would be one of the following scenarios:

• You have a nice apartment!
• You have a nice house!
• You have a nice house with a north-facing garden!
• You have a nice house with a east-facing garden!
• You have a nice house with a south-facing garden!
• You have a nice house with a west-facing garden!

Any other possible combination still exists, but just wouldn't affect the output.

In the current variation cache, we'd eventually see the following reachable cache entries:

Not really efficient, right? There would also be some cache entries that can no longer be reached (e.g. keys:apartment) depending on which entries were stored first.

In the old RenderCache, we'd eventually see the following cache entries:

As you can see, any house without a garden is still stored inefficiently and would also possible have some "dead" cache entries.

So what we're trying to reach is this:

This is actually possible if we store the minimal shared contexts between redirects at every step of the way rather than the total sum of contexts up front.

So in the above scenario we'd end up with the following cache redirects:

Meaning that the retrieval of an apartment only needs to follow one redirect, a house without a garden needs to follow two and a house with a garden three. This could prove to be a performance hit. The upside is that your cache is now far more efficient and will contain no dead entries whatsoever!

Any thoughts on this?

This should fix the DynamicPageCacheIntegrationTest failure in mentioned in #72. It also takes a new approach to variation caching as outlined in #73 by storing several redirects for complex cache entries.

If everything works out great, we're down a test fail. If this breaks something I did not foresee (ReousrceTestBase jumps to mind), we'll see how bad it breaks :o

I probably still need to further dumb down RenderCache and expand FinishResponseSubscriber as mentioned in #68.

Because getting it right the first time is too much to ask for :D

This adjusts FinishResponseSubscriber, which should fix RendererBubblingTest and TrackerTest, but will probably break other stuff. I also rewrote the self-healing tests already in preparation of removing it entirely in favor of VariationCache tests (which will also check for self-healing).

Great so they got fixed!

The new REST/JSONAPI fails are because the user.permissions cache context is being optimized away and X-Drupal-Cache-Tags and X-Drupal-Cache-Contexts now properly reflect that. So those tests need updating.

This leaves only a few failures, which I'm hoping to fix by moving the FinishResponseSubscriber rework to DynamicPageCacheSubscriber so it happens earlier and is available to all subscribers that should have the updated contexts/tags.

This should fix a few more tests (and again might break thousands). I'm expecting to see the rest/jsonapi stuff from #80 still broken because I haven't put in a fix for those yet.

Aha! It turns out X-Drupal-Cache-Contexts is set too late!

Cache contexts only apply to the dynamic page cache, which runs at $events[KernelEvents::RESPONSE][] = ['onResponse', 100];. However, we do have RESPONSE subscribers that add cacheability after that. This cacheability is still used by page cache, but since that system only leverages cache tags, any cache context added after DynamicPageCacheSubscriber is irrelevant and should thus not show up in the header.

So what I suggest is this:

• We fold cache contexts in DynamicPageCacheSubscriber and output X-Drupal-Cache-Contexts there.
• We allow other subscribers (such as ClientErrorResponseSubscriber, FinishResponseSubscriber, ...) to add more tags
• We still output these cache tags in FinishResponseSubscriber's X-Drupal-Cache-Tags because this is now the full set that will be used by PageCache. Although one could argue we should now ideally output it in PageCache instead to be consistent. Who knows what negatively weighted KernelEvents::RESPONSE might still add tags?

Please note: RouteAccessResponseSubscriber will also still be capable of adding cache tags, but any cache contexts added there (by running $response->addCacheableDependency($access_result);) will be disregarded in the header because it is too late there to actually do anything.

Maybe in the future we should have three headers?

• X-Drupal-Dynamic-Page-Cache-Contexts
• X-Drupal-Dynamic-Page-Cache-Tags
• X-Drupal-Page-Cache-Tags

The above would be the most accurate way to represent what is actually being used in the caching layer.

I've moved the contexts debug header to the more logical place. A lot of tests extending ContentTranslationUITestBase were failing but it seems many of them were specifying incorrect cache contexts.

Sigh, service definition fell through the cracks of patch creation...

The problem we're facing here is that the debug headers contained incorrect data but a lot of tests actually queried for (and found!) the right data. The only reason this worked was because RenderCache bled context-folding-affected cacheability up to the header level, even though it shouldn't have. I've since exposed (and fixed) this, but ideally we should have caught and fixed that bug first in a separate issue.

Can still split up the final patch to tackle both issues separately, I suppose.

Okay, so take Cache.Drupal\Tests\node\Functional\NodeAccessCacheabilityTest for instance: It fails because of these lines:

    // User should be able to edit/delete their own content.
    // Therefore after the access check in node_node_access the user cache
    // context should be added.
    $this->drupalGet('node/' . $node1->id() . '/edit');
    $this->assertCacheContext('user');

'user' is no longer in the header because the header is now set in DynamicPageCacheSubscriber, given how cache contexts have zero impact on caching after that point. Yet, RouteAccessResponseSubscriber adds the user cache context to the response, after DynamicPageCacheSubscriber.

So even though the user cache context will be set by the time the page reaches PageCache, PageCache does not care about cache contexts (and shouldn't!) which begs the question: Why is NodeAccessCacheabilityTest even checking for the user cache context in the first place? It won't do anything at all.

The only time that 'user' could be set on the actual build is if it were added manually or an access result with the context was added to '#access', which makes Renderer::doRender() add it as a dependency to the build.

Footnote:
The only exception to the above that I could find is AnonymousUserResponseSubscriber, which checks the contexts after DynamicPageCacheSubscriber so that it could optionally add a cache tag to the PageCache entry. But that one checks for 'user.permissions', not 'user'.

Right, so we are definitely going to want to "fix" the header issue we have going on here in a separate issue. For that reason I have restored the original headers to their original location (FinishResponseSubscriber) but added two new ones already that properly reflect the actual cache contexts that are being used. I might need to revert a few test adjustments left and right now or make them use the new headers.

For the record, we now have:

• Dynamic page cache:
  • X-Drupal-Dynamic-Cache-Tags: All tags that are used for dynamic page cache
  • X-Drupal-Dynamic-Cache-Contexts: All (folded!) contexts that are used for dynamic page cache
• Page cache:
  • X-Drupal-Cache-Tags: All tags that are used for page cache
  • X-Drupal-Cache-Contexts: Purely for debugging, no more folding happened past DPCS, so ['user', 'user.permissions'] is a possible outcome

Just to say #73 is really useful, but I'm having trouble thinking through what the real-world implications are in terms of hit rates vs. cache redirects (probably it would depend a lot on what the actual variations and traffic patterns are).

This whole header thing is really getting in the way. There are so many unrelated test fails in this issue because:

• Our headers are wrong
• RenderCache used to bleed info to said headers it shouldn't have

Created a spinoff issue here: #3052395: Our cacheability debug headers show completely wrong information. If we can commit that to 8.7.x and 8.8.x already we might have a better shot at fixing this issue without so many random test failures.

Re #91 that's where the performance experts should come in :) Every get will be slightly more expensive for complicated cases only. So only if you have three layers of cache contexts bubbling up, will the retrieval have to go through extra steps.

Old render cache

• Without contexts: 1 get
• With contexts but no bubbling contexts: 1 get (because of #cache being a hacky way of roughly knowing how things got cached)
• With bubbling contexts: 2 gets

New variation cache

• Without contexts: 1 get
• With contexts but no bubbling contexts: 2 get (because we don't have #cache and need to follow a CacheRedirect)
• With bubbling contexts: 2 gets + 1 get per nested context level, but only if the context values lead you down such a redirect trail

So as you can see, we already take an inherent hit by dropping the expectation of #cache being present so the variation cache can be used for storing items other than render arrays. Any hits beyond that only happen if there are multiple layers of redirects possible and the active context values point in that direction.

hmm I think we should stick with the current method to be honest, that could add up to a lot of cache gets on a single page and while theoretically the number of variations could be a lot higher, it's very dependent on traffic.

Okay so given how I've been sidetracked by this whole header thingy, here's what I want to do:

• Remove all header changes here and tackle those in #3052395: Our cacheability debug headers show completely wrong information
• Only focus on the cache changes and accept that a lot of tests might fail because they expected to see cache tags coming from a context optimization in the headers. This only happened because RenderCache affected the very thing it was supposed to cache.
• Update said tests to no longer expect the render array that was cached to be influenced in any way
• Consequentially, update said tests to no longer expect the updated cacheability to bleed through to the headers

@catch in #94: We can't stick with the old system 100% because that one used to cheat by having #cache available to it. So we will see an increase to at least 2 gets when there are contexts (2nd bullet for variation cache in #93).

We can try and stick to the self-healing mechanism from RenderCache from that point on, but that would increase the second bullet point from #93 to 3 gets as well:

1. Follow the keys, get a redirect
2. Follow the redirect, find a "self-healing redirect"
3. Follow this redirect to the end result

So by sticking to the old self-healing system, we need 3 GETs for every cache retrieval in a self-healing scenario. With the new multiple-redirect system we also have 3 at minimum.

So the question is: Do we opt for a system with a far better hit ratio and more efficient storage that might need to do more retrievals as soon as we have 2 or more layers of bubbling contexts? Or do we stick to the old, less efficient system but still accept that from now on everything will need 3 lookups?

Either way we can't get around the extra lookup. What we can make sure is that, with the new system, an object with only top-level contexts will still only need 2 lookups instead of 3.

No interdiff because this basically undoes a lot of try-and-error bughunting with regard to the test fails. I know now that whatever fails we get regarding missing tags we need to adjust to no longer expect tags the bled through from RenderCache.

A lot of similar failures in Hal, jsonapi and Rest and no more weird Umami failure. I call that a win.

Well this was fucking stupid of me.

// Always returns max-age 0 because CacheableResponseInterface doesn't extend CacheableDependencyInterface.
$cacheability = CacheableMetadata::createFromObject($response);

// The below does work.
$cacheability = CacheableMetadata::createFromObject($response->getCacheableMetadata());

Also moved the context folding after the max-age check because max-age cannot be affected by it and we thus save a performance hit in case of max-age 0 scenarios.

The last one is easy.

TrackerTest tests code that never adds the user cache context to its content. What it does do, however, is add the user cache context to the local tasks. But those get properly placeholdered now and thus the optimized user.permissions cache context's tags never makes it to the response level (and thus cacheability headers).

Expecting fully green tests now. Adding "Needs tests" tag for adding unit/kernel tests to the new VariationCache.

🎉🎉🎉 FINALLY 🎉🎉🎉

Now that I can prove this works, I can already use it in Group and get a release out the door at long last.

This adds a performance boost and unit tests showing that the variation cache works (which we already knew). Nevertheless, it makes it easier for people to understand what's going on inside the variation cache.

This should still go green and as far as I'm concerned is finished. I will be using this version in Group while waiting for it to be committed to core. Unless all of the sudden tests start failing, that is. Which they shouldn't :o

Overall this is looking very good, if there are remarks in here that have been resolved earlier I'm sorry about that. Most of these are nitpicks about documentation.

1. +++ b/core/lib/Drupal/Core/Cache/CacheRedirect.php
   @@ -0,0 +1,29 @@
   +    // Cache redirects only care about cache contexts.
   +    $this->cacheContexts = $cacheability->getCacheContexts();
   

   I'm not sure about this comment, we should probably explain why or remove the comment. Because all this does is make me confused.

2. +++ b/core/lib/Drupal/Core/Cache/VariationCache.php
   @@ -0,0 +1,209 @@
   +class VariationCache implements VariationCacheInterface {
   

   Needs docblock.

3. +++ b/core/lib/Drupal/Core/Cache/VariationCache.php
   @@ -0,0 +1,209 @@
   +  public function get(array $keys) {
   

   I'm not sure if I like just calling this operation get(), since that is not very telling. Especially when doing VariationCache::get($keys) somewhere else. Should this be getRedirect, or is this not what this does?

4. +++ b/core/lib/Drupal/Core/Cache/VariationCache.php
   @@ -0,0 +1,209 @@
   +  public function set(array $keys, $data, CacheableDependencyInterface $cacheability) {
   

   Same remark

5. +++ b/core/lib/Drupal/Core/Cache/VariationCache.php
   @@ -0,0 +1,209 @@
   +  public function delete(array $keys) {
   

   This method is also not super clear. We're deleting only the last item from the chain, but because we are passing in an entire array of keys it sounds like the entire chain will be deleted and not one item.

   I think a clearer method name can help here.

6. +++ b/core/lib/Drupal/Core/Cache/VariationCache.php
   @@ -0,0 +1,209 @@
   +    if ($contexts = $cacheable_metadata->getCacheContexts()) {
   

   I don't really like assignments in if statements. But that is just personal preference I guess. Just wanted to mention that I think splitting this in 2 lines can make this somewhat easier.

   $contexts = $cacheable_metadata->getCacheContexts();
   
   if (empty($contexts)) {
   return implode(':', $keys);
   }
   
   ...
   

7. +++ b/core/lib/Drupal/Core/Cache/VariationCacheFactory.php
   @@ -0,0 +1,64 @@
   +class VariationCacheFactory implements VariationCacheFactoryInterface {
   

   Needs docblock.

8. +++ b/core/lib/Drupal/Core/Cache/VariationCacheFactory.php
   @@ -0,0 +1,64 @@
   +  public function get($bin) {
   

   Same remark as earlier: getBin?

9. +++ b/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php
   @@ -313,24 +293,25 @@ public function providerTestContextBubblingEdgeCases() {
   +   * @todo Revisit when we have self-healing tests for VariationCache. This is
   +   * essentially a clone of the other bubbling tests now.
   

   Is this something you'd want to tackle here as well? I think we should probably already open a followup and link to it in the code?

Re #105: Thanks for your review! Answers below.

1. We could add an @see VariationCache::createRedirectedCacheId(). It clearly shows only cache contexts are taken into account.
2. You're right, need to add a docblock.
3. The ::get() actually gets the item you stored from the cache. You do not need to care about whether it uses any redirects to get to the final result, ::get() takes care of that internally.
4. Same answer :)
5. I added ::delete() and ::invalidate() because some core tests were using it to make up for the fact that they leaked state. The interface clearly states that it only deletes/invalidates the entry that you would get from a ::get() call. I.e.: It takes the active cache contexts into account. I'd argue this is a bonus as you can target the item specifically for the active user. In any other case, I'd advise you to clear multiple items through cache tags.
6. It's down to personal taste, as the coding standards don't dictate either way. I like it the way it is :P
7. You're right, need to add a docblock.
8. This mimics the behavior of CacheFactory, which also names its factory method ::get()
9. I'd rather tackle that one in a follow-up to keep the patch here as clean as possible. The tests still go green, they might just have become redundant thanks to VariationCacheTest.

@catch in #94, I just thought of another amazing benefit to the new redirect system. In the current system, you are not allowed to vary a local action by a cache context with a parameter that has many options. In the new system, it would not be a problem.

An easy example is a fictional "Node Bookmarks" module with context user.has_bookmarked_node:<NODE_ID> returning '0' or '1'. If you want to show a local action saying "Add bookmark" or "Edit bookmark" to every node page you'd effectively kill caching altogether because the local tasks block would keep adding the context for every node to the ever-growing RenderCache redirect and start varying every page by said contexts.

In the new system, we no longer have this notion of ever-growing cache contexts, but would instead have to perform only one extra ::get() on every node page that uses the bookmark module. Nodes of node types that don't use the module wouldn't even have to perform said extra ::get()!

This opens up so many options as we can finally use any cache context with a potentially high amount of parameter options without having to worry where it'll end up in the render tree and whether it will kill caching.

So to sum up the benefits of the new system:

• Items are only cached by what they vary by, instead of also what something unrelated that used the same keys so happened to vary by
• Ergo: We have a higher cache hit-ratio because items only vary by what they specified they vary by
• This completely eliminates duplicate cache entries (Last example in #73)
• Ergo: We have better performance because items don't need to be recalculated when the redirect couldn't find a duplicate entry
• We have a higher cache hit-ratio because redirects never need to be cleared
• This cache is possibly more performant in high-variation scenarios where an expensive context is used because only those items that use said context will call it. In the old system, all cache items would call it.
• The new system is possibly more performant across the board because of all of the above: higher hit-ratio, redirects being stored permanently (they get cleared very often in RenderCache), items only calling the contexts they used

I'd really like to get rid of the old system in favor of this new one as it:

• Allows us to more freely use any context we like
• Stops filling our cache with duplicate entries
• Is simply more elegant and easier to understand than an ever-growing redirect

TL;DR: I think a performance hit of sometimes having to perform one extra get is far more acceptable than the current performance hit where we completely recalculate whole render trees that were already cached, simply because we couldn't find it in the cache.

Whether it's faster or slower, we need to know how much...

I've never done benchmarks in my life. Any information on how to execute them with Drupal 8?

Well, there are two things:

One is single-request comparison, you can for example use blackfire for that, on cache hits (with and without dynamic page cache I'd say) and also misses.

The other is how it behaves across multiple requests, which will be much more challenging but at least as important.

I am eager to benchmark this but due to lack of time I can not at the moment...

For Drupla 8 there are currently 2 ways to test a single request (as @Berdir mentioned).
- One is locally (no service registrations) with tideways + xhprof (DrupalVM has that prepared). Then install the Drupal's xhprof module and you are good to go.
- The other way is the BlackFire that does essentially the same but you need account on their site. I have not used that yet though.

In the end it will expose information about CPU time and RAM usage.
Once you have profiled the no-patch warm and cold caches and the same with the patch, you can compare and decide are the changes OK or not.

Note that any benchmark tool will add overhead due to the data collection routines.
Some PHP extensions have a high overhead as well, for example x-debug. Whenever bench-marking something it's a good thing to stop the x-debug extension fully from php (not just disable all breakpoints).

So in the end you can test the warm cases with the simplest AB tool that will just bombard a URL with some amount or requests.

ab -n 100 -c 1 http://example.com

This will hit example.com a hundred times with a single client thread and present you with a nice aggregated results.

That's pretty much all that I am using when in need to do benchmarks.

Thanks for the thorough explanation! I just got Blackfire working and will create a few pages that mimic the things we should definitely be testing. For making sure the caches are cold, I can use an event subscriber that clears certain cache tags on request.

Result 1: Warm caches, logged in as an admin, getting a simple node page.

WITHOUT patch:

• https://blackfire.io/profiles/4e741add-f948-4e83-8beb-98d632a0420e/graph
• Puts more stress on the renderer
• Slightly faster cache

WITH patch:

• https://blackfire.io/profiles/b4e2acbf-f65a-4f88-a30a-4e2030f7f5e6/graph
• Puts more stress on the cache (because better cache-hit ratio)
• Slightly slower cache due to many calls to the cache contexts manager

As you can see, the new cache does not affect load time for simple requests. What I did find was that you can clearly see how the case without the patch puts more stress on the renderer whereas the profile with makes the cache work harder.

If we can optimize CacheContextsManager::convertTokensToKeys() and CacheContextsManager::optimizeTokens() to statically cache its results for the duration of the request, then we should definitely see the new approach beat the old one.

Seeing similar result for other "simple pages": Same results or the new system being ever so slightly slower (1ms-5ms on a total of 1000-1200ms).

Keep in mind that this was to be expected. The new cache should definitely outperform the old when it comes to not having to render things we still have in cache (better hit ratio) and not having to call cache contexts that don't apply. These things are harder to test with blackfire because they usually span multiple requests, not just a single one.

I'd have to develop a custom page for this. Going to create one that works like #73

These results are from the complex case from #73 where I included a 20ms usleep() for every render call. This is very generous depending on what you're rendering.

• Without: https://blackfire.io/profiles/aa6c4d3e-858f-494b-bafe-ae66369bfe4e/graph
• With: https://blackfire.io/profiles/08ecf0e6-a982-45a9-9910-77783df323b1/graph

Because of the higher cache hit ratio, the profile without the patch called usleep() 36 times whereas the one with the patch called it only 13 times! This clearly indicates that the volatility of our current redirect system (i.e.: it clears too much, too often) will cause reduced performance on a site that introduces new content on a regular basis.

It also shows a much higher cache hit ratio as:

1. The cache warming caused 12 usleep() calls
   • Old cache: 12 calls
   • New cache: 12 calls
2. The new cache could immediately find said items whereas the old one couldn't due to the expanding redirect. So that's another 12 usleep() calls.
   • Old cache: 24 calls
   • New cache: 12 calls
3. The new cache could find all but one item after clearing a single item's tags vs the old cache couldn't find any.
   • Old cache: 36 calls
   • New cache: 13 calls

During this entire process the old cache didn't get a single hit, whereas the new one got 23 out of 24 hits!

If you want, I can upload the module I used to test this. But I don't want to attach even more files here unless necessary.

For #115 do you have numbers for a fully warmed cache?

As there are now profiles available I am dropping the needs benchmarks tag.
Added more related tags.

@catch in #115 Will try and get those numbers too. Will probably be early next week.

@catch re #116

Performance with a fully warmed cache, only retrieving:

• Without patch: https://blackfire.io/profiles/540c14f1-12a1-40b5-a465-739ce94bf4a3/graph
• With patch: https://blackfire.io/profiles/5b69e506-336a-4bae-9aac-a5bb66e1d6ae/graph

Again shows a marginal increase (+1%) in time spent (1.56s vs 1.58s) on a page with multiple complicated retrievals and perfectly warmed caches. Given how rare that actually is with the old caching system (as a single invalidate would "soft invalidate" the whole lot), I'd say the new system is worth the absolutely minor performance hit in favor of the increased hit ratio and possibility to now really use cache contexts with parameters (as detailed in #107)?

Looks like the 'without' case is doing 48 merge queries so might not have been a fully warmed cache?

I added usleep() calls in the Renderer. So if something was not retrieved, it needed to be rendered and as such would call usleep(). What you're seeing is my way of changing cache context values during runtime:

protected function renderRedirectedElement($house_type, $garden_type, $house_orientation) {
    $this->state->set('house_type', $house_type);
    $this->state->set('garden_type', $garden_type);
    $this->state->set('house_orientation', $house_orientation);

And every context looks like this:

  /**
   * {@inheritdoc}
   */
  public function getContext() {
    return $this->state->get('house_type');
  }

Rerolled with changes from #105/#106.

Please note that I addressed point 7 by adding a docblock to VariationCacheFactory but almost all implementations of CacheFactoryInterface do not have one. So even though core is at fault for not having them, adding one kind of breaks the habit.

+++ b/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php
@@ -313,24 +293,25 @@ public function providerTestContextBubblingEdgeCases() {
+   * @todo Revisit when we have self-healing tests for VariationCache. This is
+   * essentially a clone of the other bubbling tests now.

I should probably get rid of this soon as the variation cache test now checks for self-healing, but let's see what people think of the patch now instead of focusing on changing one test.

I thought of one edge case and one problematic scenario that needed resolving. The tests in the interdiff speak volumes, so read that :)

I see what's going on. My changes to ResourceTestBase are now irrelevant because the latest patch properly handles overlap scenarios whereas the original one only dealt with supersets and subsets, but not partial matches. This is actually a good thing if my assumptions are correct because it means the patch will become even smaller in size, yet more robust.

Glad I thought of, caught and tested the edge cases in today's patch. :)

This basically reverts the changes to ResourceTestBase/EntityResourceTestBase and thus makes the patch even smaller.

This basically reverts the changes to ResourceTestBase/EntityResourceTestBase and thus makes the patch even smaller.

The patch got bigger though, so I don't understand this comment?

Okay so both ResourceTestBase and EntityResourceTestBase should not be checking for cache redirects as it's:

• Not the responsibility of these tests
• No longer predictable how many redirects there will be
• No longer the case that a redirect is at the given address.
  
  The old render cache would instantly look for keys:[foo]=bar and then maybe get redirected to keys:[foo]=bar:[cat]=dog whereas the new system actually first checks for keys and then could instantly look for keys:[foo]=bar:[cat]=dog, but could also have redirect steps in between. So the amount of CIDs matching LIKE %[foo]=bar% are no longer predictable.

The solution is to simply stop checking for redirects and make sure there is at least one entry in the DB.