Optimize placeholder retrieval from cache

kristiaanvandeneynde → credited berdir → .

kristiaanvandeneynde → credited catch → .

Will fail tests, but that's the idea. If it's only performance tests that fail, we have no collateral damage.

One performance test to update, but otherwise looks like it should be green. Not entirely sure why there are more cache gets in this case.

Asserted the render bin operations, but not sure I fully understand this.

--- Expected
+++ Actual
@@ @@
     16 => 'entity_view:block:stark_site_...-admin'
     17 => 'entity_view:block:stark_syndi...-admin'
     18 => 'navigation:navigation:[langua...-admin'
-    19 => 'navigation_user_block:[languages:language_interface]=en:[theme]=stark:[user.permissions]=is-admin, shortcut_set_navigation_links:[languages:language_interface]=en:[theme]=stark:[user]=2'
+    19 => 'navigation_user_block:[languages:language_interface]=en:[theme]=stark:[user.permissions]=is-admin'
     20 => 'navigation_user_block:[langua...ser]=2'
-    21 => 'view:frontpage:display:page_1:[languages:language_interface]=en:[theme]=stark:[url.query_args]=:[url.site]=http://core.ddev.site:[user.node_grants:view]=all:[user.permissions]=is-admin'
-    22 => 'view:frontpage:display:page_1:[languages:language_interface]=en:[theme]=stark:[user.permissions]=is-admin'
+    21 => 'shortcut_set_navigation_links:[languages:language_interface]=en:[theme]=stark:[user]=2'
+    22 => 'view:frontpage:display:page_1:[languages:language_interface]=en:[theme]=stark:[url.query_args]=:[url.site]=http://core.ddev.site:[user.node_grants:view]=all:[user.permissions]=is-admin'
+    23 => 'view:frontpage:display:page_1...-admin'
 )

something about the shortcut placeholder in the navigation, I think it's being fetched separately now, but I'm not sure why the CachedStrategy isn't picking it up with the other one? maybe due to the user cache context somehow?

Ok, now that block placeholders are in, we're seeing major improvements here that more than make up for the regression introduced by that issue. Not 100% sure it really makes sense to split this from ✨ Optimize redirect chain retrieval in VariationCache Active but probably yes, just the performance test impact overlaps are a bit painful.

Now that this removes those initial cache get requests, the benefits of the other issue will be be limited in the current implementation. However, the combined improvements should be even bigger once the other issue supports getMultiple() properly.

I also tracked down the change around the login test changes. It's... interesting, especially around the block login one.

What's happening is that we now no longer do a cache lookup for placeholdered blocks. However, on POST requests, we actually don't put placeholders in but always immediately render them. Combined with the change a while ago about being able to render cache elements on POST requests, we are actually able to return a render-cached menu block in this scenario, but not anymore with this change.

We could keep the current behavior in this scenario, by adding an additional check for !isMethodCacheable() so that we still use the render cache one-by-one on POST requests, assuming that if we start rendering blocks, we're likely going to be processing a form in one of them.

I was wondering if skipping placeholders on POST request is still the right to do but the short answer is yes. We can not process a form in bigpipe after the response has been sent.

something about the shortcut placeholder in the navigation, I think it's being fetched separately now, but I'm not sure why the CachedStrategy isn't picking it up with the other one? maybe due to the user cache context somehow?

I think I tracked this one down although it's not a new issue.

Placeholder strategies only run on placeholders at the top level of a $response via HtmlResponsePlaceholderStrategySubscriber

If a placeholder contains further placeholders, which the navigation bar does, then those don't run through CachedStrategy and end up going via big pipe eventually which then via the renderer checks the cache individually.

If we recursively process placeholders returned from the render cache, we can bypass that and save the individual cache gets.

I did very basic implementation of this, there are probably two things to do:

1. Collect all of the placeholders from all of the returned cache items, check the cache in one go, instead of once per top-level placeholder.

2. Potentially do this recursively so that placeholders returned by placeholders returned by placeholders get the same treatment.

One more performance test updated. I didn't investigate too closely what's going on there, that one bumps the cache tag lookup from 4 to 8. sounds like that previously had those blocks cached as part of the dynamic page cache. Probably because this test doesn't empty the render cache after logging the user in, so a bunch of blocks are already render-cached, which is exactly what this is about. 📌 Try to preload cache tags in cache getMultiple Active should help with that and reduce that again by 3-4?

Pretty challenging how we have so many overlapping performance issues that all have non-trivial, overlapping impacts.

I split my CachedStrategy change out to 📌 Recursively replace placeholders in CachedStrategy Active , it makes no difference to the navigation performance test on that issue, but it does here - so either it fixes a small regression here or it's a further improvement we can make once this issue lands. I really don't know which one it is, but in the end it is a single cache get and we've identified it so I think it's OK in its own issue, also there is more to do there I think.

Pretty challenging how we have so many overlapping performance issues that all have non-trivial, overlapping impacts.

It's very hard to keep track of, but every issue is working together with every other issue to compound improvements on top of each other which I don't really remember happening for a long time so great problem to have!

Needs work for the newly failing navigation performance test failure.

I adjusted the navigation test. I also implemented my idea about !isMethodCacheable(). In regards to queries on standard profiles, it's a bit a wash, one query saved, 7 extra cache gets and therefore also 7 extra cache tag lookups. But menu blocks on real sites might have huge menus and so on that would be expensive to calculate and several of the cache tag lookups should go away again with 📌 Use a single per-theme cache tag for block config entities Active (will pick that one back up once the current set of optimization issues are in)

The isMethodCacheable() change looks good to me, big difference between a handful of cache tag queries and a menu tree one. Also we didn't have render cache retrieval on POST at all until a couple of releases ago so sites can survive a few cache tag lookups.

will rebase this after 📌 Try to preload cache tags in cache getMultiple Active

Just rediscovered 📌 Use multiple get for #pre_render / #cache where possible Active which might be worth another look once the current cluster of 3-6 issues have landed.

Still postponed, but I've already rebased this on top of 📌 Try to preload cache tags in cache getMultiple Active to confirm my assumption that it makes sense and the answer I think is yes.

It's really cool to see how all these performance issues don't just add their improvements, but they actively combine to result in even nicer improvements.

This reduces the cache tag lookup increase for the dynamic page cache hit that this issue has on top of HEAD (3 to 8) to just 3 to 5. The reason it's multiple is that we have 3 groups of cache tag lookups. One is the menu cache tags being separate from the active trail cache lookup for the cache contexts, then we have a filter format cache tag from RenderCache::getMultiple() because that one apparently doesn't have a cache redirect and then all the other placeholdered blocks together. The reason it's a regression on HEAD is that previously, they would have been part of dynamic page cache, even though we explicitly asked for them to be placeholdered. This might sound like a regression, but it's actually good, because we want them to be separate from dynamic page cache, I think the current behavior also mean that autoplaceholdering is pretty busted because on cache hits on the inner elements, they still bubble up their expensive cacheability metadata to dynamic page cache.

And in multiple other cases with dynamic page cache misses, this saves 3-6 additional cache tag lookup queries in many test scenarios due to them now being multi-loaded.

It's really cool to see how all these performance issues don't just add their improvements, but they actively combine to result in even nicer improvements.

When we originally landed the #pre_render render cache, cache tags, cache collectors , cache contexts, placeholdering, dynamic page cache, big pipe, mostly in Drupal 8 with a few in Drupal 7, it felt like the cache system was close to feature complete.

But variation cache + performance tests the past year or so have really shown that we've not previously been using those features to their maximum in core, and every enhancement or tweak we've made has built on the last. And then once a batch of issues goes in, it opens up another new possibility that was not obvious before.

Yes and although it's more cache tag lookups compared to HEAD, should still be down from 11.1 overall which is the real comparison.

Looks like this needs a manual rebase.

local git rebase automatically identified the cache tags preload commit I included as redundant, so this should be fine, lets see.

Looks great to be honest. Comment is also very clear.

I nearly committed this, then went down a bit of a rabbit hole.

Because we're not fetching from cache when create_placheholder is TRUE, it might be possible for a nested item that has both cache keys and #create_placheholder set to skip caching altogether. The idea I had was to then only skip things depending on $is_root_call, but that undoes the change here.

So... I'm going to go ahead here, but we should continue in 📌 Use multiple get for #pre_render / #cache where possible Active and 📌 Recursively replace placeholders in CachedStrategy Active to make sure that nested cacheable placeholders are optimized as well as the top level

OK I think this is what I was missing:

  /**
   * Renders a placeholder into markup.
   *
   * @param array $placeholder_element
   *   The placeholder element by reference.
   *
   * @return \Drupal\Component\Render\MarkupInterface|string
   *   The rendered HTML.
   */
  protected function doRenderPlaceholder(array &$placeholder_element): MarkupInterface|string {
    // Prevent the render array from being auto-placeholdered again.
    $placeholder_element['#create_placeholder'] = FALSE;

    // Render the placeholder into markup.
    $markup = $this->renderInIsolation($placeholder_element);
    return $markup;
  }

When we get to the code being changed, because it goes through doRenderPlaceholder(), even though we original set #create_placeholder to TRUE, when we actually want to check the placeholder item, #create_placeholder is false again, so it gets from cache anyway.

So the bug I thought we were introducing is not in fact introduced, everything is fine. The split between placeholder strategies and the renderer makes this hard to follow though.

After all that, committed/pushed to 11.x, thanks!

catch → closed merge request !11462