Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add dist.compression-profile option to control compression speed #109124

Merged
merged 3 commits into from
Mar 21, 2023

Conversation

pietroalbini
Copy link
Member

PR #108534 reduced the size of compressed archives, but (as expected) it also resulted in way longer compression times and memory usage during compression.

It's desirable to keep status quo (smaller archives but more CI usage), but it should also be configurable so that downstream users don't have to waste that much time on CI. As a data point, this resulted in doubling the time of Ferrocene's dist jobs, and required us to increase the RAM allocation for one of such jobs.

This PR adds a new config.toml setting, dist.compression-profile. The values can be:

  • fast: equivalent to the gzip and xz preset of "1"
  • balanced: equivalent to the gzip and xz preset of "6" (the CLI defaults as far as I'm aware)
  • best: equivalent to the gzip present of "9", and our custom xz profile

The default has also been moved back to balanced, to try and avoid the compression time regression for downstream users. I don't feel too strongly on the default, and I'm open to changing it.

Also, for the best profile the XZ settings do not match the "9" preset used by the CLI, and it might be confusing. Should we create a custom-rustc-ci/ultra profile for that?

r? @Mark-Simulacrum

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Mar 14, 2023
@pietroalbini
Copy link
Member Author

I recommend hiding the whitespace while reviewing this change, I had to change indentation of a bunch of code :(

@Mark-Simulacrum
Copy link
Member

Do we think it makes sense to continue maintaining these options here and a semi-duplicate in rust-lang/promote-release for the recompression support?

Mostly raising the question -- I think I'm not sure yet myself. Will review in detail soon.

@pietroalbini
Copy link
Member Author

Do we think it makes sense to continue maintaining these options here and a semi-duplicate in rust-lang/promote-release for the recompression support?

I think the difference between the support here and in promote-release is that promote-release is specific to our (Rust project) builds and releases, while this knob would be relied upon by downstream users, for whom it doesn't make much sense to use promote-release.

@Mark-Simulacrum
Copy link
Member

Right, I mean that we could (for example) only produce .tar files in rust-lang/rust's bootstrap code, or perhaps only the fast profile out of those here. Then promote-release or others can recompress if needed. It seems to me that if you're running x.py dist purely locally, then probably no compression or super-fast compression is what you want; if you're distributing you probably want balanced or best (depending on number of users).

Long-term I suspect that maybe zstd is the right thing for rust-lang/rust to produce, presumably that can give us fast, relatively well-compressed artifacts, and maybe works for ~everyone. Then from there recompression is maybe easy enough.

We could also consider a compression-pipe-through=["program", "args"] or something as another option.

config.example.toml Outdated Show resolved Hide resolved
@Mark-Simulacrum
Copy link
Member

r=me (maybe with @bjorn3's comment resolved) but I think we should continue thinking about the right shape here.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 18, 2023
@pietroalbini
Copy link
Member Author

@bors r=Mark-Simulacrum rollup

@bors
Copy link
Contributor

bors commented Mar 21, 2023

📌 Commit 0177176 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 21, 2023
Noratrieb added a commit to Noratrieb/rust that referenced this pull request Mar 21, 2023
…ark-Simulacrum

Add `dist.compression-profile` option to control compression speed

PR rust-lang#108534 reduced the size of compressed archives, but (as expected) it also resulted in way longer compression times and memory usage during compression.

It's desirable to keep status quo (smaller archives but more CI usage), but it should also be configurable so that downstream users don't have to waste that much time on CI. As a data point, this resulted in doubling the time of Ferrocene's dist jobs, and required us to increase the RAM allocation for one of such jobs.

This PR adds a new `config.toml` setting, `dist.compression-profile`. The values can be:

* `fast`: equivalent to the gzip and xz preset of "1"
* `balanced`: equivalent to the gzip and xz preset of "6" (the CLI defaults as far as I'm aware)
* `best`: equivalent to the gzip present of "9", and our custom xz profile

The default has also been moved back to `balanced`, to try and avoid the compression time regression for downstream users. I don't feel too strongly on the default, and I'm open to changing it.

Also, for the `best` profile the XZ settings do not match the "9" preset used by the CLI, and it might be confusing. Should we create a `custom-rustc-ci`/`ultra` profile for that?

r? `@Mark-Simulacrum`
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 21, 2023
Rollup of 10 pull requests

Successful merges:

 - rust-lang#106434 (Document `Iterator::sum/product` for Option/Result)
 - rust-lang#108326 (Implement read_buf for a few more types)
 - rust-lang#108842 (Enforce non-lifetime-binders in supertrait preds are not object safe)
 - rust-lang#108896 (new solver: make all goal evaluation able to be automatically rerun )
 - rust-lang#109124 (Add `dist.compression-profile` option to control compression speed)
 - rust-lang#109240 (Walk un-shifted nested `impl Trait` in trait when setting up default trait method assumptions)
 - rust-lang#109385 (fix typo)
 - rust-lang#109386 (add myself to mailmap)
 - rust-lang#109390 (Custom MIR: Support aggregate expressions)
 - rust-lang#109408 (not *all* retags might be explicit in Runtime MIR)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 09b1254 into rust-lang:master Mar 21, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 21, 2023
@pietroalbini pietroalbini deleted the pa-compression-mode branch March 21, 2023 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants