Problem/Motivation

Found in #3436527: Record total css and js file size in performance tests

Steps to reproduce

Install Umami. Visit the front page, then visit the 'articles' page.

The aggregate filename is the same, but the 'include' query parameter is different this is because one CSS-only library isn't attached on the articles page, so doesn't affect the file contents.

This is 'by design' in that the files should be identical when the libraries passed result in the same contents, but I wonder if we can improve edge caching but making the query parameter the same too.

This should have several benefits:
1. Browser and edge caches will get higher hit rates.
2. Slight reduction in Drupal page weight, for any HTML response.
3. Slight reduction in the request size from clients when they fetch the aggregates, since it's already small, this might be a higher percentage even if it's only a few bytes. Also often upstream bandwidth is a lot more constrained than downstream bandwidth.

Proposed resolution

When building JavaScript aggregate URLs, don't bother sending libraries with only CSS in them.

For CSS aggregate URLs, don't bother sending libraries with only JS in them.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#19 3437839-nr-bot.txt90 bytesneeds-review-queue-bot
#12 3437839-nr-bot.txt90 bytesneeds-review-queue-bot
#10 3437839-nr-bot.txt90 bytesneeds-review-queue-bot

Issue fork drupal-3437839

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review

Pushed a commit, this is only a small change to AssetResolver, most of it is indentation changes because we can remove a condition in a later hunk.

Will see whether this requires any test changes.

Also realised we can do this in the opposite direction for CSS aggregates - remove the non-CSS libraries when building those URLs too,

Apart from the potential improvements to browser and edge caches, this also makes the URLs significantly shorter.

Before:
https://drupal-dev.ddev.site/sites/default/files/js/js_Xc0CqWop9HPDPJJr9o1ZfdcVQmp-Q9tMliiLOllT6-4.js?scope=footer&delta=0&language=en&theme=umami&include=eJx1j0sOwjAMRC9E2jM5jikW_lR2CiqnpyrqgkU2Xvg9jWZyz046V0i6bQrKMwpk7tP5QQ-azUNB-HMJi3gF-TF0Xd3Iek4nLKVxHgmsFCOjcxcawbt7pyhVHJ-3F9M75_NO6m2Tq0IQ8koFXYSws1uO8oKgFT3YSECI9r_cvA3tCmbjZTXAGtsy4o-jDMUXI82NiA

After:

https://drupal-dev.ddev.site/sites/default/files/js/js_Xc0CqWop9HPDPJJr9o1ZfdcVQmp-Q9tMliiLOllT6-4.js?scope=footer&delta=0&language=en&theme=umami&include=eJxLzi9K1U_Ozy3Iz0vNKynWK81NzM3U1c1ITUxJLQIAqggLKQ

It should also reduce the potential for differently ordered libraries to change the asset order.

catch opened merge request !7308

catch’s picture

catch
he/him
Primary language English
commented
Title: Don't send non-js libraries with aggregate URLs » Only send libraries with aggregate URLs that have the aggregate type included
Issue summary: View changes
catch’s picture

catch
he/him
Primary language English
commented

We have implicit test coverage of this code in that the code path is tested, I think we can rely on #3436527: Record total css and js file size in performance tests to cover the performance/caching optimization, the newly added test in that issue is how I found this in the first place.

luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented

Ran against a page on a site with ~230 CSS-only component libs and ~5 JS-only libs, and ~10 CSS+JS libs.

Single page

Without MR Applied:

    DOM: 65kB
    CSS: 25.4kB
    JS: 96.9kB

With MR Applied:

    DOM: 64.4kB
    CSS: 25.4kB transferred
    JS: 97.1kB transferred

Delta:

    DOM -.6kB
    CSS 0kB
    JS +.2kB

Net: -.4kB

No clue why / how an extra 200 bytes of JS was delivered. That shouldn't have been possible. In theory, less variance means:

  1. Quicker loads overall
  2. Fewer Drupal bootstraps (important for managed hosting customers who pay per view).

This improvement sounds good to me all around.

Note - I didn't review the code; just performed a basic sanity test against a highly customized 10.1.x site.

catch’s picture

catch
he/him
Primary language English
commented

Fewer Drupal bootstraps (important for managed hosting customers who pay per view).

It won't actually mean fewer Drupal bootstraps, because this doesn't change the number of files on disk, I'm working on that in #3437499: Use placeholdering for more blocks, which is how I indirectly found this. What it should (hopefully) do is instead of a many-to-one relationship of URLs to files, make it 1-1. Because of that, it will reduce the number of requests that fall through from the browser cache, CDN, or varnish, to the webserver itself.

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: +gander, +front end performance

https://git.drupalcode.org/project/drupal/-/merge_requests/7313 now has test coverage for this, showing that it saves one very large file download when browsing two different Umami pages as an authenticated user.

Before:

  $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:

    $this->assertSame(4, $performance_data->getStylesheetCount());
    $this->assertSame(94355, $performance_data->getStylesheetBytes());
    $this->assertSame(1, $performance_data->getScriptCount());
    $this->assertSame(132038, $performance_data->getScriptBytes());
catch’s picture

catch
he/him
Primary language English
commented

Rebased now that #3436527: Record total css and js file size in performance tests is in.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3437839-nr-bot.txt90 bytes

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.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Rebased.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3437839-nr-bot.txt90 bytes

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.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Rebased again.

thejimbirch’s picture

thejimbirch commented
Status: Needs review » Reviewed & tested by the community

Following the validation steps, I can confirm the query parameter on the JavaScript file remains the same after the merge request had been applied.

## With MR

### Home

<script src="/sites/default/files/js/js_AmsTgkZtlnQSPjPq3dYt_0F5jMg4cWrd8Dzn1C0fPEs.js?scope=footer&amp;delta=0&amp;language=en&amp;theme=umami&amp;include=eJxLzi9K1U_Ozy3Iz0vNKynWK81NzM3U1c1ITUxJLQIAqggLKQ"></script>

### Articles

<script src="/sites/default/files/js/js_AmsTgkZtlnQSPjPq3dYt_0F5jMg4cWrd8Dzn1C0fPEs.js?scope=footer&amp;delta=0&amp;language=en&amp;theme=umami&amp;include=eJxLzi9K1U_Ozy3Iz0vNKynWK81NzM3U1c1ITUxJLQIAqggLKQ"></script>


## Without MR/11.x Branch

### Home

<script src="/sites/default/files/js/js_AmsTgkZtlnQSPjPq3dYt_0F5jMg4cWrd8Dzn1C0fPEs.js?scope=footer&amp;delta=0&amp;language=en&amp;theme=umami&amp;include=eJxLzi9K1U_Ozy3Iz0vNKynWK81NzM3U1c1ITUxJLQIAqggLKQ"></script>

### Articles

<script src="/sites/default/files/js/js_kYim7ymiBmzlTrrCnusXensmAsaPewPRBnRIVZr8DE8.js?scope=footer&amp;delta=0&amp;language=en&amp;theme=umami&amp;include=eJx1j0EOwzAIBD8Ux2_CmKaoYCJwWqWvb5Qqhxx84cCMFjb26KS5QNC0KShnFIjY53OD5pSbuYLw9xIWsQLyZ2i6WqPWYz5hSpXjSGAlHxmdu9AIPsw6eSpi-JreTJ_I55zV6ibXC07IKyU0EcLO1mKU5wQ16cFGAoLXe_NmdWgXh1a5LSP-PM6R_wAzGIIn"></script>

Marking as RTBC for the validation steps. If it needs a code review, please switch it back to Needs review. Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 10.3.x-dev
Category: Task » Bug report
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 22707d5cd0 and pushed to 11.x. Thanks!

We need a 10.3.x MR...

  • alexpott committed 22707d5c on 11.x 
    Issue #3437839 by catch, Luke.Leber, thejimbirch: Only send libraries...

catch opened merge request !7671

catch’s picture

catch
he/him
Primary language English
commented
Status: Patch (to be ported) » Needs review

Opened a 10.3 MR. We can backport this to 10.2.x too I think.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3437839-nr-bot.txt90 bytes

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.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Rebased. Green apart from a known failure in 10.3.x

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

The backport only needed to update performance tests, so re-RTBCing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 4dd78b9 and pushed to 10.3.x. Thanks!

  • alexpott committed 4dd78b90 on 10.3.x 
    Issue #3437839 by catch, thejimbirch, Luke.Leber: Only send libraries...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.