[PP-2] Use placeholdering for more elements to optimize asset serving

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