#gander

larowlan → closed merge request !10531

You can see 17 database queries and 31 cache gets and one cache tag checksum fetch less https://git.drupalcode.org/project/drupal/-/merge_requests/10531/diffs?c...

🔥 love it

Committed to 11.x - awesome work

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;
}

There is 📌 Update performance tests when Klaro tags a new release Postponed open which updates the performance test coverage - it shows that out of the box Drupal CMS will have 0 js or css from Klaro loaded now which is great.

What we don't have tests for is the impact once an app is enabled, that would be good to add especially once 🐛 Klaro library seems way too large? Active is in a tagged release.

I think this issue can probably be marked fixed/duplicate once the above issues are all in a tagged release. Really nice to see this come together.

In issue #3493540, we ensured that the JavaScript files are only loaded if services are also activated. This minimizes the standard footprint for the moment.

In issue #3491681 we changed the library from klaro-no-css.js to klaro-no-translations-no-css.js and thus reduced it from 208K to 164K.

During the work on #3491681 we also realized that the Klaro library is built with compatibility options for very old browsers. We have reduced these to the Drupal browser defaults and thus reduced the library to 64K. We are in contact with the maintainer and will adopt the new lean library.

In #3493822 we discuss whether the library can be integrated with `defer` or even `async`. However, due to the way the library is used, I am rather pessimistic about finding a solution here.

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

/**
 * Retrieves multiple cached items, handling redirects for each key set.
 *
 * @param array $items
 *   An array where each element is an array containing:
 *   - keys (array): An array of cache keys to retrieve entries for.
 *   - cacheability (CacheableDependencyInterface): Cache metadata for these keys.
 *
 * @return array
 *   An array where keys are the cache IDs, and values are the final cache items 
 *   or NULL if not found.
 */
public function getMultiple(array $items): array {
  $results = [];
  $cids_to_process = [];

  // Generate all cache ids for $items
  foreach ($items as $item) {
    [$keys, $cacheability] = $item;
    $cid = $this->createCacheIdFast($keys, $cacheability);
    $cids_to_process[$cid] = $keys;
  }

  // While there are still cache ids to process:
  while (!empty($cids_to_process)) {
    $current_cids = array_keys($cids_to_process);
    $fetched_items = $this->cacheBackend->getMultiple($current_cids);

    $new_cids_to_process = [];

    // Generate new list of cache ids to process further
    foreach ($cids_to_process as $cid => $keys) {
      $result = $fetched_items[$cid] ?? NULL;

      if ($result && $result->data instanceof CacheRedirect) {
        // If there's a redirect, generate a new CID to follow the redirect
        $new_cid = $this->createCacheIdFast($keys, $result->data);
        $new_cids_to_process[$new_cid] = $keys;
      } else {
        // If not redirected or no result, store the result directly
        $results[$cid] = $result;
      }
    }

    // Update $cids_to_process with new CIDs for the next iteration
    $cids_to_process = $new_cids_to_process;
  }

  return $results;
}

So to make this work:

Add a CachedPlaceholderStrategy - this does a multiple get on render cache items, replaces the placeholder directly with the cached HTML.

This runs all the time, before big pipe's own strategy (if big pipe is enabled). Big pipe then will only deal with placeholders which aren't already render cache hits.

When you have several placeholders render cached for stuff within the viewport, this will significantly reduce content layout shift and maybe largest contentful paint too, because all the HTML gets sent together until it can't be.

It will also work very nicely in tandem with 🌱 Adopt the Revolt event loop for async task orchestration Active because then anything expensive in the uncached placeholders that can be done async will be.

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.

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.

Talking to Fabianx:

CachedPlaceholderStrategy
c) BigPipe in particular would be combined with a CachedPlaceholderStrategy. Compared to single flush, that one could make use of $renderCache->getMultiple() (as soon as that exists), because again something that is cached already does not need to be BigPipe’d.

placeholder_strategy.cache:
    class: Drupal\Core\Render\Placeholder\CachedPlaceholderStrategy
    tags:
      - { name: placeholder_strategy, priority: -900 }

With the code to first generate the CIDs, then do a $cache->getMultiple() and return the HTML for those placeholders that had a cache hit.

Another thought.

For dynamic_page_cache efficiency, we would want this in a placeholder.

But to avoid cumulative layout shift, we might not want it to be rendered via BigPipe.

BigPipe already has non-js placeholders, for placeholders within attributes etc. which are blocking and sent with the main response. 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.

catch → changed the visibility of the branch 3493406-test-coverage to hidden.

Went ahead and made a pre-emptive MR here - just updated Klaro to the latest dev locally and ran the tests to get the new numbers.

Thought about this more. Because this should have a high cache hit rate and be on every page, it's more part of 'site chrome' than most things. So instead of trying this now, I think we should get 📌 Refactor system/base library Needs work and similar issues done first - which would result in more other js/css added via placeholders. And 📌 Implement a caching strategy for the menu links Active which would reduce the amount of HTML stored by the dynamic_page_cache.

Opened 📌 Render the navigation toolbar in a placeholder Active for the (possible) next step.

RTBC + 1. The the assert in the latest commit confirms there user, route, and query params cache contexts are not part of the cache key.

Oh I also manually checked what the cache_render cid looks like in the database, and it's this:

navigation:navigation:[languages:language_interface]=en:[theme]=stark:[user.permissions]=is-admin 

So that confirms nothing is creeping in that is per-user or per-route or anything like this. The test coverage does not explicitly test that it's not cached per-route because we hit the same page each time. We avoid per-user caching because the user menu and shortcuts links are already placeholdered within the navigation (otherwise dynamic page cache would be broken with or without this MR).

After typing out the above sentence I was about to say not sure if we need explicit test coverage for this, because pretty sure route-specific stuff is supposed to go in the top bar and user-specific stuff would probably break dynamic_page_cache entirely. But then realised it would take longer to type all that than adding the test coverage. Since that's just a one liner of extra test coverage, leaving RTBC but a +1 would be great.

Code looks good and test coverage ensures that this is going to make a difference.

I think this can be marked as RTBC.

Thank you @catch!

OK test coverage:

Had to think about where to put it, in the end I decided it should be in Navigation module because it's experimental, but that we should move it to StandardPerformanceTest when Navigation is stable (and we don't need to uninstall toolbar etc.).

First of all I wrote test coverage for the status quo in HEAD - so a 'tests-only' branch here except it should pass.

https://git.drupalcode.org/project/drupal/-/merge_requests/10546

Then I cherry-picked that test coverage into the branch and pushed, giving a test failure here: https://git.drupalcode.org/project/drupal/-/jobs/3689239

Then I updated the test coverage to match the state with the fix.

You can see 17 database queries and 31 cache gets and one cache tag checksum fetch less https://git.drupalcode.org/project/drupal/-/merge_requests/10531/diffs?c...

As you can see from the test coverage, the toolbar does get cached in dynamic_page_cache itself, but only at the level of the whole page, which means it's per URL/route, so this saves rebuilding it every time you browse to a different page.

Also apart from the menu tree/access queries and rendering, it's also saving a path alias lookup for the help link and maybe some other logic as well so I think this will probably make sense to keep even with 📌 Implement a caching strategy for the menu links Active .

I did wonder if we want to put the entire navigation block in a lazy builder so the bigpipe can placeholder it. This would increase the cache hit rate for the assets (because they'll be served in their own aggregate) and also reduce the cache size in the dynamic page cache (because it would only be a placeholder in there), but needs its own issue because it will also mean it's loaded by BigPipe which could introduce jank. Will open that separately since it's not strictly related to this.

Only moving to NW for the mentioned performance test coverage.

Very neat!

OK I don't think I've ever seen this before. Changing to a bug report. Should be relatively easy to debug at least the symptom if not the root cause by sticking a backtrace in RedirectResponse and seeing what calls it.

It's definitely not on purpose, but seems like a bug caused by the order of things.

We are setting the home page to '/home' because we can't assume the node ID of this page on install. But, I think the node doesn't exist when this config is set, so it's not connecting properly. If I simply save the site settings form, the redirect no longer occurs. (And if I change the initial config to be

'/node/2'

on install it also works as expected. But we can't do that.)

This is a pretty weird exception because you can't save the form from the UI to set an invalid path, so this is pretty hard to replicate without recipes and default content. You can reproduce it if you manually import the config with an invalid path, and then create the node with the matching alias after -- but seemingly only with Pathauto installed; it doesn't occur in vanilla Drupal, I think because in that case you are setting the alias manually on the node.

I haven't dug into this any deeper because I am sure there are others who would be much better equiped to look at how this is actually wired up in core :)

Let's postpone this on 🐛 Klaro library seems way too large? Active but there might be config or other changes required in Drupal CMS to take advantage of any improvements in the module.

Sorry if my comment could be understood wrong. I did never want to propose COOKiES over klaro, just wanted to give an alternative, if anyone needs a quickfix. I edited my comment to highlight the key point, which was meant to be:

• Let's get this fixed in Klaro lib
• We could combine both solutions if needed
• Switching to COOKiES isn't an alternative here anymore

Hope that's clearer now.

We already discussed with @jurgenhaas that COOKiES is planned to be deprecated in favour of Klaro in the future, once feature-parity is reached, and we have an upgrade path to Klaro!

Let's focus on getting the Klaro lib smaller and all will be fine? :)

PS (technical detail FYI): COOKiES 2.x is just a replacement of the JS Lib (now smaller by using Svelte), module-wise it's identical to 1.x)

Please have a look into the ADR which explains the decision on why the privacy track went with Klaro: https://git.drupalcode.org/project/drupal_cms/-/wikis/Architecture-Decis...

We've been working hard months ago and did consider the COOKiES module back then. But as described in the ADR, there have been licensing issue with the external library back then and maintainers have not been available for discussions despite the fact that we really tried hard. The fact that they now decided to go for a rewrite makes it a completely new module and it's not the time to consider a major change now that Drupal CMS is in RC phase.

As for the IS about the size of the JS library, I agree that we need to push for an optimisation there.

Thank you for raising the issue.

Code is simple and it can make a difference in the long term.

I'm not marking it as RTBC yet, waiting for the decision about test implementation.

Thanks, yes this should definitely be solved in the Klaro JS lib.

More lightweight solution can in the meantime be found in https://www.drupal.org/project/cookies/ → 2.x (Lib rewrite, because we had similar issues in 1.x) but we already discussed with @jurgenhaas that switching the solutions won't make sense at this point of time anymore. Klaro should solve it or maybe the lib should be forked for Drupal?

COOKiES 2.x Lib was rewritten in Svelte and has 12kb (34kb uncompressed) JS, 3kb (12kb uncompressed) CSS. Functionality is very very similar, both have some minor features which the other module doesn't have.

If it can't be fixed in Klaro lib, we could merge both projects and take the best parts from each and use the smaller JS lib from COOKiES.