Beginning last week, :atoll turned on HTTP 2 for bugzilla-dev, and started asking me a bunch of questions about where and when we could use caching headers. We covered lot of ground (and there are bugs already filed for the specifics of caching) but one thing that keeps coming up is that the way we handle CSS and JS files is problematic. Basically, there are two problems: 1) Filenames are not deterministic (they change, every push, even if none of the css or js files change) 2) They're stored in NFS, because they're generated at runtime For testing HTTP 2 on dev, we disabled asset concatenation and this itself was promising. Following that, I put together an experimental branch: https://github.com/dylanwh/bmo/tree/new-assets In this case, all js, css, image and even font files get a content-addressable filename. This means we can use a cache-control max age of forever (and, even Cache-Control: immutable) However, this still lacks asset concatenation. Fortunately, I have a plan for that: Pre-concatenate combinations of files that I know ahead of time will be needed at the same time. For any combination (order-dependent) that is not listed, we fall back to individual files. Some of the benefits from this are: 1) Deployment to cloud-services becomes easier (one less thing to use the shared NFS filesystem!) 2) Predictable builds -- the asset files can be known ahead of time 3) Better caching
After much deliberation, I have decided to push for removing the current asset concatenation code, and instead focusing on having very cacheable filenames. I believe this when combined with HTTP/2 will offer the best possible experience to the largest number of users. Because of the caching headers, even the experience for non-http/2 clients will be okay. I have left a placeholder in the design to allow asset concatenation to be added, but doing this is fundamentally difficult in a way that doesn't reproduce the current problems.
Created attachment 8866461 [details] [diff] [review] 75.patch And a patch, for glob to review (it is the same as the pull request)
Created attachment 8866531 [details] [diff] [review] 75_2.patch
Comment on attachment 8866531 [details] [diff] [review] 75_2.patch Review of attachment 8866531 [details] [diff] [review]: ----------------------------------------------------------------- this appears to be two diffs concatenated together. as there's another review happening on gh, i'll conduct my review there.
Attachment #8866531 - Attachment is obsolete: true
Comment on attachment 8866459 [details] [review] PR Revised per last review
Attachment #8866459 - Flags: review- → review?(glob)
Comment on attachment 8866459 [details] [review] PR I've done two passes of review on Github and I believe Glob did one pass as well, so this is r+ from me.
Attachment #8866459 - Flags: review?(glob) → review+
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Re-opening for test failures
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8866459 - Attachment is obsolete: true
Summary: Fix problems with current js and css concatenation → Remove asset concatenation
Implicit in the decision to do this is that we're going to regress on performance. That's an acceptable risk to rid ourselves of this code.
Comment on attachment 8938002 [details] [review] github pull request r=kohei, plus testing on dev.
Attachment #8938002 - Flags: review+
Status: REOPENED → RESOLVED
Last Resolved: 7 months ago → a month ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.