Auto-placeholdering for #lazy_builder with bubbling of contexts and tags

#2543332: Auto-placeholdering for #lazy_builder without bubbling just landed, this is now unblocked! Have the patch ready, just need to rebase and clean it up.

Here's an updated IS already. Patch is rebased. Patch is also updated to support high-invalidation frequency cache tags, which was added in #2543332: Auto-placeholdering for #lazy_builder without bubbling, but was missing in the original patch.

Now cleaning up the patch in steps that make sense so that it's much more reviewable.

Step 1/4: support for auto-placeholdering in case of bubbled cache contexts

Notable changes

• Implemented at the render cache service level
• Specifically, implemented by decorating the existing RenderCache class, so that that class can continue to do the basics — so: class PlaceholderingRenderCache extends RenderCache
• The renderer now also tracks #lazy_builder and #create_placeholder in its $pre_bubbling_elements, instead of only #cache. This is necessary to allow the render cache to correctly determine when it can auto-placeholder something.

Conceptual explanation

Implementing this at the render cache level makes a lot of sense conceptually: you first retrieve something from the render cache, then notice that its cacheability meets auto-placeholdering conditions. (The whole point of render caching is that the subtree no longer needs to be rendered: that already happened. Which means you have all bubbled cacheability metadata, et voila: bubbling support!) When you notice that, this render cache implementation decides not to return the actually stored render cache item, but a placeholder, because it doesn't want to poison the rest of the page with poor cacheability. And then when the placeholder is replaced, it will be a simple render cache hit.

Step 2/4: cache tags for cache redirects

Conceptual explanation

From the patch:

+      // Create a redirect cache tag, which is used for two purposes:
+      // 1. invalidate cached supertrees (otherwise redirected render cache
+      //    items may continue to not be automatically placeholdered, because a
+      //    high-cardinality cache context was not yet known originally)
+      // 2. invalidate cache items that become obsolete when the cache contexts
+      //    stored in the cache redirect are discovered to be incomplete.

So, this helps with keeping the number of cache items lower (2), but much more importantly, it is necessary for correctness: in case a redirect changes (i.e. additional cache contexts are bubbled), then something that is not yet placeholdered because only the ['user.roles'] cache context was bubbled, may suddenly need to become placeholdered, because now the ['session'] cache context is also bubbled. Without a cache tag to invalidate e.g. a SmartCache-cached page that contains the redirected, but non-placeholdered output that varies by user, that would not start showing a placeholder as it should.

Step 3/4: support for auto-placeholdering in case of bubbled cache tags

This really does not need additional logic; step 1 already provides all the necessary infrastructure. This just adds test coverage for this case.

Step 4/4: support for auto-placeholdering in case of bubbled max-age

Notable changes

• RenderCache is split up into slightly more methods, so that PlaceholderingRenderCache is able to override in the right places
• PlaceholderingRenderCache grew significantly more complex to be able to handle the efficient auto-placeholdering of bubbled max-age=0.

Conceptual explanation

To be able to efficiently know that a subtree bubbles max-age=0, we must be able to … cache the fact that max-age=0 was bubbled! This seems utterly and completely wrong at first sight :)
But! If the subtree bubbled max-age=0, then that must be deterministic. Therefore, given the same cache contexts (i.e. same input from the request context) and given the same tags (i.e. same input data from Drupal), it must remain max-age=0.

This is highly counter-intuitive, but I haven't been able to find any example where this breaks down. Props to the amazing @Fabianx for dreaming this up! :D

See the documentation in the patch for more explanations.

Next step: as we said in #2543332: Auto-placeholdering for #lazy_builder without bubbling, we should update the comment form to bubble the necessary cache contexts, so we can simplify CommentDefaultFormatter.

Finally, I really want to stress that the above would not have been possible without @Fabianx. This is basically a completely rewritten implementation of his PoC code in #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts — this code should be of sufficient quality to be acceptable for D8 core, with sufficient test coverage, and so on.

I used his PoC as a general guideline, but often did things differently to achieve a simpler code flow.

In any case, without his PoC, i.e. without him having validated that this was at all possible, we wouldn't be here.

Fabianx++
Fabianx++
Fabianx++

Before fixing the test failures: here's a test showing (proving) why step 2 (#5), i.e. a cache tag for every cache redirect, is necessary.

Note that this could technically be moved into a separate issue, but it is only once we have placeholdering for bubbled metadata (which is exactly what this issue adds), that this pre-existing bug becomes more apparent.

And here's an additional unit test that verifies render cache redirect cache tags are indeed generated in the expected manner.

And here are then finally the test fixes for the test failures in all preceding patches; all due to missing cache redirect cache tags in some of the integration tests.

This makes the patch 50% bigger. Which is why I'm now inclined to move the cache redirect cache tag portion of this patch into a separate patch/issue. It's a pre-existing bug anyway. And that will make this patch significantly simpler to review.

New failures, so this won't be green.

I moved all the cache redirect stuff to #2548891: Cache context bubbling: cache redirects need cache tags. This makes comments #5, #13, #15 and #16 obsolete here.

Hence this patch is now back to just #4 + #6 + #7, i.e. steps 1, 3 and 4. Step 2 is moved to #2548891.

I'm thinking that adding a private render_placeholder service (just like we have a private render_cache service) that is used by both the renderer and render_cache services is the simplest/sanest way to fix the duplication that we currently have in renderer + render_cache services.

#22 referred to #32, that should have been #21 I assume? :)

Also, you surely must have *some* remarks, questions or at least nitpicks? :)

First doing my proposal in #21, since Fabianx was +1 to that in #22.

And when we did #27, we might as well move canCreatePlaceholder() in there too, out of PlaceholderingRenderCache. Then Renderer can reuse that as well.

Oops, I meant this to be part of #28 too. Well, in any case: this makes the service private and adds some documentation to PlaceholderGeneratorInterface.

#26

1. Done.
2. You're right, but the only reason this would not be cached already is if max-age = 0. So it's implied by the other checks, but you're right, we should do this for consistency.
3. Of course, good catch, fixed.
4. :) Yes, much simpler than what you had.

Apparently \Drupal\Tests\views\Unit\Controller\ViewAjaxControllerTest instantiates a Renderer. Had to update that too.

Failed on old testbot, passed on DrupalCI. Weird. Re-testing.

First, a straight reroll. Let's see if this was a PIFR problem after all. Because DrupalCI was reporting #32 as all-green.

Could it be this?

#43: Thanks for the review! Much appreciated :)

1. Is it possible that you misread the patch slightly? The if-test calls canCreatePlaceholder(), the code within the if-test calls createPlaceholder(). (So the code you cited first check if it can be placeholdered at all, then it checks if it should be placeholdered, and if the answer to those questions is "yes" in both cases, then it does create a placeholder.
2. First, note that this was added in #7. This is only necessary for a bubbled max-age that causes placeholdering. Max-age-triggered placeholdering definitely is by far the most complex part here. Perhaps it's better to split that off into a separate issue?

   Second: an attempted explanation. In the ::set() method, we are caching the fact that a max-age = 0 was bubbled. That sounds strange, but we need to cache that to not have to render the entire subtree to know in the first place that a max-age=0 is being bubbled, otherwise we're still rendering everything only to throw it away. So, we need to cache the fact that this subtree can not be cached. This aspect is cacheable, because it must be happening for a specific combination of cache contexts, that is, because the fact that this subtree is not cacheable must be deterministic. So, put simply: we're caching the fact that this particular branch of code results in a render array that cannot be cached.
   Now, given that we've cached the uncacheability of this branch of code, we actually get to the point where I can explain the reasoning behind the code you quoted. That code is dealing with a render cache hit, and specifically a render cache item that caches the uncacheability of a branch of code (of a render array subtree). And not only that, but it specifically checks whether placeholders are currently being replaced (see \Drupal\Core\Render\Renderer::renderPlaceholder(), which contains a comment // Prevent the render array from being auto-placeholdered again. for the code $placeholder_elements['#create_placeholder'] = FALSE; — this ensures that we don't do recursive placeholdering) by checking if #create_placeholder === FALSE. In that case, it needs to return FALSE, to ensure that we actually build the entire subtree (by executing the #lazy_builder) and render it. In the other case (where #create_placeholder is not set or TRUE), the preceding code block is executed, and the cached max-age=0 causes this subtree to be placeholdered.
   (Who said Drupal is only joining bits and pieces together and you don't need algorithmic knowledge/understanding? This is a fairly advanced algorithm exploiting tree properties and deterministicness.)

   If this does either still leave you with questions or makes you uncertain about this part of the patch (which I would completely understand, because this is very tricky), then I'd ask that we move max-age-triggered bubbling to a follow-up, because as I mentioned earlier, this is by far the most complex part of the patch.

   (While writing this comment, I tried to simplify the logic, but I couldn't. Because it needs to check specifically for a subset of reasons to placeholder (only max-age, the first condition of shouldAutomaticallyPlaceholder()) and because it needs to check for a subset of the reasons that show whether something can be placeholdered (only the second condition of canCreatePlaceholder()), it is indeed very confusing.

3. I don't think it makes sense to split doSet(). It's long, but only because of that very (very!) long comment. And this too is only complex/nasty because o bubbled-max-age-triggered auto-placeholdering is so complex.
   If there's a way to split up doSet(), I'm not seeing it. Because we want all that logic, we just want to override the max-age. We could add an optional $overridden_max_age parameter to doSet(), but it would never make sense for RenderCache itself. So it'd make that method more difficult to understand.

   So… let's move bubbled-max-age-triggered auto-placeholdering to a separate issue?

In this reroll, I removed the bubbled-max-age-triggered auto-placeholdering, so we can do that in a separate issue, opened #2559847: Auto-placeholdering for #lazy_builder with bubbling of max-age for that. That makes this issue just about "placeholdering for things that are cacheable but that we don't want to cache because it'll be too expensive to cache". Either because of too many variations (certain cache contexts) or too frequent invalidations (certain cache tags).

The max-age portion that lived in this patch until now is much more complex because it is very different: it's for placeholdering things that are not cacheable!

Clarifying title for #44.

1. A double cache get is harmless, because DatabaseBackend and others already have static caches.

2. I don't think we can afford to build a block that takes 5 or 10 seconds and is uncacheable, but should be placeholdered after bubbling per the rules twice. As that would double the time used for that block.

   To clarify:

   1. if the expensive value for the placeholder is cacheable at all (e.g. per user), then it would simply be a cache hit once it is computed a first time. => the claimed problem does not exist in this case.
   2. if the expensive value is not cacheable (i.e. max-age=0), then #2559847: Auto-placeholdering for #lazy_builder with bubbling of max-age ensures a cache item is created with just the metadata to let the placeholdering render cache know that a placeholder must be created. => with just this patch, the claimed problem does exist; but once #2559847 is in, it will no longer exist

Therefore, I don't think any extra work is necessary here.

Darn, misleading variable names! You're right.

So, #48.1 is invalid and needs a solution.

But point #48.2 still stands. And that's the trickier point you raised. Could you reply to that also?

I was very hopeful #53 would be an elegant solution. But… we still have a static cache in PlaceholderGenerator. I thought the point was that the StaticCacheStrategy placeholder strategy would contain all that? That the rest would not have to think/be aware about this?
Hence this solution much more complex than I had anticipated
I expected that nothing in the existing auto-placeholdering patch would have to change. But that all the static caching/double-get avoiding stuff would live in a static cache placeholder strategy. Instead, it now adds static caching to both the existing patch and the static cache placeholder strategy.

I now see why that was foolish of me, why it is absolutely necessary that this happens inside the render cache logic: that's the place where auto-placeholdering happens, so it's also where we have to statically cache them (again, to avoid double cache read or double render).

Hence I propose this patch, which is relative to #44, which adds a static cache to PlaceholderingRenderCache, which makes this much easier to understand IMHO.

1. This is why I'm wrapping it in if (isset($elements['#create_placeholder']) && $elements['#create_placeholder'] === FALSE) {, which only evaluates to TRUE when replacing placeholders or when an element explicitly marks itself as unplaceholderable. So the overhead is extremely, extremely limited.
2. Sure, added the getCacheableRenderArray() call in the cache set case. I actually first had that, but I thought it'd make the code slightly more complex. But I totally agree it's better.

#55's I think if possible we should key that by the request, just to be sure. still needs to be addressed

So, to answer #55:

<Fabianx-screen> Subrequests should not have their own cache.
<Fabianx-screen> As we only deal with it in the master request.

… and that just happens to make it effectively the same question that catch asked over at #2349011-53: Support placeholder render strategies so contrib can support BigPipe, ESI….1: he was wondering about placeholder strategies not applying to subrequests (and thus placeholder replacement not happening in subrequests).

But, already in HEAD, placeholder replacement only happens in the master request, because HtmlResponseAttachmentsProcessor only runs on the master request. Hence this cannot be a problem.

Back to RTBC per #58.

Chasing HEAD. Straight reroll.

#2511516: Make local tasks and actions hooks provide cacheability metadata, to make the blocks showing them cacheable causes additional cache tags, hence these new failures. lauriii already fixed them, I was cross-posting with him :P. Thanks, lauriii! :)

Also fixed #63. And fixed two typos in #65.

(This reroll is 100% documentation nits and unused use statement removals.)

Thanks, @lauriii!

I also rebased my own local branch, and arrived at the same end result. Here is a rebase interdiff (for #70.1) and an interdiff for the necessary casting changes (#70.2).

And a patch is relative to latest HEAD as of today while I'm posting interdiffs anyway. This patch is equivalent with #70.

Given that this is a straight rebase, with code moved by this patch being updated, and two simple string casts necessary due to changes in HEAD in that moved code, and those string casts being used only to allow them to be used as array keys, I think it's fine for me to re-RTBC this.

#2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping, which landed after #70 was posted, added a test that is now failing. Reproduced locally. Working on a fix.

Cause of those two failures: the local tasks on the tracker page bubbles the user cache context. In HEAD, this causes Dynamic Page Cache to not cache it. But with this patch, the local tasks block is instead auto-placeholdered, allowing the rest of the page to be cached. And because those two failures are both due to expected changes in output after setting something using \Drupal::state(), naturally, the pages are still cached by Dynamic Page Cache.

The fix is simple: apply the same rule as we've applied dozens upon dozens of times now: if a test does things by using State, then we have to manually force it to bypass caches.

Glad you liked it so much, @catch! And indeed the point is that it's a transparent change :)

Next step: #2559847: Auto-placeholdering for #lazy_builder with bubbling of max-age, which can easily happen after RC1, precisely because it's also a transparent change.