Closed Mark-Simulacrum closed 1 year ago
@Mark-Simulacrum: no appropriate reviewer found, use r? to override
The main tradeoff here is the dictionary size; currently our preset configures it to 8 MB and this raises it to 128 MB. Generally the effect is that we get better compression, but decompression (i.e., rustup) will also require more memory (8 -> 128 MB, roughly, though obviously there are other users of memory there).
I think given how much memory running rustc on any reasonable sized crate will take, this seems fine to me. IMO if users are worried about memory usage on this order of magnitude rustup etc are probably not great tools as-is for them regardless, and we should work to enable something much lower impact (e.g., non-compressed downloads or something).
Discussed in t-infra meeting, decided to raise to 64 MB rather than 128. That should help avoid some of the impact for low-memory virtual machines (e.g., for free or cheap cloud hosting) while still giving us a majority of the benefits for most files, with only extended tarballs benefiting significantly from the larger compression window.
Has this been tested end-to-end? We're usually decompression bound, not network or disk IO, in rustup these days. Tuning for faster decompression is one of the tuits sitting around for rustup, and for instance, moving to async + tokio will drive up memory pressure rather than alleviate it.
I'm obviously glad that we're worrying about user experience, but 15% decrease on network is not going to be a 15% decrease in 'time to install'.
(Followed up in zulip)
This adjusts our compression settings for xz compressed files, which should reduce download sizes by around 5-15%, at the cost of ~44% longer compression times. Given that compression happens almost universally in CI, rather than something humans are directly waiting for, this tradeoff feels worth it.
If we end up seeing this be too significant an increase in CI times, moving the more aggressive compression solely to promote-release should be relatively straightforward and give most of the benefits.
I expect we'll need to iterate a few times on the exact parameters as we try to run this through rust-lang/rust CI, but unfortunately I expect it'll need the full CI so I think merging this makes sense after review rather than trying to iterate through try builds.