Use placeholdering for more elements to optimize asset serving

Created on 1 April 2024, 11 days ago
Updated 8 April 2024, 5 days ago

Problem/Motivation

Follow-up from #769226: Optimize JS/CSS aggregation for front-end performance and DX which is very old but we still have some of the same problems. #977844: Remove the 'every_page' option for CSS/JS assets: it is confusing, even damaging also has lots of background.

This is a spin-off from Allow CSS to be added at end of page Active since we will need to do this issue to make that one effective.

Drupal's CSS and JavaScript aggregation puts files into one or two aggregates per page. A major problem over the years has been duplication between aggregates.

i.e. visit the front page of the site, and the aggregate will contain a mixture of site-wide CSS and CSS that is specific to the front page. The visit a node page, and the aggregate will contain a mixture of site-wide CSS and CSS that is specific to the node page. The result can be that instead of downloading 50kb +10kb + 10kb of CSS you're now downloading 60kb + 60kb of CSS. The same problem exists for JavaScript.

Steps to reproduce

Even with BigPipe enabled, a lot of content is not rendered via placeholders, we currently only do that to improve cache hit rates, not to optimize asset handling. Therefore you can see the original problem described in #769226: Optimize JS/CSS aggregation for front-end performance and DX with Umami.

1. Browse to the front page - note the three CSS aggregates created.
73K css_AD1Hn2WFqLD7fmsN-QRSIv7W-iE1t07DC546l3sXBT8.css
33K css_F6ua64ArNPnvFjJTt6gRulGbIdGiJ7kJGb2LMNHs2Xo.css
4.2K css_oSlWcVKLLDVnJnov5RFzD9u_RLcNFMy31r61RbJCgbY.css

2. Click on 'Articles' - note
73K css_AD1Hn2WFqLD7fmsN-QRSIv7W-iE1t07DC546l3sXBT8.css
33K css_F6ua64ArNPnvFjJTt6gRulGbIdGiJ7kJGb2LMNHs2Xo.css
72K css_ktGu9CM5DvTepixYiS5mZ3xk0zTeqc0toanu-L3COMk.css
4.2K css_oSlWcVKLLDVnJnov5RFzD9u_RLcNFMy31r61RbJCgbY.css

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

Proposed resolution

BigPipe already has a mechanism to fix this problem, but in practice it doesn't actually fix it in a lot of cases yet.

When BigPipe is enabled, assets that are attached within in process of rendering a placeholder are rendered with the placeholder instead of added to the main aggregates that are sent with the initial page response.

The page starts to render with only the CSS needed for the initial response before the placeholders are rendered. This allows this CSS to be downloaded and parsed while the rest of the page is still rendering, allowing for faster First Contentful Paint and Largest Contentful Paint.

Assets that are only needed by placeholders get put into their own placeholder-specific aggregates. This would mean for example that if you had a js slideshow that only shows up on the front page below the fold, the CSS and JavaScript would get loaded only once everything above the fold has been rendered (give or take).

Because the slideshow CSS and JS is now in its own aggregate, if you browse to the front page, then another page on the site, where the only different assets are rendered via placeholders, the 'main' aggregates (also the ones the block the initial page render) are cached. No more downloading of duplicate CSS rules in a different file.

With the Umami banner situation, if we force the banner block to be rendered as a BigPipe placeholder, then the assets for that placeholder end up in their own aggregate, and there is no more duplication between the front page and articles pages.

The MR is a proof of concept and not committable, however one possibility would be for block_content module to alter hook_block_build_block_content_alter() so that all of its blocks get #create_placeholder => TRUE.

We should also do the same for all views blocks.

Once we've made this change, if we then do Allow CSS to be added at end of page Active then this will also have the same effect for non-BigPipe responses - either when the module is off, or for anonymous users.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

📌 Task
Status

Needs work

Version

11.0 🔥

Component
Render 

Last updated about 14 hours ago

Created by

🇬🇧United Kingdom catch

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @catch
  • 🇬🇧United Kingdom catch

    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.

  • 🇬🇧United Kingdom catch
  • 🇬🇧United Kingdom catch
  • Status changed to Needs review 11 days ago
  • 🇬🇧United Kingdom catch

    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.

  • 🇬🇧United Kingdom catch

    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.

  • Status changed to Needs work 11 days ago
  • 🇺🇸United States smustgrave

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

  • 🇬🇧United Kingdom catch
  • Status changed to Needs review 9 days ago
  • 🇬🇧United Kingdom catch

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

  • 🇬🇧United Kingdom catch

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

  • Status changed to Needs work 7 days ago
  • 🇬🇧United Kingdom catch

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

  • Status changed to Needs review 7 days ago
  • 🇬🇧United Kingdom catch

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

  • Pipeline finished with Canceled
    7 days ago
    Total: 1676s
    #139319
  • Pipeline finished with Failed
    7 days ago
    #139330
  • Pipeline finished with Failed
    7 days ago
    #139343
  • 🇬🇧United Kingdom catch

    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.

  • Pipeline finished with Canceled
    7 days ago
    Total: 525s
    #139360
  • Pipeline finished with Success
    7 days ago
    Total: 660s
    #139372
  • Merge request !7355Draft: Resolve #3437499 "With tests" → (Open) created by catch
  • Pipeline finished with Failed
    7 days ago
    Total: 651s
    #139383
  • Pipeline finished with Failed
    7 days ago
    Total: 607s
    #139400
  • 🇬🇧United Kingdom catch

    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.

  • Pipeline finished with Canceled
    7 days ago
    #139408
  • Pipeline finished with Failed
    7 days ago
    Total: 626s
    #139411
  • Pipeline finished with Success
    7 days ago
    Total: 657s
    #139422
  • 🇬🇧United Kingdom catch

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

  • 🇬🇧United Kingdom catch

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

  • 🇬🇧United Kingdom catch
  • Pipeline finished with Success
    6 days ago
    Total: 629s
    #140008
  • 🇬🇧United Kingdom catch

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

  • Merge request !7360Draft: Add an extra page to the test. → (Open) created by catch
  • Pipeline finished with Failed
    6 days ago
    Total: 633s
    #140052
  • Status changed to Needs work 6 days ago
  • 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.

  • Pipeline finished with Failed
    5 days ago
    #140278
  • Pipeline finished with Canceled
    5 days ago
    Total: 649s
    #140280
  • Pipeline finished with Canceled
    5 days ago
    Total: 636s
    #140286
  • Pipeline finished with Canceled
    5 days ago
    Total: 27s
    #140303
  • Pipeline finished with Failed
    5 days ago
    Total: 598s
    #140299
  • Pipeline finished with Canceled
    5 days ago
    #140304
  • Pipeline finished with Success
    5 days ago
    Total: 567s
    #140306
  • Pipeline finished with Canceled
    5 days ago
    Total: 514s
    #140308
  • 🇬🇧United Kingdom catch

    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.

  • Pipeline finished with Success
    5 days ago
    Total: 593s
    #140315
  • 🇬🇧United Kingdom catch

    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.

Production build https://api.contrib.social 0.62.1