- Issue created by @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
- SingleFlushPlaceholderStrategyThe 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 thecurrent_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.
- Merge request !11064Draft: Issue #3493911: Add a CachedPlaceholderStrategy and use a placeholder to... β (Open) created by plopesc
- πͺπΈ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
- Checkout the MR branch
- Install Standard profile and Navigation module
- Login as admin
- Go to the homepage while the cache is cold, and the page is loaded as expected
- Reload the page, once the cache is warm
- 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.
- Merge request !11069Issue #3493911: Add a CachedPlaceholderStrategy and use a placeholder to... β (Closed) created by catch
- π¬π§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.
- π¬π§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!
- πͺπΈ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.
- πͺπΈ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.
- πͺπΈ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.
- π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
Addressed all feedback. Some nice finds, @berdir.
Code looks a lot cleaner after this last round of feedback too :)
- π¨π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.
- π¨π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.
- π¬π§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.
- π¬π§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.