Create placeholders for more things

Issue summary will need a write up, but what the MR does is produce this result with asset aggregates.

To reproduce install umami, you can stay logged in as user/1. Clear caches, visit the front page, click on 'articles' in the menu.

Before:

Request 1, front page:

-rw-rw-r-- 1 catch catch  73K Apr  1 21:57 css_AD1Hn2WFqLD7fmsN-QRSIv7W-iE1t07DC546l3sXBT8.css
-rw-rw-r-- 1 catch catch  33K Apr  1 21:57 css_F6ua64ArNPnvFjJTt6gRulGbIdGiJ7kJGb2LMNHs2Xo.css
-rw-rw-r-- 1 catch catch 4.2K Apr  1 21:57 css_oSlWcVKLLDVnJnov5RFzD9u_RLcNFMy31r61RbJCgbY.css

Request 2, /en/articles:

-rw-rw-r-- 1 catch catch  73K Apr  1 21:57 css_AD1Hn2WFqLD7fmsN-QRSIv7W-iE1t07DC546l3sXBT8.css
-rw-rw-r-- 1 catch catch  33K Apr  1 21:57 css_F6ua64ArNPnvFjJTt6gRulGbIdGiJ7kJGb2LMNHs2Xo.css
-rw-rw-r-- 1 catch catch  72K Apr  1 21:58 css_ktGu9CM5DvTepixYiS5mZ3xk0zTeqc0toanu-L3COMk.css
-rw-rw-r-- 1 catch catch 4.2K Apr  1 21:57 css_oSlWcVKLLDVnJnov5RFzD9u_RLcNFMy31r61RbJCgbY.css

After, request 1 front page:

-rw-rw-r-- 1 catch catch  822 Apr  1 22:00 css_CuIYoi_uDZE8jkwxH1sRuErQL24iJh6063KTVU3GSc4.css
-rw-rw-r-- 1 catch catch  33K Apr  1 22:00 css_F6ua64ArNPnvFjJTt6gRulGbIdGiJ7kJGb2LMNHs2Xo.css
-rw-rw-r-- 1 catch catch  72K Apr  1 22:00 css_IWmmyS0P1c2cRts6TOwW9dJLirai7q-q1dI8wE8Sd1Y.css
-rw-rw-r-- 1 catch catch 4.2K Apr  1 22:00 css_oSlWcVKLLDVnJnov5RFzD9u_RLcNFMy31r61RbJCgbY.css

After request 2 /en/articles:

-rw-rw-r-- 1 catch catch  822 Apr  1 22:00 css_CuIYoi_uDZE8jkwxH1sRuErQL24iJh6063KTVU3GSc4.css
-rw-rw-r-- 1 catch catch  33K Apr  1 22:00 css_F6ua64ArNPnvFjJTt6gRulGbIdGiJ7kJGb2LMNHs2Xo.css
-rw-rw-r-- 1 catch catch  72K Apr  1 22:00 css_IWmmyS0P1c2cRts6TOwW9dJLirai7q-q1dI8wE8Sd1Y.css
-rw-rw-r-- 1 catch catch 4.2K Apr  1 22:00 css_oSlWcVKLLDVnJnov5RFzD9u_RLcNFMy31r61RbJCgbY.css

As you can see, with the MR, on the front page first request, there is one, additional, 822b aggregate. This is because the umami banner component is rendered in a placeholder (which causes its CSS to be served separately via bigpipe's AJAX usage), instead of all together with the rest of the CSS.

On the second page, with the MR, there is no additional CSS aggregate at all, because the banner CSS was served separately, and otherwise the other assets are exactly the same.

What this means is we saved the user from downloading 72kb of identical CSS.

If we later were to combine this with ✨ Allow CSS to be added at end of page by rendering assets with placeholders Active then it will have the same effect not only for auth users with bigpipe but for anonymous users too.

MR is up.

Note this doesn't work for the 'related recipes' views block on Umami content pages because that block is rendered via a layout, and layouts don't support lazy builders for blocks yet, we'd also need to add the auto placeholder logic once that's done to 📌 Support auto-placeholdering for blocks placed in Layout Builder Needs work is already open.

I am pretty sure we can write tests for this using 📌 Record total css and js file size in performance tests Fixed . The test can visit the two pages within the ::collectPerformanceData() call, then we can assert the CSS and JavaScript request number and size across the two pages.

Went to review but see phpcs errors so moving to NW for that.

📌 Record total css and js file size in performance tests Fixed now includes test coverage for this scenario, currently this is only an improvement for auth users because it relies on Big Pipe (but we can make it work for anonymous users in the future).

Before, from https://git.drupalcode.org/project/drupal/-/merge_requests/7213 which is just the test coverage.

$this->assertSame(4, $performance_data->getStylesheetCount());
    $this->assertSame(94355, $performance_data->getStylesheetBytes());
    $this->assertSame(2, $performance_data->getScriptCount());
    $this->assertSame(264076, $performance_data->getScriptBytes());

After, from https://git.drupalcode.org/project/drupal/-/merge_requests/7312 - test coverage + this issue:

   $this->assertSame(2, $performance_data->getStylesheetCount());
    $this->assertSame(45296, $performance_data->getStylesheetBytes());
    $this->assertSame(1, $performance_data->getScriptCount());
    $this->assertSame(132038, $performance_data->getScriptBytes());

Note that this causes some of the same difference in performance as 📌 Only send libraries with aggregate URLs that have the aggregate type included Needs review although not exactly, however it's for a slightly different reason and both are complementary.

What looked like a random failure in demo_umami I think is due to the MR. The test requests pages and looks for the content of custom blocks. Because those a placeholdered, they'll be rendered as no-js bigpipe placeholders in functional tests, which could mean they get sent after guzzle gets the response. Not sure I've seen that happen with big pipe specifically, only post-response tasks type stuff, so see if it actually works with WaitTerminateTestTrait...

#12 didn't work at all, so did some manual testing, #create_placeholder as implemented here is breaking content blocks.

I think we need to implement both #lazy_builder and #create_placeholder in core/modules/block_content/src/Plugin/Block/BlockContentBlock.php instead maybe.

Pretty sure this is uncovering a bug specifically in umami' page.tpl.php and nowhere else. Committed a minimal fix for this.

Views also has a bug where it doesn't correctly handle invalid plugins when #create_placeholder is TRUE, this is a 1 line change to runtime code which has test coverage (as evidenced by the MR failures), so putting in here for now.

After fixing the Umami bug, the changes are more obvious and reflect reality instead of the bug side-effects. Created a draft MR here with the test coverage from 📌 Record total css and js file size in performance tests Fixed so it's easier to see the difference.

CSS aggregate count goes up from 4-7 in one test, but the important thing here is it's not just more files vs. better cache hit ratio, with BigPipe, a block like the Umami footer which has its own CSS, only causes that CSS file to be requested when it's actually loaded, which is below the fold. So even if there are a handful more network requests, those network requests are deferred behind some more important ones.

So the differences are:

1. More CSS and JavaScript aggregates, but only a handful, aggregation is still reducing dozens of files down to less than ten.
2. Better cache hit rates for both the small and large aggregates, so less bytes transferred over mulitiple pages.
3. Page-level blocking aggregates are smaller, which should result in them arriving faster when there's a cold browser cache.
4. Component CSS and JavaScript served as late as possible in the HTML.

At the moment non-bigpipe placeholdering doesn't change how CSS/JS is rendered, but if we do that in the follow-up issue, it will have the same effect there.

Draft MR is passing now, so this is ready for review again.

catch → changed the visibility of the branch 3437499-with-tests to hidden.

Rebased now that 📌 Record total css and js file size in performance tests Fixed is in.

Test coverage here isn't actually showing an improvement yet, might need to find a better scenario to show the difference.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Got the test-only and complete MR versions in sync now. You can see an improvement for auth users, but there's a regression for anon users so needs looking into.

OK did some manual testing to see why we're not seeing much benefit here, and this will only help consistently if we also do at least 📌 Support auto-placeholdering for blocks placed in Layout Builder Needs work .

The reason is that Umami has views blocks in the main content block (which is never placeholdered) via layouts (which doesn't allow placeholdering yet), and different views blocks on different pages. This means that the extra auto-placeholdering added here doesn't affect those blocks, so any related CSS gets loaded as part of the 'main' CSS resulting in files that differ only by the CSS for those blocks. This is a very realistic situation for real sites, so Umami's 'close enough' as a test case here to show the problems.

I diffed a 47k CSS aggregate vs a 48k CSS aggregate with this MR applied and the only difference was css/components/blocks/recipe-collections/recipe-collections.css which is shown on one page and not another one I was using for testing.

If we had 📌 Support auto-placeholdering for blocks placed in Layout Builder Needs work that file would be served separately, and we'd have two 47k CSS aggregates, an an extra 1k CSS aggregate loaded on one of the pages. There might be other, similar issues, but need to get to at least that baseline.

Not marking this postponed though because there are some bugfixes here that can be split out to other issues, as well as the extra test coverage.

Issue summary:

Those 72k and 73k aggregates are nearly identical, except for the Umami banner SDC (which is a content block near the top of the front page).

😱

#11 looks epic/very convincing … but was impacted by a bug. #17 contains the actual truth. Still very convincing.

#25 made me aware of 📌 Support auto-placeholdering for blocks placed in Layout Builder Needs work which is also very interesting 🤓

It's been many years since I saw this code, so at first glance I'm having a hard time understanding the changes MR 7360 is making in AssetResolver. Sprinkling some explanatory comments would probably sufficient for me (and others) to understand what's really changing now :)

The AssetResolver changes have already landed in other issues, MRs here were stacked on older versions of those changes, see 📌 Only send libraries with aggregate URLs that have the aggregate type included Needs review and then 📌 Optimize AssetResolver caching Fixed for the issues that did that.

I've rebased https://git.drupalcode.org/project/drupal/-/merge_requests/7280 without any of the test changes. This should result in some test failures.

OK re-did the basic test fixes to reflect the additional placeholders/aggregates, so tests should hopefully be green now.

However I did not yet do the change to the asset aggregation across pages test to hit extra URLs - that change is necessary to get any kind of different numbers in byte size here and is what's in the other MR. To avoid future rebase hell, I think it'd be better to extract that to its own issue first, then maybe bring it to the MR here stacked until it lands.

There's also the Umami theme and database logging changes to split out still too.

"the other MR" == ✨ Allow CSS to be added at end of page by rendering assets with placeholders Active ?

Looks like this MR confirms my hunch at #3439017-5: Umami page.tpl.php breaks #create_placeholder for blocks → .

Split the Umami theme change out to 🐛 Umami page.tpl.php breaks block placeholders Needs review .

Split the test improvements change out to 📌 Add extra page request to the across pages asset performance test Active .

No longer postponed, but needs a rebase after OOP hooks.

This is a prerequisite for 📌 Try to replace path alias preloading with lazy generation Active .

Also would work well with 📌 Render the navigation toolbar in a placeholder Active .

Following, just a drive-by-comment for now.

Could we tackle this from a difference perspective as well, possibly in a different issue. Essentially. we'd add the ability to flag certain libraries as "this will be added on every page", which we already do with the global/default library thing of themes. We typically put most theme CSS into one big library and load some (or maybe a fair bit..) of CSS that not every page needs. But it's a large chunk that we know is going to be the same for all frontend pages.

We could then put all those into its own aggregate chunk which we can safely assume is going to be identical for all pages. Will only reasonably work those libraries are together in the same chunk.

So we used to have that until #977844: Remove the 'every_page' option for CSS/JS assets: it is confusing, even damaging → which removed it, I've been slowly deliberating whether we should bring it back but haven't opened an issue yet.

The problem was that if you had a handful of libraries that were 'every_page' but not really, they'd cause duplication in the 'every_page' aggregate. And conversely if you had libraries that actually were every page but weren't marked as such, they'd be duplicated in the 'not every page' aggregate. But... removing the option didn't really result in less duplication as this issue shows, and we have more tools than we had then.

Will probably open an issue up and link back to/from this one, it's worth looking at again I think.

Since I've found another use-case for this, altering the issue title.

Opened 📌 Consider adding back the 'every page' concept to libraries Active .

Turns out this is potentially useful for a lot more than just asset aggregation hit rates, opened 🌱 [meta] Placeholder-driven performance improvements Active to summarise various findings from other issues.

I've rebased this with one change. Added a 'createPlaceholder' method to BlockPluginInterface/BlockPluginTrait which should be allowable under the 1-1 rule, this means modules don't need to implement an alter, just a method. Defaults to FALSE.

Otherwise it still has one change to make a views broken block plugin test module not blow up, and updates performance tests to show the asset hit rate improvement.

Will need a change record/release note now to document the new plugin interface method, but could use review.

Hopeful that this will be enough to enable better testing of 📌 Try to replace path alias preloading with lazy generation Active .

catch → changed the visibility of the branch 3437499-moar-pages to hidden.

This appears to need 📌 Deprecate RendererInterface::render()'s sole $is_root_call parameter Needs work at least when combined with more Fibers usage in 📌 Try to replace path alias preloading with lazy generation Active .

What about adding this to local tasks and breadcrumbs? Both tend to be fairly slow with complex cacheability and breadcrumbs often also contains multiple links that need aliases to be resolved that aren't used otherwise.

Another thought, what if we expand the scope of the new method to not just returning true or false but possibly other, maybe even multiple caching related flags?

Context: For 📌 Make language switcher block cacheable Postponed , I'd like to have a way to say that the language block should not be attempted to be cached because the amount of variations makes that not worth it. But it shouldn't prevent the whole page from being cached.

What about adding this to local tasks and breadcrumbs? Both tend to be fairly slow with complex cacheability and breadcrumbs often also contains multiple links that need aliases to be resolved that aren't used otherwise.

Yes definitely :)

For the language switcher stuff, this is a way to say that cache keys should not be set rather than max-age: 0 right?

> For the language switcher stuff, this is a way to say that cache keys should not be set rather than max-age: 0 right?

Yes, exactly. I've wondered about solving this by adding another negative value (-2 or so) to the max age system, but I'm concerned about BC and not sure if there really are other use cases for this outside of block plugins.

Another use case for that are very simple block plugins where I can't imagine that loading a cache from mysql/redis is faster than just rendering it. A core example would be \Drupal\system\Plugin\Block\SystemPoweredByBlock. Especially when it's combined with some dynamic context like the current path. A real world example for that that I've encountered a few times are ad placeholders that are just a single div with some classes or data attributes, they might be very dynamic (based on path, possibly even the user if you for example have a system where paying users don't get ads) but caching them is overkill.

Not saying this should solve this here, just wondering if we want to consider a more generic method. Or we don't, if we can add this then we can also add another specific method, might be better DX.

I think we could add an additional method like ::cacheIndependently() maybe for that, it might be clearer.

One concern I have with this is that the placeholdering will cause layout shift. Sites with very large/complex menus that currently take a considerable amount of the total render time might run into this.

Did a bit of manual testing on a client project, and it doesn't seem as bad as I expected, but if I enable placeholdering for all blocks for example it's clearly visible.

One idea I had is that we introduce a new flag next to create_placeholder that's a bit similar to loading=eager/lazy on images. For things that are above the fold, we can define to placeholder it but still render within the initial response. if I understand correctly then this wouldn't actually bring the benefits you're looking for yet and we'd need the followup. We could then include that by default for the blocks we change for now and contrib could extend and make that configurable per block or something. Things like the footer navigation are then very obvious targets to render later, while the main navigation probably should be there from the start.

One concern I have with this is that the placeholdering will cause layout shift. Sites with very large/complex menus that currently take a considerable amount of the total render time might run into this.

I think we can resolve that with 📌 Render the navigation toolbar in a placeholder Active - then the layout shift would only happen on cold render caches, and big pipe wouldn't run at all on warm render caches.

The one drawback of this is that the original reason I opened this thread was to rely on how bigpipe renders assets to reduce duplication between asset aggregates, but we can handle that differently (possibly by collecting and serving asset aggregates from placeholders separately when big pipe isn't enabled). Some discussion of that in ✨ Allow CSS to be added at end of page by rendering assets with placeholders Active but not sure what an actual good approach looks like yet.

We could then include that by default for the blocks we change for now and contrib could extend and make that configurable per block or something. Things like the footer navigation are then very obvious targets to render later, while the main navigation probably should be there from the start.

This probably wouldn't be necessary with the CachedPlaceholderStrategy, but I think it would be very useful for the rendering CSS later issue (if we want to try that).

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

This still needs a CR. And the issue summary could be improved.

After reading in particular #47 and #48, I wonder we can change the status to 'Postponed' or 'Postponed (maintainer needs more information': progress seems to be waiting for maturation of other issues like: https://www.drupal.org/project/drupal/issues/3493911 📌 Render the navigation toolbar in a placeholder Active and #2989324: Allow CSS to be added at end of page by rendering assets with placeholders.

Tried to rebase. It was rejected due to merge conflict in '...src/FunctionalJavascript/AssetAggregationAcrossPagesTest.php'. Selected 'ours' to fix the merge conflict.

Updated the issue summary because this issue in itself won't change css/js aggregation much, but it does have benefits for other things.

Rebased again.

Got here from 📌 Make language switcher block cacheable Postponed but noticed it needs a rebase. Wonder if during the rebase if also can get the CR?

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

Can we think of a use case where this could be dynamic based on block configuration or something?

Before the CachedPlaceholderStrategy I would have said yes, or at least use-cases for sites to customise per block so that blocks rendered above the fold don't go through big pipe, but the possibility of maybe wanting to configure it for menu blocks or views blocks down the line.

But afterwards I'm not sure there's a use case for that - and site specific customisations could still happen in the alter hook if people come up with them.

@smustgrave Sorry I just saw the timestamp on #55. I was sure it was more than 48 hours ago. Just rebased. Fixed conflict that was in only one line of one of the 2x telemetry test files in umami profile FunctionalJavascript tests. I changed the actual value from 7 to 4 because 4 was the expected value. With the value 7 the test failed with the value 4 it now passes.

Not certain that this was the correct action to take from a technical perspective. Should not have done it anyway as should have allowed 48 hours to elapse.

You’re good!

For the interface name, how about Drupal\Core\block\AlwaysPlaceholderInterface?

Unlike CacheOptionalInterface I can't really see us having a similar situation elsewhere because the vast majority of code can set #create_placeholder => TRUE if it wants to, and if we do, we can move the interface and deprecate this one.

Can we think of a use case where this could be dynamic based on block configuration or something?

I mean, it's not unthinkable that someone might want this flexibility. Switching to an interface means it's always on or always off.

Imagine you have a block that can talk to one of many sources to get its data and you can configure it to use one of these sources. Now, some sources come at a significant cost compared to others, so I could imagine that whoever wrote that block plugin wants to make sure that using one of these sources leads to the block being placeholdered.

@kristiaanvaneyde so it's possible to do it in an alter hook - that's what I did in very early versions of the MR, but it's then disconnected from the block definition and would be fairly hard for someone to figure out exactly which hook to do it in. If we think there's a good use-case, we could just keep the method approach as-is. Definitely don't want to try converting over if we end up converting back again.

I'll go ahead and add local tasks and breadcrumbs per #42 using the current approach.

The realist in me says: "There's very few people who actually know what lazy builders are and how/why to use them." So that part of me would tell you to go right ahead with the interface.

Then the pragmatic side of me says: "If either approach is fine and easy to implement, then why not go with the one that gives implementers more freedom?" This is more of a gray area compared to what we did with CacheOptionalInterface, where it really was a rather binary choice.

So I'd say let's keep the method approach.

Added the local tasks and breadcrumbs blocks.

Also we have new performance test assertions and this has an interesting effect on cache tag queries, but that effect will mostly or entirely disappear after 📌 Use a single per-theme cache tag for block config entities Active .

Looks good to me. Test failures seem to be unrelated, will rerun the pipeline.

Settings the local tasks block to be placeholdered, leads to the test failure. The body of the page seems to return a stale cache now that the local tasks block is no longer there.

Before placeholdering local tasks:

            <tr>
              <td>French</td>
              <td>  <div class="dropbutton-wrapper" data-drupal-ajax-container>
                <div class="dropbutton-widget"><ul class="dropbutton"><li><a href="/entity_test/structure/article/fields/node.article.translatable_field/translate/fr/edit" class="use-ajax" data-dialog-type="modal" data-dialog-options="{&quot;width&quot;:880}">Edit</a></li><li><a href="/entity_test/structure/article/fields/node.article.translatable_field/translate/fr/delete" class="use-ajax" data-dialog-type="modal" data-dialog-options="{&quot;width&quot;:880}">Delete</a></li></ul></div>
              </div></td>
            </tr>

After placeholdering local tasks:

            <tr>
              <td>French</td>
              <td>  <div class="dropbutton-wrapper" data-drupal-ajax-container>
                <div class="dropbutton-widget"><ul class="dropbutton"><li><a href="/entity_test/structure/article/fields/node.article.translatable_field/translate/fr/add" class="use-ajax" data-dialog-type="modal" data-dialog-options="{&quot;width&quot;:880}">Add</a></li></ul></div>
              </div></td>
            </tr>

So my guess is the translation UI doesn't add all the necessary cacheable metadata and got saved because of the local tasks containing the metadata that the translation UI should be setting. Now that this metadata is stripped due to placeholdering, the translation UI breaks.

Yeah if I set that method to add max-age 0, the tests go green.

The local tasks block had the following:
Contexts:

• languages:language_interface
• route
• user.permissions

That's to be expected so probably not what's missing.

Tags:

• local_task
• config:field.field.node.article.translatable_field

The first is probably not the cause but the second seems like our culprit. Adding that hard-coded makes the test pass. So now to figure out how to add that tag (or multiple sometimes?) dynamically.

We should probably placeholder the primary admin actions block as well as local tasks and breadcrumbs for similar reasons. Although given the above I also wonder whether we should split those three out to a follow-up?

The test failure highlights another benefit here - every placeholdered block's cache tags get 'contained' within the placeholder and don't get added to the dynamic_page_cache entry. This should mean less invalidation of dynamic page cache entries, especially when it's something like node:list etc.

Fixed it, but the changes in config translation UI might warrant their own issue, which would block this issue. What do others think?

We should probably placeholder the primary admin actions block as well as local tasks and breadcrumbs for similar reasons. Although given the above I also wonder whether we should split those three out to a follow-up?

I don't mind dealing with them in one go here, it's just that it might expose caching issues in seemingly unrelated code as demonstrated by comments 65 through 69. Fixing those first would require we write tests for them, delaying this issue more.

In case of the translation UI, we could call Renderer::renderInIsolation() on the $build generated by ConfigTranslationController::itemPage() to see if we add the correct cacheable metadata but that seems like testing the solution, not the problem. The problem is the page getting stale and to test that we'd need to test DPC itself, which is not something a test suite for a page controller should be concerned with.

which is not something a test suite for a page controller should be concerned with.

I think that given the test failed, there is implicit test coverage already, which is probably enough for a controller. If we split both the placeholdering and the fix to its own issue, it could be postponed on this one maybe, but fine doing it here if others are.

Pushed a commit to placeholder the local actions block.

I'll split off the ConfigTranslationController bugfix so that people using git blame (or log) can find a dedicated issue instead of stumbling upon a commit that has seemingly nothing to do with the translation UI. Still fine keeping the other changes contained here as they are related.

See: 🐛 ConfigTranslationController::itemPage() does not specify cacheable metadata Active

This will need a rebase after 📌 Remove cache tag checksum assertions from performance tests Active but it's reviewable again.

Also test changes will conflict heavily with 📌 Use a single per-theme cache tag for block config entities Active but no dependency either way.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

I noticed a bunch of commits got added from a branch other than 11.x as the diff showed all of those changes. I was fortunate enough to have the last "real" commit on my local so I merged in origin/11.x and force pushed that here.

Only failing test is AssetAggregationAcrossPagesTest on something you added @catch: 'StylesheetBytes' => 89000,. I've taken the 86642 the tests report and rounded it up to 86650, feel free to correct if necessary.

Just beat me to the same change by 45 seconds, that looks fine.

All green again 🎉

This looks good to me, does need a CR, but then I can RTBC this.

The performance tests show a bit of a tradeoff, we have quite a few extra render cache lookups with this, we do save a few cache tag lookups though. However, this opens up the door for several improvements to do more with fibers and multiple-loading, hopefully fewer invalidations due to views blocks and so on.

Added a change record. Agreed on the trade-offs with extra cache gets, one thing to add is that with a dynamic page cache hit these are multiple cache gets now (due to the cached placeholder strategy) so it's mostly neutral there (as long as the page already had at least one placeholder).

Added a change record.

> hit these are multiple cache gets now

True. I did look into this a bit, as \Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryAuthenticatedPerformanceTest::testFrontPageAuthenticatedWarmCache reports two render cache lookups. But that's because of cache redirects on the block_content blocks.

Thinking about it some more. The multi-load for a cached page is fine, but in testAnonymous(), we have +10 cache lookups in case of an empty cache.

When I set render cache to NULL, set a breakpoint in NullBackend::get() for primary_local_tasks cid I get there 3/4 times. Once through recursive rendering initialized from page.html.twig, then it placeholders it. Then a getMultiple() for that same cid among others. Then again from bigpipe, with the same cache lookup, then it builds the block, and then again to figure out the redirect chain while setting the cache.

That isn't new, but with olivero, we now do this for 6 additional blocks/placeholdered elements.

There is a scenario with multiple requests that some of these blocks have indeed in the meantime been cached by a separate process, but in most cases, that won't happen. Should we add some static caching, specifically for cache misses and maybe cache redirects? that shouldn't cause only minimal memory overhead and if there is a cache hit we can assume it won't hit it again.

I'm also confused why we even do a cache lookup in \Drupal\Core\Render\Renderer::doRender() for a placeholdered block? shouldn't we skip that?

I think if we do those two things this might actually result in a reduction of cache lookups because CachedStrategy/RenderCache::getMultiple() will fill the static cache with the misses, and then later calls already know those are a miss?

I'm also confused why we even do a cache lookup in \Drupal\Core\Render\Renderer::doRender() for a placeholdered block? shouldn't we skip that?

The actual rendered output of something that got placeholdered can still be cached when the thing had cache keys, even if it varies by user or something else we normally don't want to bubble up to DPC. Blocks meet those criteria.

So when we encounter a placeholder in the attachments, it contains the cache keys and we take that to the Renderer to see if it can find whatever we want to replace the placeholder markup with in the placeholdering render cache.

If we didn't do this, we would always have to calculate placeholdered stuff live, even though it's possible to cache those calculations. We just don't want their cacheable metadata to "poison" the entire page, which is why we placeholder them.

Trying to come up with an answer for your debugging...

Then again from bigpipe

This bit surprises me, as we now have CachedStrategy and that one should hit warm caches by the time we get to them and as a result stop big pipe from looking up the placeholders. I'm thinking this is because you're using NullBackend for debugging and therefore CachedStrategy can't prevent big pipe from running.

But yeah, generally what happens is this:

1. We try to render the block, a first cache lookup happens for the rendered output of the lazy builder
2. Regardless of whether we get a cache hit or miss, the block is placeholdered either in the Renderer (miss) or PlaceholderingRenderCache (hit) because we don't want it to bubble up. So when we get to the final part of the render process, we have to do another cache lookup to see if the placeholder's actual output is in the cache. However, the PlaceholderingRenderCache has an optimization for this and keeps the placeholders it already found or set in memory so we don't repeat the work.
3. During the above lookups and/or writes, VC will try to follow or construct a redirect chain, leading to a few lookups on the underlying cache backend.

1. and 2. mean that for any placeholdered block you will always have two render cache cache lookups, but only one of them ever goes to the actual underlying cache backend.

Let's assume most blocks have 1 cache redirect because the lazy builder added extra contexts, so in total we're talking about:

1. Cold cache GET: 1 lookup in VC for get. 1 lookup in VC during SET. 0 lookups during replacement because of optimization in PlaceholderingRenderCache.
2. Warm cache GET with 1 redirect: 2 lookups in VC for get. No set. 0 lookups during replacement because of optimization in PlaceholderingRenderCache.
3. Invalidated warm cache GET with 1 redirect: 2 lookups in VC for get, but it fails to find the entry. 2 Lookups during SET as it follows the previously created redirect (which does not get flushed). 0 lookups during replacement because of optimization in PlaceholderingRenderCache.

So that's 2 lookups on both a cold and a warm cache or 4 after invalidating a warm cache. However, if the blocks weren't placeholdered, we'd still have the initial cache get regardless so we're really only talking about up to three extra cache gets for every placeholdered block. usually it will only be one extra.

In the MR we placeholder 5 blocks and we're seeing 10 extra cache gets. So that could come from the above explanation.

P.S.: While writing this I realized we can optimize the redirect chain fetching in VC by storing it in memory and flushing it whenever a set on the same cache keys is done. That would reduce the amount of cache gets for scenario 3 above.

Actually, the docs in PlaceholderingRenderCache seem off. Because we don't write the placeholder output until we start rendering the placeholder. At that point there should be no more GETs for said placeholder. Only during subsequent requests.

So it seems the optimization on PRC::set() does nothing. Need to look into that...

Spin-offs from recent discussion:

• ✨ Optimize placeholder retrieval from cache Active
• ✨ Optimize redirect chain retrieval in VariationCache Active

Now that we identified what's going on, I think this can go back to RTBC.

With the current state in all those 3 issues combined, the diff is:

-    'CacheGetCount' => 122,
+    'CacheGetCount' => 108,

(So instead of the current +10, a -14)

But I think it should be possible to eliminate an additional another 7 or so queries in ✨ Optimize redirect chain retrieval in VariationCache Active once we support VariationCache::getMultiple() as well. Didn't manage to make it work yet, not in tests anyway.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Conflict is with performance tests after 📌 By default, don't aggregate jquery.js Active , rebasing now.

Rebased, only test updates again so back to RTBC.

The measurable performance improvement seen in the test coverage is: https://git.drupalcode.org/project/drupal/-/merge_requests/7280/diffs?fi...

e.g. on big pipe requests more assets get served by big pipe allowing the page to be rendered faster, particularly on cold caches and with less duplication between aggregates.

The extra cache gets look like a small regression but because we are placeholdering more things, it will result in higher cache hit rates to the dynamic page cache, which should be an improvement on real sites.

✨ Optimize redirect chain retrieval in VariationCache Active and ✨ Optimize redirect chain retrieval in VariationCache Active combine with this issue to result in a net reduction in cache gets overall, including the ones added here but also additional ones, but shifting more responsibility onto the new CachedPlaceholderStrategy.

Then there are future optimisations on top of this issue like async, 📌 Try to preload cache tags in cache getMultiple Active , path alias and entity preloading which are in the related issues. Some of those are close, some will take a while to complete, but several different strands are starting to come together which overall should result in a significant performance improvement both for cold and warm caches and a good net improvement for both in 11.2 already.

Needs another rebase after 📌 Add assertions to OpenTelemetryNodePagePerformanceTest::testNodePageWarmCache() Active , I'm also almost certain that the query list added there needs a major reshuffle after this issue because a lot of them are triggered by the local tasks block.

Rebased again, could have been worse, only three queries moved position in the list.

Test fails look random, don't have enough permissions to retest, but changes look good. And yes, query changes weren't so bad since a large chunk just moves down. I already had to reroll 3 or 4 issues at this point that make changes on the list, so we're going to need a lot of rerolls around all those performance issues.

I might open a separate issue to change the query list asserts to use $this->assertEqualsCanonicalizing($expected_queries, $recorded_queries);. From what I've seen, that results in output that's much easier to parse as it doesn't compare the array keys. but I need to check how it deals with order changes and so on.

I'd also thought about array_intersect() which would pass when queries are removed but fail when we're added, with the drawback it wouldn't detect removals and we'd end up with cruft until we redid things - but ignoring the keys sounds like a much better idea.

Looks like assertEqualsCanonicalizing() isn't as nice as I thought. it ignores order, seems to even sort the array in fact, so that would be quite annoying to resolve.

An option might be to just compare the whole thing as as string?

$this->assertSame(implode("\n", $expected_queries), implode("\n", $recorded_queries));

gives me the following when switching order of two queries:

--- Expected
+++ Actual
@@ @@
 SELECT 1 AS "expression" FROM "path_alias" "base_table" WHERE ("base_table"."status" = 1) AND ("base_table"."path" LIKE "/node%" ESCAPE '\\') LIMIT 1 OFFSET 0
 SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "core.entity_view_display.user.user.compact", "core.entity_view_display.user.user.default" )
 SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "filter.format.restricted_html" )
+SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "system.image" )
 SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "user.role.authenticated" )
-SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "system.image" )
 SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "theme:stark" ) AND "collection" = "config.entity.key_store.block"

could have an assertQueries() method on the PerformanceTestTrait that does that? Can open an issue if you think that's useful.

@berdir Or assertEquals()? assertEquals
There is an example at 1.9 of usage applied to arrays.

We could do both array_diff and array_intersect maybe, let's open a spin off to have a look.

Merged in 11.x again to resolve conflicts.

Committed bc383a3 and pushed to 11.x. Thanks!

quietone → closed merge request !7280