Remove asset concatenation

RESOLVED FIXED

Status

()

bugzilla.mozilla.org
General
P1
normal
RESOLVED FIXED
9 months ago
a month ago

People

(Reporter: dylan, Assigned: dylan)

Tracking

Production
Dependency tree / graph

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

9 months ago
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
(Assignee)

Comment 1

9 months ago
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.
(Assignee)

Updated

9 months ago
Assignee: nobody → dylan
(Assignee)

Updated

9 months ago
Depends on: 1363803
(Assignee)

Comment 2

9 months ago
Created attachment 8866459 [details] [review]
PR
Attachment #8866459 - Flags: review?(rsoderberg)
(Assignee)

Comment 3

9 months ago
Created attachment 8866461 [details] [diff] [review]
75.patch

And a patch, for glob to review (it is the same as the pull request)
Attachment #8866461 - Flags: review?(glob)
(Assignee)

Comment 4

9 months ago
Created attachment 8866531 [details] [diff] [review]
75_2.patch
Attachment #8866461 - Attachment is obsolete: true
Attachment #8866461 - Flags: review?(glob)
Attachment #8866531 - Flags: review?(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.
Attachment #8866531 - Flags: review?(glob)

Updated

9 months ago
Attachment #8866459 - Flags: review?(glob)
(Assignee)

Updated

8 months ago
Attachment #8866531 - Attachment is obsolete: true

Updated

8 months ago
Attachment #8866459 - Flags: review?(glob) → review-
(Assignee)

Updated

8 months ago
Blocks: 1361439
(Assignee)

Updated

8 months ago
Attachment #8866459 - Flags: review?(rsoderberg)
(Assignee)

Comment 6

8 months ago
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+
(Assignee)

Updated

7 months ago
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
(Assignee)

Comment 8

7 months ago
Re-opening for test failures
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

7 months ago
Attachment #8866459 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Summary: Fix problems with current js and css concatenation → Remove asset concatenation
(Assignee)

Updated

a month ago
Priority: P2 → P1
(Assignee)

Updated

a month ago
Depends on: 1083695
(Assignee)

Updated

a month ago
Depends on: 1426365
(Assignee)

Comment 9

a month ago
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

a month ago
Created attachment 8938002 [details] [review]
github pull request
(Assignee)

Comment 11

a month ago
Comment on attachment 8938002 [details] [review]
github pull request

r=kohei, plus testing on dev.
Attachment #8938002 - Flags: review+
(Assignee)

Updated

a month ago
Status: REOPENED → RESOLVED
Last Resolved: 7 months agoa month ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.