Add a CachedPlaceholderStrategy to optimize render cache hits and reduce layout shift from big pipe

To comment myself:

The reason we have a ChainedPlaceholderStrategy in core by default is not only to support big_pipe, but because I envisioned that a lot of things that are not bound by URL would become placeholders in the first place.

As can be seen from the linked issue in #7 I had this whole idea of making it super easy to put e.g. a shopping cart placeholder on the page and replace it via JS, AJAX, etc.

And therefore the natural way to go about this is to use the chain:

- CachedPlaceholderStrategy
- BigPipePlaceholderStrategy
- SingleFlushPlaceholderStrategy

The reason is that the cached placeholder strategy can do a cache->getMultiple() [for anything that can be cached] while the SingleFlushPlaceholderStrategy is just rendering them one by one.

So even for just core this can save performance.

Whoever works on CachedPlaceholderStrategy here is how the raw architecture should look like (untested code):

// \Drupal::cache('render')
protected $cache;

// \Drupal::service('placeholder_strategy')
protected $placeholderStrategy;

// \Drupal::service('render_cache')
protected $renderCache;

function processPlaceholders($placeholders) {
  $cids = [];
  $new_placeholders = []; 

  foreach ($placeholders as $placeholder => $elements) {
    $cid = $this->renderCache->getCacheID($elements);
    if ($cid) {
      $cids[$cid] = $placeholder;
    }
  }

  $cached_items = $this->cache->getMultiple(array_keys($cids));

  foreach ($cached_items as $cid => $cache) {
    $elements = $cache->data;

    // Recursively process placeholders
    if (!empty($elements['#attached']['placeholders']) {
      $elements['#attached']['placeholders'] = $this->placeholderStrategy->processPlaceholders($elements['#attached']['placeholders']);
    }
    
    $placeholder = $cids[$cid];

    $new_placeholders[$placeholder] = $elements;
  }

  return $new_placeholders;
}

The reason for the recursion is to support the following scenario (part of ORIGINAL 2014 big pipe demo):

- There is a sleep(1) on the page in a cacheable block content, which sets max-age=0
- Then this makes the whole block uncacheable.
- As soon as you create a placeholder within the block for the current_time, then the block becomes cacheable again. You still want to create a placeholder for the block as it contains other content that is independent of the page.

With the CachedPlaceholderStrategy with above implementation:

- If the block is uncached then the whole block is streamed via big_pipe.
- If the blocked is cached then just the placeholder is streamed via big_pipe.

Without the recursive calling of the strategy, this would fail and the `current_time` would again make the whole page slow.

While this is untested, here is the code that Grok + ChatGPT created together with some direction for VariationCache::getMultiple():

/**
 * Retrieves multiple cached items and follows any redirects until resolution.
 *
 * Given an array of items—each defined by keys and cacheability—this method:
 * 1. Generates initial cache IDs (CIDs) for each item.
 * 2. Fetches them from the cache in batches.
 * 3. If any fetched item indicates a redirect (via a CacheRedirect object),
 *    updates the CID and repeats the process.
 *
 * Only items that ultimately resolve to a cached value are returned. Items that
 * do not resolve (cache misses after following possible redirects) never appear
 * in the returned array.
 *
 * @param array $items
 *   An associative array keyed by arbitrary identifiers. Each value is:
 *   [ (array) $keys, (CacheableDependencyInterface) $cacheability ].
 *
 *   Example:
 *   $items = [
 *     'item_a' => [ ['key1', 'key2'], $cacheability_a ],
 *     'item_b' => [ ['another_key'], $cacheability_b ],
 *   ];
 *
 * @return array
 *   An associative array keyed by the same keys as $items, containing only
 *   the items found in the cache. Items that never resolve are omitted.
 */
public function getMultiple(array $items): array {
  // Initialize processing with a map of CIDs to their associated item info.
  $cids_to_process = [];
  foreach ($items as $item_key => [$keys, $cacheability]) {
    $cid = $this->createCacheIdFast($keys, $cacheability);
    $cids_to_process[$cid] = [
      'item_key' => $item_key,
      'keys' => $keys,
    ];
  }

  $results = [];

  // Process CIDs until none remain. Each loop fetches all current CIDs, then:
  // - Resolves final items.
  // - Identifies redirects and prepares them for the next iteration.
  while (!empty($cids_to_process)) {
    $current_cids = array_keys($cids_to_process);
    $fetched_items = $this->cacheBackend->getMultiple($current_cids);

    $next_cids_to_process = [];

    // Process each fetched item. Misses are simply absent from $fetched_items.
    foreach ($fetched_items as $cid => $fetched_item) {
      $info = $cids_to_process[$cid];

      // If the item signals a redirect, we'll follow it in the next iteration.
      if ($fetched_item->data instanceof CacheRedirect) {
        $redirect_cid = $this->createCacheIdFast($info['keys'], $fetched_item->data);
        $next_cids_to_process[$redirect_cid] = $info;
        continue;
      }

      // Otherwise, we've reached a final cached item. Record it.
      $results[$info['item_key']] = $fetched_item;
    }

    // Only process redirects in subsequent iterations.
    $cids_to_process = $next_cids_to_process;
  }

  return $results;
}

With that in place, we can simplify our strategy significantly:

Let's just add a getMultiple() to RenderCache as well (help from ChatGPT).

/**
 * Retrieves multiple cached render arrays at once, following redirects until resolution.
 *
 * Steps:
 * - For each render element, compute a cache ID (CID) based on its keys and cacheability.
 * - Group elements by their cache bin.
 * - For each bin, use getMultiple() to fetch all items in one go.
 * - Follow any redirects discovered, updating CIDs and refetching until resolved.
 * - If the current HTTP request method is not cacheable, skip items tagged
 *   with 'CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:'.
 *
 * Only items that resolve to a final cached value are returned. Misses or 
 * disallowed items (due to request method constraints) are omitted entirely.
 *
 * @param array $multiple_elements
 *   An associative array keyed by arbitrary identifiers. Each value should be
 *   a render array that includes '#cache' with 'keys' and optionally 'bin', e.g.:
 *   $multiple_elements = [
 *     'item_a' => [
 *       '#cache' => ['keys' => ['key1', 'key2'], 'bin' => 'render'],
 *       // ... additional render array data ...
 *     ],
 *     'item_b' => [
 *       '#cache' => ['keys' => ['another_key']], // defaults to 'render' bin
 *       // ... additional render array data ...
 *     ],
 *   ];
 *
 * @return array
 *   An associative array keyed by the same keys as $multiple_elements for items that
 *   are found and allowed. Items that never resolve or violate request conditions 
 *   are not returned.
 */
public function RenderCacheGetMultiple(array $multiple_elements): array {
  $bin_map = [];
  foreach ($multiple_elements as $item_key => $elements) {
    if (!$this->isElementCacheable($elements)) {
      continue;
    }

    $bin = $elements['#cache']['bin'] ?? 'render';
    $keys = $elements['#cache']['keys'];
    $cacheability = CacheableMetadata::createFromRenderArray($elements);

    $bin_map[$bin][$item_key] = [$keys, $cacheability];
  }

  $results = [];
  $is_method_cacheable = $this->requestStack->getCurrentRequest()->isMethodCacheable();

  foreach ($bin_map as $bin => $items) {
    $cache_bin = $this->cacheFactory->get($bin);
    if (!$cache_bin) {
      continue;
    }

    $fetched_items = $cache_bin->getMultiple($items);

    foreach ($fetched_items as $item_key => $cache) {
      if (
        !$is_method_cacheable &&
        !empty(array_filter($cache->tags, fn (string $tag) => str_starts_with($tag, 'CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:')))
      ) {
        continue;
      }
      $results[$item_key] = $cache->data;
    }
  }

  return $results;
}

Then our CachedStrategy looks like:

// \Drupal::service('placeholder_strategy')
protected $placeholderStrategy;

// \Drupal::service('render_cache')
protected $renderCache;

function processPlaceholders($placeholders) {
  $new_placeholders = []; 

  $cached_items = $this->renderCache->getMultiple($placeholders);

  foreach ($cached_items as $placeholder => $cache) {
    $elements = $cache->data;

    // Recursively process placeholders
    if (!empty($elements['#attached']['placeholders']) {
      $elements['#attached']['placeholders'] = $this->placeholderStrategy->processPlaceholders($elements['#attached']['placeholders']);
    }
    
    $new_placeholders[$placeholder] = $elements;
  }

  return $new_placeholders;
}

I could have some bandwith to start to work on this issue this week.
Went through the comments and it's not 100% clear to me from where I should start my investigation, or even if all the pieces to start are already in place.
Would be great if you could add comment to indicate the possible steps to follow that would help me to kick it off.

Thank you for your feedback.

Started to work on a draft MR based on the boilerplate code from #11 & #12 with some minor adjustments to make it work, but found some issues related to recursion of CachedStrategy & Big Pipe. After the 1st load, once the cache is warm, the page content is not being loaded and the following error is thrown instead:
The #lazy_builder property must have an array as a value, containing two values: the callback, and the arguments for the callback. in <assert() (line 327 of core/lib/Drupal/Core/Render/Renderer.php)

It seems due to the fact that the same placeholder is being rendered in BigPipeStrategy::createBigPipeNoJsPlaceholder() twice from different places, ended up in having the same #lazy_builder callback duplicated. Hence, when the #attached arrays are merged as part of the page build process, the callback array has 4 elements instead of 2. This is happening for the Logout link placeholder, which is rendered both in the User Account menu and in Navigation.

Steps to reproduce

1. Checkout the MR branch
2. Install Standard profile and Navigation module
3. Login as admin
4. Go to the homepage while the cache is cold, and the page is loaded as expected
5. Reload the page, once the cache is warm
6. Confirm that page content is not being displayed and the error above is included in the HTML markup

I'm not sure how to proceed here, we could indicate at build time that #lazy_builder callbacks should not be merged, but that could be complex. Another option could be to keep track of the generated placeholders somewhere and avoid duplicates.

Any idea?

No additional input just appears to need a rebase

If you are another contributor eager to jump in, please allow the previous posters at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

Back to Needs Review once conflicts have been solved and last comments in the MR addressed.

If the changes here are good and the approach is validated, it might be time to figure out how to implement specific tests.

Reviewing the whole thread, bear with me:

Maybe we can add some kind of additional hint for the placeholder strategy so that the navigation placeholder always gets rendered via a nojs placeholder (i.e. inline with the main response) - this would address both caching efficiency and layout shift then.

+1

Re #9 and #21 Would indeed be great to get some extra information here from @fabianx.

Re #11: Holy shit, AI wrote that? Because that looks like it might work.
Re #12: I need to start believing in AI more, both this and the above proof of concept look amazing.

Will check the MR next.

Okay so I'm willing to RTBC this from a code point-of-view, provided a few things are cleaned up.

First and foremost: The new methods' documentation on the interfaces is far too verbose and opinionated. If you want, I can rewrite them for you but I'll give you first dibs.

Secondly, I don't think it's wise to gut VariationCache::get() to make it use ::getMultiple(). It's slower, it's less readable and we still have ::delete() and ::invalidate() using the original ::get() body, so we'd be confusing people trying to read and understand VariationCache as to why ::get() is doing weird stuff.

Finally, the same could be said about RenderCache::get() but I'm a bit less fussed here. I still think we should preserve as many performance optimizations as the original code had so I'd like to see a bin existence check earlier in the process of getMultiple().

Overall really exciting MR, though. Thanks for working on this!

Also at this point should we tackle navigation in here or in (yet another) follow-up? It seems the discussion and MR here went in a totally different (yet logical) direction :)

I already made the Navigation changes in the old MR that was replaced by the current one, so I could easily cherry-pick them. On the other hand, keeping the focus here would make this issue simpler.

I'm totally OK if you feel that's better to have methods separated, just wanted to reduce a bit the amount of code, but the downsides could be higher than the benefits.

Regarding docblocks, I can take a look there, current ones were just a copy&paste from the suggested in #12.

Thanks, feel free to ping me on Slack for feedback or assistance with the last few bits. Again, this is looking amazing so I definitely want to give you my full support on getting this in.

Worked on the bits mentioned in the MR. I think this is ready for a new review.

Made a rebase once #3504386: Use a placeholder for the navigation toolbar was merged and addressed the last comment in the MR.

Some documentation still mentions redirects where we're supposed to be agnostic of its existence. Will update myself, but as far as I am concerned this is RTBC. Just give me a minute to make some final adjustments.

Done. If I had only updated the docs I would RTBC, but I made a few changes to RenderCache to make it more readable and bail out early if we could not load any bin, just like ::get() does. For that reason, I'll yield to @catch to RTBC.

But everything I did not touch and the general concept is RTBC to me now.

Reviewed.

Addressed all feedback. Some nice finds, @berdir.

Code looks a lot cleaner after this last round of feedback too :)

Thanks, looks good to me now.

One thing I noticed is that CacheFactory does not have a static cache, every time you ask it for a bin, it creates a new instance. That's likely the reason why the Renderer in the past used the service and not the cache factory directly.

This probably doesn't have a big impact on the database. It does for redis because it stores some information like the last deleted flag, but the redis cache backend factory does have a static cache per bin for this reason.

Rebased this, the only conflicts where in PerformanceTest and StandardPerformanceTest, and the resulting numbers are identical, it's just that there were already some decreases in HEAD.

So I think it's OK if I set that back to RTBC.

it needs summary update about API change at least, or change record

The change record that is linked to this issue is not correct it seems?

It is. It's the new API that we introduced here.

The CachedStrategy itself isn't that CR-worthy (IMO) as it is merely an optimization.