Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
See #2499157: [meta] Auto-placeholdering. Blocked on #2543332: Auto-placeholdering for #lazy_builder without bubbling.
Proposed resolution
Elements that have:
#lazy_builder
- no poor cacheability (the other case was already handled in #2543332: Auto-placeholdering for #lazy_builder without bubbling)
… where the lazy builder returns content that results in the bubbling of poor cacheability (low max-age, high-cardinality cache contexts or high-invalidation frequency cache tags (concrete examples: see below)
Remaining tasks
- As agreed in #2543332: Auto-placeholdering for #lazy_builder without bubbling: update the comment form to bubble the necessary cache contexts, so we can simplify
CommentDefaultFormatter
. - Review.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#75 | auto-placeholdering-step2-2543334-75.patch | 50.65 KB | Wim Leers |
Comments
Comment #1
Wim LeersComment #2
Wim Leers#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.
Comment #3
Wim LeersHere'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.
Comment #4
Wim LeersStep 1/4: support for auto-placeholdering in case of bubbled cache contexts
Notable changes
RenderCache
class, so that that class can continue to do the basics — so:class PlaceholderingRenderCache extends RenderCache
#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.
Comment #5
Wim LeersStep 2/4: cache tags for cache redirects
Conceptual explanation
From the patch:
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.Comment #6
Wim LeersStep 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.
Comment #7
Wim LeersStep 4/4: support for auto-placeholdering in case of bubbled max-age
Notable changes
RenderCache
is split up into slightly more methods, so thatPlaceholderingRenderCache
is able to override in the right placesPlaceholderingRenderCache
grew significantly more complex to be able to handle the efficient auto-placeholdering of bubbledmax-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.
Comment #8
Wim LeersNext 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
.Comment #9
Wim LeersFinally, 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++
Comment #13
Wim LeersBefore 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.
Comment #14
Wim LeersAnd here's an additional unit test that verifies render cache redirect cache tags are indeed generated in the expected manner.
Comment #15
Wim LeersAnd 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.
Comment #18
Wim LeersNew failures, so this won't be green.
Comment #20
Wim LeersI 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.
Comment #21
Wim LeersI'm thinking that adding a private
render_placeholder
service (just like we have a privaterender_cache
service) that is used by both therenderer
andrender_cache
services is the simplest/sanest way to fix the duplication that we currently have inrenderer
+render_cache
services.Comment #22
Fabianx CreditAttribution: Fabianx for Acquia commented+1 to
#32#21, sounds great!Comment #23
Fabianx CreditAttribution: Fabianx for Acquia commentedCNW for #21, so no longer blocked on reviews.
Comment #24
Wim Leers#22 referred to #32, that should have been #21 I assume? :)
Also, you surely must have *some* remarks, questions or at least nitpicks? :)
Comment #25
Fabianx CreditAttribution: Fabianx for Acquia commented#24: Yes, will review more later.
Comment #26
Fabianx CreditAttribution: Fabianx for Acquia commentedIncomplete review. this needs some deep thinking.
I think I would prefer NULL and isset() instead of FALSE.
Should this not only cache the max-age-0 if if _should_ placeholder the max-age=0 case?
I know its an edge case.
Interesting solution, I am still analyzing all cases I had.
I think the order should be consistent in both same checks?
Nice! :)
Now you finally know what I meant there.
I like the solution in throwing away the top frame and populating it again from whatever is in $elements.
Comment #27
Wim LeersFirst doing my proposal in #21, since Fabianx was +1 to that in #22.
Comment #28
Wim LeersAnd when we did #27, we might as well move
canCreatePlaceholder()
in there too, out ofPlaceholderingRenderCache
. ThenRenderer
can reuse that as well.Comment #29
Wim LeersOops, I meant this to be part of #28 too. Well, in any case: this makes the service private and adds some documentation to
PlaceholderGeneratorInterface
.Comment #30
Wim Leers#26
Comment #32
Wim LeersApparently
\Drupal\Tests\views\Unit\Controller\ViewAjaxControllerTest
instantiates aRenderer
. Had to update that too.Comment #37
Wim LeersFailed on old testbot, passed on DrupalCI. Weird. Re-testing.
Comment #40
Wim LeersFirst, a straight reroll. Let's see if this was a PIFR problem after all. Because DrupalCI was reporting #32 as all-green.
Comment #42
Wim LeersCould it be this?
Comment #43
Crell CreditAttribution: Crell as a volunteer commentedThis is inefficient as it calls createPlaceholder() twice. Instead, check shouldPlaceholder() first, and then $placeholder = createPlaceholder() within the if statement, then you can simply return $placeholder.
I read this paragraph twice and I'm still not following. This method seems to be responsible for returning, um, a placeholder or filtering out what? Sorry, I'm just confused.
Also, a method returning a meaningful value or FALSE is always wrong, always, period. If you mean "there's no value here", that's NULL, not FALSE.
So just refactor RenderCache::doSet() to make it easier to swap out the pieces you need. :-) Just from this comment it sounds like doSet() needs to be broken out into multiple methods anyway.
Comment #44
Wim Leers#43: Thanks for the review! Much appreciated :)
canCreatePlaceholder()
, the code within the if-test callscreatePlaceholder()
. (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.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 amax-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 cachedmax-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 ofcanCreatePlaceholder()
), it is indeed very confusing.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 todoSet()
, but it would never make sense forRenderCache
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!
Comment #45
Wim LeersClarifying title for #44.
Comment #46
Fabianx CreditAttribution: Fabianx for Acquia commentedThe limitation of this solution is that you will always need to have a cache hit for the placeholder to work.
So you can never make use of the information that is available in the redirect.
As the parent::get() won't give you that information, which is why I had way more tight coupling in my proof-of-concept patch.
e.g. updating the redirecting cache item on ::set().
e.g.
For a block cacheable by user, where the user is bubbled up:
My solution:
After any user loads a page with that block:
- Cache redirect contains 'user'
- Detected as frequent cache context
- #create_placeholder is added to 'redirecting cache item'
The next time for a different user:
- Cache miss on the cache, but a cache hit on the 'redirecting cache item' and seeing '#create_placeholder' => creating a placeholder directly (if it canCreatePlaceholder), as such BigPipe could deliver the block later.
Your solution / solution here:
- Cache miss
- Item is re-created by executing the block
- Checking for should-placeholder
- Placeholder then
- Item is re-created by re-executing the block
---
Also we have double cache_get() here, right?
Which is why I stored that information somewhere, in this case the "cache" could live in the placeholderGenerator.
Overall looks great!
I think it is a good intermediate step and I think adding more smarter auto-placeholdering should be fine as an internal refactoring.
The double cache_get is a little unfortunate, but without render strategies, where that could live in a 'cached placeholder' render strategy, this is difficult to generically resolve here.
The critical problem is that a block would be executed twice, as the executed lazy builder callback result is stored nowhere - however it would theoretically be cached - unless it is max-age = 0, which could happen.
I suggest as a quick fix to:
- add a second argument to createPlaceholder
e.g. RenderCache::get
e.g. RenderCache::set
Then in Renderer ask the placeholderGenerator if the placeholder is already statically cached and use that:
e.g. in Renderer::renderPlaceholder
That would solve both cases of double cache_get and throwing away existing information.
Comment #47
Fabianx CreditAttribution: Fabianx for Acquia commentedThe scope for this patch that needs work and which I would like to see is:
- Ensure that any data we pre-computed is statically cached (for this request) and used in renderPlaceholder() of the renderer (until render strategies are in).
Motivation:
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.
The implementation details I propose are in #46 starting from "I suggest as a quick fix to:" ...
But here they are again in pseudo diff form:
PlaceholderingRenderCache::get
PlaceholderingRenderCache::set
In PlaceholderGenrator::createPlaceholder:
Then in Renderer ask the placeholderGenerator if the placeholder is already statically cached and use that:
In Renderer::renderPlaceholder
Comment #48
Wim LeersDatabaseBackend
and others already have static caches.To clarify:
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.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 existTherefore, I don't think any extra work is necessary here.
Comment #49
Fabianx CreditAttribution: Fabianx for Acquia commented#48: Would be nice for certain cases, but unfortunately it is not true:
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Cache%21D...
There is no static cache.
catch in particular does not like double Cache::get, so I don't think this will fly.
Comment #50
Wim LeersDarn, 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?
Comment #51
Fabianx CreditAttribution: Fabianx for Acquia commentedWe will need a way to store computed data in general, we could postpone this one on placeholder strategies, which makes it way more straightforward to add, but as I said even if we retrieve it again from cache, I don't think a cache:set, cache:get to itself is a good practice.
Comment #52
Fabianx CreditAttribution: Fabianx for Acquia commented#48.2: For the first request it would exist as well as that information would need to come from somewhere and also this would at the moment mean that if auto-placeholdering for max-age = 0 is on, then it would render all of that twice instead.
Per pure logic:
So if any of that is TRUE it will create a placeholder and throw away the computed data.
- canCreatePlaceholder checks for #lazy_builder present and that the #create_placeholder is not FALSE
- shouldCreatePlaceholder checks max-age condition
=> Can be met
=> Placeholder created for max-age = 0
=> Expensive block re-computed as result is stored nowhere
=> Needs work
Comment #53
Fabianx CreditAttribution: Fabianx for Acquia commentedcan be used to show the problem of #52.
With HEAD, build is just called once; with #44 patch above build is called twice.
The attached patch solves the problem using placeholder strategies and is rolled on top of them and build is again just called once.
Comment #54
Wim LeersI was very hopeful #53 would be an elegant solution. But… we still have a static cache in
PlaceholderGenerator
. I thought the point was that theStaticCacheStrategy
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.Comment #55
dawehnerI think if possible we should key that by the request, just to be sure.
Comment #56
Fabianx CreditAttribution: Fabianx for Acquia commentedOverall the counter proposal looks great!
It really is a matter of which pill to take and which service to add the state, too.
Note: That adding it here does not prevent us from moving that to render strategies transparently, so I am inclined to RTBC for now, but there is a little problem, which is that this is missing to use getCacheableRenderArray(), even for a static cache its important to use the cacheable version, not whatever is passed in.
The advantage of moving that to placeholder strategies is that there is no additional run-time overhead for normal RenderCache::get() requests of which there could be more than just a few.
I made the mistake first, too.
Either here or in createPlaceholderAndRemember it should be used via $this->getCacheableRenderArray(). Else you get a ->getPlugin() not found error.
Comment #57
Wim Leersif (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.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.Comment #58
Fabianx CreditAttribution: Fabianx for Acquia commentedRTBC, thanks all feedback has been addressed.
Comment #59
Wim Leers#55's still needs to be addressed
Comment #60
Wim LeersSo, to answer #55:
… 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.
Comment #62
Wim LeersChasing HEAD. Straight reroll.
Comment #63
alexpottSome nits that can be fixed on commit.
Comment #65
lauriiiJust quick reroll. It seems like something adds user.permissions cache context which is being optimized away and for that reason some extra cache tags are bubbling up. Don't credit me for this because this is a small change in a big change :)
Comment #66
Wim Leers#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.)Comment #70
lauriiiRerolled the patch.
This chunk has changed where its copied from
Also had to add casting here
Comment #71
Wim LeersThanks, @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.
Comment #74
Wim Leers#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.
Comment #75
Wim LeersCause 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.Comment #76
Fabianx CreditAttribution: Fabianx for Acquia commentedChanges look great to me.
Back to RTBC.
Comment #78
catchThis looks great. The docs explain what's going on, and this should be very transparent to most people which is of course the point.
I didn't find anything other than a couple of very minor nits.
Committed/pushed to 8.0.x, thanks!
Comment #79
Wim LeersGlad 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.