cargo: straighten out LTO configuration#5955
Merged
BurntSushi merged 1 commit intomainfrom Aug 9, 2024
Merged
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
konstin
approved these changes
Aug 9, 2024
Member
konstin
left a comment
konstin
left a comment
There was a problem hiding this comment.
I mainly want lto for the 15% - 20% compressed size reduction, i'm fine accepting a minor divergence between profiling and release for that
charliermarsh
approved these changes
Aug 9, 2024
Member
|
Thanks Andrew! |
BurntSushi
commented
Aug 9, 2024
| [profile.release] | ||
| strip = true | ||
| lto = true | ||
| lto = "fat" |
Member
Author
There was a problem hiding this comment.
"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.
| inherits = "release" | ||
| strip = false | ||
| debug = true | ||
| debug = "full" |
Member
Author
There was a problem hiding this comment.
"full" is equivalent to true, but full is more descriptive.
charliermarsh
added
the
releases
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR tweaks the change made in #5904 so that the
profilingCargoprofile does not have LTO enabled. With LTO enabled, compile times
even after just doing a
touch crates/uv/src/bin/uv.rsare 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
releaseprofile, but the
distprofile (used bycargo dist) was still settingit to
lto = "thin". However, because of shenanigans in our releasepipeline, we we actually using the
releaseprofile for binaries weship. This PR does not make any changes here other than to remove
lto = "thin"from thedistprofile to make the fact that they are the samea bit clearer.
cc @davfsa