Render the navigation toolbar in a placeholder

Created on 13 December 2024, about 2 months ago

Problem/Motivation

Spin-off from πŸ“Œ Add render caching for the navigation render array Active .

We could render the entire navigation menu via a placeholder, this would have the following benefits:

1. With dynamic_page_cache, we'd only cache the placeholder HTML in the cache item, reducing overall cache bin sizes by quite a lot.

2. With big pipe, we'd be able to send HTML and the page header, and start rendering them after. On some sites this will significantly speed up Largest Contentful Paint.

3. With big pipe, navigation-specific libraries will be added via Big Pipe in isolation from other libraries on the page, this should improve CSS and JavaScript aggregation efficiency and cache hits rates, because they won't end up bundled with other aggregates that may or may not be loaded by different front and admin-facing pages.

However there is one potential drawback:

The navigation bar is consistently in the same spot on every page and it currently loads fairly quickly. Placeholdering it may make it load later relative to other elements in the main viewport and could increase Content Layout Shift / (jank) and potentially make Largest Contentful Paint worse (contrary to the pro above) if it moves elements around that would otherwise have finished rendering.

We should be able to get an idea on the last point via manual testing between Standard, Umami and Drupal CMS.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

πŸ“Œ Task
Status

Active

Version

11.0 πŸ”₯

Component

navigation.module

Created by

πŸ‡¬πŸ‡§United Kingdom catch

Live updates comments and jobs are added and updated live.
  • Performance

    It affects performance. It is often combined with the Needs profiling tag.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @catch
  • πŸ‡¬πŸ‡§United Kingdom catch
  • πŸ‡¬πŸ‡§United Kingdom catch
  • πŸ‡¬πŸ‡§United Kingdom catch
  • πŸ‡¬πŸ‡§United Kingdom catch

    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.

  • πŸ‡¬πŸ‡§United Kingdom catch

    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.

  • πŸ‡¬πŸ‡§United Kingdom catch

    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.

  • πŸ‡©πŸ‡ͺGermany Fabianx

    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.

  • πŸ‡©πŸ‡ͺGermany Fabianx

    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.

  • πŸ‡¬πŸ‡§United Kingdom catch

    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.

  • πŸ‡©πŸ‡ͺGermany Fabianx

    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;
    }
    
  • πŸ‡©πŸ‡ͺGermany Fabianx

    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;
    }
    
    
  • πŸ‡¬πŸ‡§United Kingdom catch

    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.

  • πŸ‡¬πŸ‡§United Kingdom catch

    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.

  • πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

    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.

  • πŸ‡¬πŸ‡§United Kingdom catch

    @plopsec so #12 has a draft of what it should look like.

    We need:

    1. RenderCache::getMultiple() as a new method - this is allowed via the 1-1 rule I think. It's not strictly required to make this work, but it'll be an additional performance improvement. If it turns out to be hard we can split it to its own issue.

    2. The CachedPlaceholderStrategy itself, these needs a priority so it runs before any other placeholder strategy (e.g. so cached placeholders are replaced first, then bigpipe et al get to process what's left over).

    I am pretty sure that a working MR will cause performance tests to fail - e.g. on bigpipe warm cache requests different things will happen. If they fail in the right way and most other things pass, then it's working.

    Not sure how best to add explicit test coverage yet but that might be clearer once it's up and running.

    I'm pretty excited about the possibilities this issue opens so please ping me in slack if the above isn't useful or if something else comes up. I can't speak for fabianx but my guess is he'd be the same given it was his original idea 8+ years ago.

  • Pipeline finished with Failed
    10 days ago
    Total: 213s
    #410510
  • πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

    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?

  • πŸ‡¬πŸ‡§United Kingdom catch

    One question on the MR but it could be complete misdirection because I didn't try to verify or anything.

  • Pipeline finished with Failed
    9 days ago
    Total: 114s
    #410924
  • Pipeline finished with Failed
    9 days ago
    Total: 4176s
    #410930
  • Pipeline finished with Failed
    9 days ago
    Total: 613s
    #411034
  • Pipeline finished with Failed
    9 days ago
    Total: 111s
    #411273
  • Pipeline finished with Failed
    9 days ago
    Total: 331s
    #411279
  • Pipeline finished with Failed
    9 days ago
    Total: 459s
    #411301
  • Pipeline finished with Failed
    9 days ago
    Total: 505s
    #411351
  • πŸ‡¬πŸ‡§United Kingdom catch

    So the current placeholder implementation explicitly supports duplicate placeholders on the page like CSRF links, I think it would be a bc break to prevent that.

    I did some debugging of the original recursion bug that plopsec found, tried some different versions of the MR etc. and tried to fix it without a bc break in various unsuccessful ways.

    The recursion is definitely resulting in the big pipe placeholder strategy being run twice - once called from within CachedPlaceholderStrategy and once by itself later on. This does not happen without the recursion.

    I have read and re-read this from #9 and I can't see how it's the case:

    - 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.
    

    This MR has zero impact on a cache miss - it will behave exactly the same as now with everything streamed via big_pipe. I'm not seeing how anything can get worse on a cache hit either, placeholders within something that is returned from a cache hit will be picked up and streamed by bigpipe too - if they weren't, they wouldn't get replaced at all.

    So... I think the answer here is to remove the recursion. Would be good to get either confirmation from Fabianx that #9 was mistaken or a test case to demonstrate that there really is a problem if we don't recurse.

  • πŸ‡¬πŸ‡§United Kingdom catch

    Tagging as a release priority for navigation.

    And also as a release highlight since it's a general render caching improvement.

  • πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

    plopesc β†’ changed the visibility of the branch 3493911-add-a-cachedplaceholderstrategy to hidden.

  • Pipeline finished with Failed
    6 days ago
    Total: 462s
    #413335
  • πŸ‡¬πŸ‡§United Kingdom catch

    Briefly discussed the recursion with Fabianx in slack - he said it would only optmise an extreme edge case which may not matter in practice. I think it would be good to have a follow-up to investigate whether it's needed for that edge case, and if so how to implement it transparently without a bc break, but definitely no need to block this issue on getting it working (which I am glad about because I tried for a couple of hours or so and didn't get anywhere).

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    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!

  • Pipeline finished with Failed
    5 days ago
    Total: 371s
    #414320
  • Pipeline finished with Success
    5 days ago
    Total: 689s
    #414408
  • πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

    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.

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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.

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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!

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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 :)

  • πŸ‡¬πŸ‡§United Kingdom catch

    With navigation it might be easier to do it in a spin-off issue - it's soft-blocked on this issue because this issue fixes content layout shift from placeholders, but the implementation itself doesn't require anything here, that would allow us to work on them/review/commit in parallel. afaik the only thing we need to change for navigation is from #pre_render to #lazy_builder + setting #create_placeholder = TRUE.

    I haven't fully digested the re-use vs not question on the methods, but if we think it's better optimized to keep the logic separate, that seems fine.

  • πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

    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.

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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.

  • πŸ‡¬πŸ‡§United Kingdom catch

    Went ahead and opened πŸ“Œ Use a placeholder for the navigation toolbar Active - we can cherry-pick the commits from the MR here over to that issue.

  • Pipeline finished with Success
    4 days ago
    Total: 522s
    #415415
  • πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

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

  • πŸ‡¬πŸ‡§United Kingdom catch

    One very minor comment on the MR but no actual complaints, this is looking really good to me.

  • πŸ‡¬πŸ‡§United Kingdom catch

    Re-titling now that the navigation change is split out.

  • Pipeline finished with Success
    4 days ago
    Total: 601s
    #415841
  • πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

    Made a rebase once πŸ“Œ Use a placeholder for the navigation toolbar Active was merged and addressed the last comment in the MR.

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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.

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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.

  • Pipeline finished with Failed
    4 days ago
    Total: 128s
    #415929
  • Pipeline finished with Failed
    4 days ago
    Total: 107s
    #415932
  • Pipeline finished with Failed
    4 days ago
    Total: 109s
    #415933
  • Pipeline finished with Failed
    4 days ago
    Total: 125s
    #415934
  • Pipeline finished with Success
    4 days ago
    Total: 467s
    #415942
  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    Reviewed.

  • Pipeline finished with Failed
    3 days ago
    Total: 144s
    #416617
  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    Addressed all feedback. Some nice finds, @berdir.

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

  • Pipeline finished with Failed
    3 days ago
    Total: 452s
    #416630
  • Pipeline finished with Success
    3 days ago
    Total: 1513s
    #416634
  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    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.

  • πŸ‡¬πŸ‡§United Kingdom catch

    Just committed πŸ“Œ Drupal\Core\Theme\ComponentNegotiator::negotiate uses a lot of memory Active which may cause merge conflicts here.

  • πŸ‡¬πŸ‡§United Kingdom catch

    Yes need a rebase.

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    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.

  • Pipeline finished with Success
    3 days ago
    Total: 378s
    #417225
    • catch β†’ committed eac8181f on 11.x
      Issue #3493911 by plopesc, catch, kristiaanvandeneynde, berdir, fabianx...
  • πŸ‡¬πŸ‡§United Kingdom catch

    So glad that Drupal CMS performance testing uncovered a way to improve navigation caching which in turn resurrected Fabianx's 8 year old idea to do this, and that we got it done.

    This unblocks πŸ“Œ Create placeholders for more things Active which should in turn increase the impact of several other issues.

    Committed/pushed to 11.x, thanks!

  • πŸ‡«πŸ‡·France andypost

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

  • πŸ‡¬πŸ‡§United Kingdom catch

    Added a change record and filled out the proposed resolution in the issue summary for any git archaeologists.

    https://www.drupal.org/node/3504958 β†’

  • πŸ‡¬πŸ‡§United Kingdom catch

    Fabianx pointed out we should add some explicit test coverage, opened πŸ“Œ Explicit tests for VariationCache::getMultiple() and RenderCache::getMultiple() Active for that. There's a lot of implicit test coverage that this doesn't break anything but a more specific test would help us understand why if/when we do break it.

    Also opened πŸ“Œ Evaluate recursive placeholder replacement in CachedPlaceholderStrategy Active to evaluate the sleep(1)/recursion edge case.

Production build 0.71.5 2024