1361890 - Remove asset concatenation

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, enhancement, P1)

Description

[Dylan Hardison [:dylan] (he/him)]

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

Comment 1

[Dylan Hardison [:dylan] (he/him)]

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.

Comment 2

[Dylan Hardison [:dylan] (he/him)]

Attachment: PR

Comment 3

[Dylan Hardison [:dylan] (he/him)]

Attachment: 75.patch

And a patch, for glob to review (it is the same as the pull request)

Comment 4

[Dylan Hardison [:dylan] (he/him)]

Attachment: 75_2.patch

Comment 5

[:glob ✱]

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.

Comment 6

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8866459 [details] [review]
PR

Revised per last review

Comment 7

[:Atoll]

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.

Comment 8

[Dylan Hardison [:dylan] (he/him)]

Re-opening for test failures

Comment 9

[Dylan Hardison [:dylan] (he/him)]

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 10

[Bug Butler]

Attachment: github pull request

Comment 11

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8938002 [details] [review]
github pull request

r=kohei, plus testing on dev.