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

Import rust-installer & adjust compression settings #108534

Merged
merged 4 commits into from
Mar 8, 2023

Conversation

Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Feb 27, 2023

This brings in rust-lang/rust-installer#123, which enables a larger compression window (8 -> 64MB) amongst other changes to the xz compression settings. The net effect should be smaller compressed tarballs which will decrease bandwidth usage for
static.rust-lang.org, download times, and decompression time.

This comes at the cost of higher baseline requirements for running rustup to use these files, which we believe should be largely acceptable (running rustc is likely to use at least this much memory) but if we get specific reports we may explore options to decrease impact (e.g., using the gzip tarballs automatically in rustup).

To simplify iteration on compression settings this also imports the rust-lang/rust-installer submodule, it is now hosted fully inside rust-lang/rust. Once we land this I'll file a followup to add a note to that repo and we can subsequently archive it.

--

CI times for dist-x86_64-linux builds:

  • threads=6, master - 2h 50m
  • threads=1, new - 3h 40m
  • threads=6, new - 2h 50m

@rustbot
Copy link
Collaborator

rustbot commented Feb 27, 2023

⚠️ Warning ⚠️

  • These commits modify submodules.

@rustbot rustbot added 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) labels Feb 27, 2023
@pietroalbini
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 27, 2023

📌 Commit 48af38f has been approved by pietroalbini

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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 27, 2023
@Mark-Simulacrum
Copy link
Member Author

@bors r-

I want to check locally with dev-static

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 27, 2023
@Mark-Simulacrum
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Feb 27, 2023

⌛ Trying commit 48af38f with merge 9060c7f5f9eae7cc9697bef19898ba1e38b2ee56...

@bors
Copy link
Contributor

bors commented Feb 27, 2023

☀️ Try build successful - checks-actions
Build commit: 9060c7f5f9eae7cc9697bef19898ba1e38b2ee56 (9060c7f5f9eae7cc9697bef19898ba1e38b2ee56)

@Mark-Simulacrum
Copy link
Member Author

Baseline install of nightly-2023-02-27 takes 53 MB with RUSTUP_IO_THREADS=1 RUSTUP_UNPACK_RAM=33554432 rustup update nightly-2023-02-27. Installing the nightly from the try build with the same environment settings takes 102 MB. So we've increased max rss for a nightly install by around 50 MB. Overall I think this fits within the parameters we were aiming for in infra discussion.

Wall-time for installation seems to take roughly the same amount of time with the minimum settings (6.9s before, 6.4s now). When not constraining I/O threads or memory, 7.2s before, 6.5s now.

Ultimately this all seems fairly reasonable so going to go ahead and move ahead, if we run into trouble we can back it out.

@bors r=pietroalbini

@bors
Copy link
Contributor

bors commented Feb 28, 2023

📌 Commit 48af38f has been approved by pietroalbini

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 Feb 28, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 28, 2023
…troalbini

Bump rust-installer

This brings in rust-lang/rust-installer#123, which enables a larger compression window (8 -> 64MB) amongst other changes to the xz compression settings. The net effect should be smaller compressed tarballs which will decrease bandwidth usage for
static.rust-lang.org, download times, and decompression time.

This comes at the cost of higher baseline requirements for running rustup to use these files, which we believe should be largely acceptable (running rustc is likely to use at least this much memory) but if we get specific reports we may explore options to decrease impact (e.g., using the gzip tarballs automatically in rustup).
@matthiaskrgr
Copy link
Member

matthiaskrgr commented Mar 1, 2023

@bors rollup=never
it looks like this blows up ci times

master:
[TIMING] dist::PlainSourceTarball PlainSourceTarball -- 123.972

my rollup which included this pr:
[TIMING] dist::PlainSourceTarball PlainSourceTarball -- 1442.916

this is like more then factor 10 for just this tarball? Just compression takes almost half an hours now (for that one artifact!)
https://github.com/rust-lang-ci/rust/actions/runs/4296947503/jobs/7489314532#step:26:50875

@Mark-Simulacrum
Copy link
Member Author

Hm, probably due to dropping the thread count. I'll update to avoid that.

@Mark-Simulacrum
Copy link
Member Author

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 1, 2023
@Mark-Simulacrum Mark-Simulacrum changed the title Bump rust-installer Import rust-installer & adjust compression settings Mar 1, 2023
@Mark-Simulacrum
Copy link
Member Author

@bors try

@Mark-Simulacrum Mark-Simulacrum force-pushed the compression branch 2 times, most recently from d4e6306 to 7040337 Compare March 6, 2023 23:09
@Mark-Simulacrum
Copy link
Member Author

@bors r=pietroalbini rollup=never

@bors
Copy link
Contributor

bors commented Mar 6, 2023

📌 Commit 70403375ae66f7e480c0ba616df5f1773895bb02 has been approved by pietroalbini

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 7, 2023

⌛ Testing commit 70403375ae66f7e480c0ba616df5f1773895bb02 with merge e4666f4fe99c67dea739cac0d674f59ce0fe75ca...

@bors
Copy link
Contributor

bors commented Mar 7, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 7, 2023
This brings in rust-lang/rust-installer#123, which enables a larger
compression window (8 -> 64MB) amongst other changes to the xz
compression settings. The net effect should be smaller compressed
tarballs which will decrease bandwidth usage for
static.rust-lang.org, download times, and decompression time.

This comes at the cost of higher baseline requirements for running
rustup to use these files, which we believe should be largely acceptable
(running rustc is likely to use at least this much memory) but if we get
specific reports we may explore options to decrease impact (e.g., using
the gzip tarballs automatically in rustup).
This moves the rust-installer code to be directly hosted in
rust-lang/rust, since it's not used elsewhere and this makes it easier
to make and review changes without needing a separate upstream commit.
Limit to 3 threads for 32-bit platforms, otherwise we use 6 threads.
This avoids exceeding the amount of memory we can allocate on a 32-bit
platform.
@Mark-Simulacrum
Copy link
Member Author

@bors r=pietroalbini rollup=never

@bors
Copy link
Contributor

bors commented Mar 7, 2023

📌 Commit dabafb4 has been approved by pietroalbini

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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 7, 2023
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 8, 2023

⌛ Testing commit dabafb4 with merge 60445fd...

@bors
Copy link
Contributor

bors commented Mar 8, 2023

☀️ Test successful - checks-actions
Approved by: pietroalbini
Pushing 60445fd to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 8, 2023
@bors bors merged commit 60445fd into rust-lang:master Mar 8, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 8, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (60445fd): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.3% [1.3%, 1.3%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.2% [-3.2%, -3.2%] 1
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

@Mark-Simulacrum Mark-Simulacrum deleted the compression branch March 10, 2023 22:38
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`
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``
@klensy
Copy link
Contributor

klensy commented Jun 21, 2023

Feels wrong that this thing not broken

RustInstaller, "src/tools/rust-installer", "rust-installer", is_external_tool = true;

"src/tools/rust-installer",

This comment strike back: while working on #112884 i found out that this flag set SourceType::Submodule for rust-installer and this is cause why rustfmt (probably?) didn't format it's sources. It seems there no working assert that submodule is actually submodule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants