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