-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Add parallel frontend to the build performance guide #15970
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 parallel frontend to the build performance guide #15970
Conversation
```console | ||
$ RUSTFLAGS="-Zthreads=8" cargo build | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should our default recommendation be the CLI or build.rustflags
? I lean towards the latter:
- The docs call out the other mechanisms
- Starts the user off with a more permanent place to put this so they don't have to remember on every invocation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Granted, this means we need to update the config section to be more nuanced since this wouldn't be set in Cargo.toml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
build.rustflags
was added in addition to the command line. Do we need both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should show both, as using RUSTLAGS
is useful for one-off tests. Not sure if we need to repeat both ways to configure rustflags for all workarounds that use it though. Once/if we have a second mechanism that uses RUSTFLAGS, I'd perhaps move this to the beginning of the section and then just refer to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If our goal is "show how to speed your build", having our recommendation be something that is not required on every command seems better to me. If we link out to the docs, it will list out how else it can be set (maybe we could even call out that there are other ways). Not a fan of the redundancy though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured that for manually benchmarking which one is faster (e.g. using back-to-back CLI invocations or e.g. hyperfine
), setting it through the environment variable is easier. For long term usage the config is ofc better.
Also regarding the config, there are trade-offs between setting build.rustflags
and target.rustflags
😆 Stuff is complicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Framing this as "what steps you need to benchmark" vs "what steps to apply to get the benefit" are very different. If we were going for the former, the debug info section should instead use the environment variable for it.
As for which *.rustflags
, this is platform agnostic so it seems like that would be build.rustflags
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which rustflags flag is important regarding their priority, and whether they are used for build scripts, but for the parallel frontend that probably won't matter, so let's keep it simple and use build.rustflags
, yeah.
|
||
Trade-offs: | ||
- ✅ Faster build times | ||
- ❌ **Requires using nightly Rust and an unstable Cargo feature** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have this link out to the documentation for the unstable feature? imo it would be good to help people track the progress. I went with the docs to add an extra step in the hope that it reduces "whats the status?" posts.
I was wondering about this for our other one but that seemed to have fallen through the cracks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add a link to the parallel frontend issue. There is no tracking issue for Cranelift, I think, but we can link to rust-lang/rust#77933.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I linked to the unstable Cargo -Zcodegen-backend
feature in Cargo reference in the book.
Recommendation: Add `-Zthreads=n` to the `RUSTFLAGS` environment variable[^rustflags], for example: | ||
|
||
```console | ||
$ RUSTFLAGS="-Zthreads=8" cargo build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we somewhere mention the way to make nightly your default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that seems a bit orthogonal to this document, and more related to rustup. We also don't describe how to actually install nightly (nor should we). The example should use +nightly
though, that's a good point.
I think it now looks as you wanted it to, but happy to make further edits :) |
Since we're looking at a couple more changes, mind cleaning up the commits while you are at it? For me, having them go back and forth in different directions makes it harder to review |
6f1c8df
to
a887a25
Compare
Ok, done. |
This extends the build performance guide in the Cargo book with the parallel frontend. This is the first mechanism we have in the guide that is not configured via profiles (unless we want to advertise setting
rustflags
through profiles?), but rather throughRUSTFLAGS
. Which means that we have to explain how to do that.I proposed using footnotes for this, so that we can reuse them also for other thing (such as explaining how to change a profile). An alternative would be to have a short paragraph at the beginning of the configuration subsection where we explain all the possible ways of configuring things, and then we refer to them.
This is a follow up to #15924
r? @epage