-
Notifications
You must be signed in to change notification settings - Fork 985
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
cargo: straighten out LTO configuration #5955
Conversation
This PR tweaks the change made in #5904 so that the `profiling` Cargo profile does _not_ have LTO enabled. With LTO enabled, compile times even after just doing a `touch crates/uv/src/bin/uv.rs` are devastating: $ cargo b --profile profiling -p uv Compiling uv-cli v0.0.1 (/home/andrew/astral/uv/crates/uv-cli) Compiling uv v0.2.34 (/home/andrew/astral/uv/crates/uv) Finished `profiling` profile [optimized + debuginfo] target(s) in 3m 47s Even with `lto = "thin"`, compile times are not great, but an improvement: $ cargo b --profile profiling -p uv Compiling uv v0.2.34 (/home/andrew/astral/uv/crates/uv) Finished `profiling` profile [optimized + debuginfo] target(s) in 53.98s But our original configuration for `profiling`, prior to #5904, was with LTO completely disabled: $ cargo b --profile profiling -p uv Compiling uv v0.2.34 (/home/andrew/astral/uv/crates/uv) Finished `profiling` profile [optimized + debuginfo] target(s) in 30.09s This gives reasonable-ish compile times, although I still want them to be better. This setup does risk that we are measuring something in benchmarks that we are shipping, but in order to make those two the same, we'd either need to make compile times way worse for development, or take a hit to binary size and a slight hit to runtime performance in our release builds. I would weakly prefer that we accept the hit to runtime performance and binary size in order to bring our measurements in line with what we ship, but I _strongly_ feel that we should not have compile times exceeding minutes for development. When doing performance testing, long compile times, for me anyway, break "flow" state. A confounding factor here was that #5904 enabled LTO for the `release` profile, but the `dist` profile (used by `cargo dist`) was still setting it to `lto = "thin"`. However, because of shenanigans in our release pipeline, we we actually using the `release` profile for binaries we ship. This PR does not make any changes here other than to remove `lto = "thin"` from the `dist` profile to make the fact that they are the same a bit clearer. cc @davfsa
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 mainly want lto for the 15% - 20% compressed size reduction, i'm fine accepting a minor divergence between profiling and release for that
Thanks Andrew! |
@@ -205,12 +205,49 @@ rest_pat_in_fully_bound_structs = "warn" | |||
|
|||
[profile.release] | |||
strip = true | |||
lto = true | |||
lto = "fat" |
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.
"fat"
and true
are the same, but fat
is more descriptive. The boolean value is a holdover from the early days when lto
was a strict binary option.
[profile.profiling] | ||
inherits = "release" | ||
strip = false | ||
debug = true | ||
debug = "full" |
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.
"full"
is equivalent to true
, but full
is more descriptive.
This PR tweaks the change made in #5904 so that the
profiling
Cargoprofile does not have LTO enabled. With LTO enabled, compile times
even after just doing a
touch crates/uv/src/bin/uv.rs
are devastating:Even with
lto = "thin"
, compile times are not great, but animprovement:
But our original configuration for
profiling
, prior to #5904, was withLTO completely disabled:
This gives reasonable-ish compile times, although I still want them to
be better.
This setup does risk that we are measuring something in benchmarks that
we are shipping, but in order to make those two the same, we'd either
need to make compile times way worse for development, or take a hit
to binary size and a slight hit to runtime performance in our release
builds. I would weakly prefer that we accept the hit to runtime
performance and binary size in order to bring our measurements in line
with what we ship, but I strongly feel that we should not have compile
times exceeding minutes for development. When doing performance testing,
long compile times, for me anyway, break "flow" state.
A confounding factor here was that #5904 enabled LTO for the
release
profile, but the
dist
profile (used bycargo dist
) was still settingit to
lto = "thin"
. However, because of shenanigans in our releasepipeline, we we actually using the
release
profile for binaries weship. This PR does not make any changes here other than to remove
lto = "thin"
from thedist
profile to make the fact that they are the samea bit clearer.
cc @davfsa