1303180 - Use Brotli to compress MAR contents

Status: RESOLVED WONTFIX

(Toolkit :: Application Update, defect, P3)

Description

[Chris AtLee [:catlee]]

Attachment: Add brotli support to updater

We currently use bz2 to compress the contents of MAR files.

Brotli is a more efficient compression algorithm, and is already something supported by Firefox. Some size comparisons are below:

53665068 firefox-43.0.1.complete.mar
50572367 firefox-43.0.1.complete-ztd.mar
49134199 firefox-43.0.1.complete-bro.mar
48614533 firefox-43.0.1.complete-xz.mar

21358088 firefox-42.0-43.0.1.partial.mar
20665205 firefox-42.0-43.0.1.partial-zstd.mar
20562696 firefox-42.0-43.0.1.partial-bro.mar
20283661 firefox-42.0-43.0.1.partial-xz.mar

Using brotli saves approximately 8% (4.5MB) for this complete mar, and 3% (795k) for the partial mar. xz does a bit better, but would require introducing a new compression scheme into Firefox.

Comment 1

[Molly Howell (she/her) (no longer active)]

Big fan of practical ways to make updates smaller, so I'm certainly into this idea.

I haven't thoroughly been through the actual Brotli code yet, but my immediate feedback would be that I don't love getting this code into the business of sniffing magic numbers. Seems a bit fragile, plus if we ever want to add a second format that doesn't have one, like Brotli doesn't, then we're really in trouble. I think we should consider adding a compression type field to the MAR format instead. We'd have to bump the (sort of implicit) version number in the MAR header and add the new field there, or to the index entry if we want to allow different compression types in the same MAR.

Comment 2

[Chris AtLee [:catlee]]

Another option would be to introduce a new additional section type to indicate which compression is used

Comment 3

[Robert Strong (they/them - no direct email)]

In general why bother with support for multiple decompression formats which adds complexity to the client side?

Comment 4

[Molly Howell (she/her) (no longer active)]

(In reply to Chris AtLee [:catlee] from comment #2)
> Another option would be to introduce a new additional section type to
> indicate which compression is used
Yeah, I like this better. It does seem more sensible to extend the format by just using the extension mechanism that it already has.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #3)
> In general why bother with support for multiple decompression formats which
> adds complexity to the client side?
It's not that much complexity, and I'm worried about synchronizing landing the client patch with switching over to generating the new format, and about breaking updates for clients that are far behind trying to use newly-generated files.

Comment 5

[Robert Strong (they/them - no direct email)]

As long as the complexity is kept to a minimum though breaking updates shouldn't be an issue since the release would use a watershed just as we do for similar changes.

Comment 6

[Molly Howell (she/her) (no longer active)]

I think avoiding a watershed is worth the small amount of additional code/complexity we're talking about. I think all we need is a function in mar_read.c to parse the new additional section, a field in the MarFile struct to store that value, and a switch in the ArchiveReader to use the right extraction function. Not a lot of stuff.

Comment 7

[Molly Howell (she/her) (no longer active)]

Chris, is there a reason for preferring Brotli over LZMA other than Brotli already being in the tree? rstrong and I talked about it, and we don't think having to add the LZMA SDK would be much of an issue, so it seems better to go straight to the one that gets us the better numbers.

Comment 8

[Chris AtLee [:catlee]]

That was the main reason, yes. I can re-examine LZMA again. One possible benefit there is LZMA archives do have standard magic bytes to check for.

Comment 9

[Robert Strong (they/them - no direct email)]

Matt, it would be better to do this work in bug 641212 and to close this bug.

Comment 10

[Robert Strong (they/them - no direct email)]

wontfixing this bug in favor of bug 641212