Only send libraries with aggregate URLs that have the aggregate type included

Created on 3 April 2024, 7 months ago
Updated 9 May 2024, 6 months ago

Problem/Motivation

Found in πŸ“Œ Record total css and js file size in performance tests Fixed

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

πŸ› Bug report
Status

Fixed

Version

10.3 ✨

Component
Asset libraryΒ  β†’

Last updated about 7 hours ago

No maintainer
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

    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.

  • Merge request !7308Don't send non-js libraries for js aggregates. β†’ (Open) created by catch
  • Status changed to Needs review 7 months ago
  • Pipeline finished with Success
    7 months ago
    Total: 2487s
    #136312
  • πŸ‡¬πŸ‡§United Kingdom catch
  • Pipeline finished with Failed
    7 months ago
    #136344
  • Pipeline finished with Success
    7 months ago
    Total: 682s
    #136512
  • πŸ‡¬πŸ‡§United Kingdom catch

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

  • πŸ‡ΊπŸ‡ΈUnited States luke.leber Pennsylvania

    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.

  • πŸ‡¬πŸ‡§United Kingdom catch

    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 πŸ“Œ Use placeholdering for more elements to optimize asset serving Needs work , 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.

  • πŸ‡¬πŸ‡§United Kingdom catch

    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());
    
  • πŸ‡¬πŸ‡§United Kingdom catch

    Rebased now that πŸ“Œ Record total css and js file size in performance tests Fixed is in.

  • Pipeline finished with Canceled
    7 months ago
    Total: 2194s
    #140010
  • Pipeline finished with Success
    7 months ago
    Total: 635s
    #140029
  • Status changed to Needs work 7 months ago
  • 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.

  • Status changed to Needs review 7 months ago
  • πŸ‡¬πŸ‡§United Kingdom catch

    Rebased.

  • Status changed to Needs work 7 months ago
  • 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.

  • Status changed to Needs review 7 months ago
  • πŸ‡¬πŸ‡§United Kingdom catch

    Rebased again.

  • Pipeline finished with Success
    7 months ago
    Total: 1081s
    #142996
  • Pipeline finished with Success
    7 months ago
    Total: 1077s
    #151200
  • Status changed to RTBC 7 months ago
  • πŸ‡ΊπŸ‡ΈUnited States thejimbirch Cape Cod, Massachusetts

    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!

  • Pipeline finished with Success
    7 months ago
    Total: 986s
    #154152
  • Status changed to Downport 7 months ago
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    Committed 281e69a 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...
  • Status changed to Needs review 7 months ago
  • πŸ‡¬πŸ‡§United Kingdom catch

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

  • Pipeline finished with Failed
    7 months ago
    Total: 987s
    #154413
  • Status changed to Needs work 7 months ago
  • 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.

  • Pipeline finished with Failed
    7 months ago
    Total: 990s
    #155431
  • Status changed to Needs review 7 months ago
  • πŸ‡¬πŸ‡§United Kingdom catch

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

  • Status changed to RTBC 7 months ago
  • πŸ‡¬πŸ‡§United Kingdom catch

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

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    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...
  • Status changed to Fixed 7 months ago
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ
  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024