#Performance

Can't decide where to post this in the myriad of new issues opened, so I'll just do it here since it's related to #8.1.

Redirect's RouteNormalizerRequestSubscriber has a much bigger problem, it completely disables core's alias preloading mechanism: 🐛 Feature or Bug? - The path-alias preloading is not used if the "Enforce clean and canonical URLs" option is enabled Active

On the site where I discovered this (and had me digging through a lot of rabbit holes), we've disabled the Enforce clean and canonical URLs setting from the Redirect module, and went with this core change instead: #2641118-178: Route normalizer: Global Redirect in core → (MR link)

@catch, for #8.2, there's a very old core issue trying to change this behavior: 🐛 Aliased paths cannot be set as front page Needs review

That issue now competes with 🐛 Updating page.front Active for the decision whether storing an alias in config is ok or not :)

Ohhh I didn't realise that #1 meant that Redirect is actually causing this behaviour. So the core bug is only an indirect cause.

Test fixed at 🐛 Navigation Top Bar hides entity local tasks even if the user has no access to the bar Active commit 738036cc.

Opened 🐛 RouteNormalizerRequestSubscriber redirects away from the front page when it's set to an alias Active too for the redirect issue.

Created 🐛 Updating page.front Active

Issue come from 9177c489 and issue 🐛 Navigation Top Bar hides entity local tasks even if the user has no access to the bar Active . You can check pipeline. We can just change number of assertion to 61.

📌 Get coffee data only when the search box is opened Active got in.

Merged into 1.x. Thanks!

Green!

Content editors still have a very large amount of JavaScript, but it's not from Klaro, it appears to be mostly because both navigation and toolbar are loaded at the same time, which I think is a Gin issue?

I didn't have enough time to explorer, but I get same error on my localhost. I believe this isn't random failure. I think I will increase count, but I want to find the reason. Thanks for your suggestion.

Thanks catch and kumareshbaksi! I checked the MR and did a local test, everything is looking good to me. Will merge it now to 2.x.

Klaro tagged a new release and performance tests are now (expectedly, beautifully) failing in HEAD: https://git.drupalcode.org/project/drupal_cms/-/jobs/3776271

Let's get this done!

Postponing this on 📌 Implement caching mechanism Active and 📌 Get coffee data only when the search box is opened Active - those can be done without complex complex cache invalidation logic (or cache staleness bugs), and should make this less necessary. If it's still necessary after those, then dynamic_page_cache support would allow for a short TTL here without much impact on server load.

Something similar happened consistently to me when rebased ✨ Allow modules to hook into top of content/footer sections of new core navigation Active . But assumed that extra cache get would be related to the extra hook invocation added in the MR and bumped the count in the test .

Just to mention it here, maybe unrelated to the reason mentioned above.

@nikolay shapovalov it's possible that's a random failure but this is the first time I've seen it reported anywhere, just re-ran the test over there. If it happens again, we should make the assertion between 60 and 61 then open a follow-up to try to track down what's changing.

There might be at least three bugs here:

1. Redirect module doesn't check for '/' before redirecting away from the front page, but if anything it should redirect /home back to the front page. It could also save all the lookup logic in RouteNormalizerRequestSubscriber if it short-circuited on '/'.

2. PathMatcher::isFrontPage() only checks the configured string of the front page path, not the URL it resolves to etc. This might be 'by design'.

3. Core should have a config listener that resolves aliases to system paths (where they exist) instead of the logic happening in the form.

Both #1 and #3 seem like potentially quick fixes that could go into patch releases fwiw.

My tests fail at latest 10154d1a commit 📌 Move helpers in node_access_test.module and delete it Needs work .
Change $this->assertSame(61, $performance_data->getCacheGetCount()); fix the error.
I am not able to find any updates about this. Is it something on my side?

Right, if nothing else, here is what we could do with ECA:

Listen to the config save event and if it's about system.site, move on: Read the page.front value and verify if it's an alias. If so, find the canonical path for it and override the value in the config.

LMK if you want to have an ECA model for that.

The MR has been tested and works as expected. The unnecessary AJAX request on every page load is removed, and the request is now triggered only when the Coffee search box is opened.

This resolves performance concerns for both low-traffic and high-traffic sites while maintaining the same functionality and user experience.

Marking as RTBC and attaching screen recordings for reference.

Annoyed enough to actually look into it. So it's not recipe specific, and it's not because the page didn't exist when the config was set. You can reproduce it by setting page.front to any alias using drush, and you'll get the same redirect behaviour.

The form validation swaps the alias with the path, so this isn't running if the config is changed outside of the form. Wondered why I never saw this before but I realised that we would always set page.front to a node path on a project, rather than an alias.

I'm not going to go any deeper into the specifics of the redirect logic, but safe to say this is a core thing. Not sure whether to move this into the core queue or create a new issue for that, because I'd like to resolve this for Drupal CMS with a workaround in the meantime. I strongly suspect that ECA can come to the rescue here as a last resort.

Not surprised you haven't seen it, before recipes it'd be nearly impossible to create this scenario without a great deal of effort. But now that I'm aware of it, it's really annoying me.

reroll to apply to 7.103

Bumping to major because this is potentially a big Content Layout Shift improvement for all big pipe responses, it will also work very nicely with the cold cache optimisations that 🌱 Adopt the Revolt event loop for async task orchestration Active will enable.

Discussed with @plopsec and I'm moving the navigation stable blocker tag to this issue from 📌 Implement a caching strategy for the menu links Active .

📌 Add render caching for the navigation render array Active just landed which unblocks this issue.

We might need to split this issue into separate 'add the API' and 'use a placeholder' issues, but since the end goal of this issue is that there's zero impact on the front end, except for a render cache miss served by BigPipe, not sure how to test all of this yet, so will be nice to be able to piggy-back off the existing test coverage and manually test etc.

I think we could probably backport this to 11.1.x given that navigation is experimental and there's no bc implications here.

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.

This has tests and multiple folks have manually tested this. LGTM.

heddn → closed merge request !60

With D7 EOL approaching in a month I'm starting to triage the D7 side of quicktabs queue.

If still an issue or needed for 4.0.x (latest branch) feel free to reopen

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.