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

handle --release flag separately for tools #136048

Closed

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented Jan 25, 2025

Right now, we are using the --release flag on everything based on the rust.optimize option, which is true by default (see the reasons). But the reasons for this are only relevant to the compiler and the standard library, so it doesn't make sense to use --release for every build.

This PR adds --release flag (similar to cargo) and separates --release handling between compiler/std and tool builds. This saves a lot of time when building or testing compiler tools (e.g., x test bootstrap without release build saves ~10 seconds on Ryzen 9 9950X).

@onur-ozkan onur-ozkan changed the title separate --release flag separately for tools handle --release flag separately for tools Jan 25, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 25, 2025

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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) labels Jan 25, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 25, 2025

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

Signed-off-by: onur-ozkan <[email protected]>
@jieyouxu jieyouxu self-assigned this Jan 25, 2025
Copy link
Contributor

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, we definitely don't need every tool to be optimized, especially the bootstrap tools. Although there might be some tools for which it can be helpful, e.g. compiletest - have you benchmarked how long does it take to run e.g. x test tests/ui with a non-optimized compiletest?

Regarding saving compilation time, removing debuginfo could also help a lot, since it will be now generated by default when release mode is no longer used. This can also increase the size of the build directory quite a lot.

I'm a bit confused by the implementation though. Why didn't you use a separate config option for this, to let people configure it in config.toml (e.g. optimize-tools)? The current behavior seems confusing to me - setting rust.optimize = true only optimizes the compiler and stdlib, using --release optimizes everything. If people want optimized tools, they would need to keep passing --release to every x invocation, which is annoying. Automatically enabling the flag on CI during config parsing is also quite opaque.

We could add rust.optimize-tools, set it to false by default, to make local builds faster, and then set it to true in the dist profile, to restore the previous behavior on CI.

@Mark-Simulacrum Mark-Simulacrum removed their assignment Jan 25, 2025
@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Jan 25, 2025

I'll unassign myself, but in general I'd love to see some numbers on relative gain here. For tidy, at least, this seems to be a net loss -- local measurements with ~1 iteration suggest this is a ~7 second win on compilation time but a 18.5 second regression on pre-compiled, clean work trees and a 12 second regression on from-scratch builds end-to-end. Tidy might be special - some other tools likely would benefit more - but it's not obvious to me that we have a ton of tools that are both built often and fall in a bucket of "not worth optimizing". And if they're built rarely, then maybe extra configuration and a CLI flag is just not worth it for them.

We could also customize what release means via Cargo.toml workspace overrides on a per-package basis. That could give a better balancing point.

Some numbers for my local setup on time ./x.py test src/tools/tidy:

Release (current):

  • post-x clean: 0m51.564s; user 2m10.373s; sys 0m24.978s
    • tidy compilation - 15 seconds
  • pre-compiled: real 0m4.170s; user 0m22.613s; sys 0m7.449s

W/o --release:

  • post-x clean: real 1m2.816s; user 2m53.379s; sys 0m27.389s
    • tidy compilation - 7 seconds
  • pre-compiled: real 0m22.771s; user 1m25.598s; sys 0m7.582s

@jieyouxu
Copy link
Member

jieyouxu commented Jan 25, 2025

Since you already did some investigation, r? @Mark-Simulacrum
(I only self-assigned because I wanted to remind myself to look at it)

@rustbot rustbot assigned Mark-Simulacrum and unassigned jieyouxu Jan 25, 2025
@onur-ozkan
Copy link
Member Author

I'm a bit confused by the implementation though. Why didn't you use a separate config option for this, to let people configure it in config.toml (e.g. optimize-tools)? The current behavior seems confusing to me - setting rust.optimize = true only optimizes the compiler and stdlib, using --release optimizes everything.

I don't think there will be any difference when we propagate another config option rather than propagating a command argument. We still need to use rust.optimize for compiler and std trees.

If people want optimized tools, they would need to keep passing --release to every x invocation, which is annoying. Automatically enabling the flag on CI during config parsing is also quite opaque.

That way they would need to change the option in config.toml if they want to disable the --release flag for tools. This sounds worse than simply passing the --release flag, as --release flag is more familiar to people as that's how cargo typically works.

We could also customize what release means via Cargo.toml workspace overrides on a per-package basis. That could give a better balancing point.

That wouldn't be good for tools like miri I suppose.

I will do more benchmarking (I only tested x build rustdoc and x test bootstrap, on first invocation I saved ~3 seconds and ~10 seconds on the second invocation). I will share more results here later.

@onur-ozkan
Copy link
Member Author

@rustbot author

@rustbot rustbot 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 Jan 25, 2025
@Kobzol
Copy link
Contributor

Kobzol commented Jan 25, 2025

That way they would need to change the option in config.toml if they want to disable the --release flag for tools.

The default would be unoptimized tools, so they wouldn't have to change config.toml to get that behavior. Although I'm not sure if that default will work for people, optimized tools have been the default so far, and I suspect that some of them will have to stay optimized, or performance will be bad). In that case, users would need to pass --release to every x command, and that sounds super annoying.

I guess that I don't see why "optimizing tools" is such a special use-case that it would have to be overridden through the CLI, and it wouldn't be possible to be configured through config.toml. Most other CLI flags can be configured through config.toml.

@onur-ozkan
Copy link
Member Author

Here are some time reports from various bootstrap commands (each called on fresh state after x clean) invoked on Ryzen 9 9950X:

Command User Time System Time Total Time
x test bootstrap --release 119.40s 9.08s 39.481s
x test bootstrap 61.05s 9.35s 34.293s
x build cargo --stage 0 --release 1362.82s 53.72s 2:09.47
x build cargo --stage 0 1097.93s 51.99s 1:43.07
x build miri --release 2541.55s 70.71s 3:18.64
x build miri 2524.75s 72.26s 3:10.69
x build rustfmt --release 2574.37s 66.94s 3:06.34
x build rustfmt 2484.26s 66.56s 3:03.39
x test tidy --bless --release 82.11s 8.32s 20.994s
x test tidy --bless 82.37s 8.05s 26.836s

Apparently it doesn't seem worth (like bootstrap) to do this on most cases. I assume building debug cargo and using it to build other heavy tools would make it even worse. tidy is also a significant loss here. I am going to close this PR and open a new one to override the bootstrap build options in Cargo.toml to fix the long-compilation issue on x test bootstrap.

@onur-ozkan onur-ozkan closed this Jan 27, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 30, 2025
…e, r=Kobzol

override build profile for bootstrap tests

Using the release profile for bootstrap self tests puts too much load on the CPU and makes it quite hot on `x test bootstrap` invocation for no good reason. It also makes the compilation take longer than usual (see rust-lang#136048 (comment)). This change turns off the release flag for bootstrap self tests.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 31, 2025
Rollup merge of rust-lang#136157 - onur-ozkan:override-release-profile, r=Kobzol

override build profile for bootstrap tests

Using the release profile for bootstrap self tests puts too much load on the CPU and makes it quite hot on `x test bootstrap` invocation for no good reason. It also makes the compilation take longer than usual (see rust-lang#136048 (comment)). This change turns off the release flag for bootstrap self tests.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jan 31, 2025
override build profile for bootstrap tests

Using the release profile for bootstrap self tests puts too much load on the CPU and makes it quite hot on `x test bootstrap` invocation for no good reason. It also makes the compilation take longer than usual (see rust-lang/rust#136048 (comment)). This change turns off the release flag for bootstrap self tests.
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-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

5 participants