-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Substantial debug-test-optimized-depedencies Performance Regression in 1.59 #97460
Comments
Sets the rustc
Sets the rustc |
So this changed in cargo/rustc 1.59? |
Also, this doesn't match |
To answer my own question, yes, this changed in 1.59, |
(FWIW this does match my observation that rustc 1.59 |
I'm not aware of any changes to rustc or cargo in the 1.59 release that would have an effect here. (You can find the list of PRs that landed in 1.59 for both projects here and here)
I see this with Rust 1.59:
Which shows I see the same on 1.58 as well
Edit: I realized that is |
Grrrrrrr I'm so sorry, I was looking at the wrong version, the regression was between 1.56 and 1.58, let me go test now. |
1.57 contained the Cargo "named profiles" feature (rust-lang/cargo#9943) which notes:
From what I'm seeing, prior to 1.57, the above Cargo.toml has no effect on Edit: Another interesting note from that PR:
|
Ah, right, so that makes sense. That doesn't, however, explain why tests got so much slower on 1.59. Ran some numbers with dev-1 and dev.package.*-2:
|
This is way more than the 25% regression in |
In all the above results that's a second run of That's why my initial response was to |
Sorry, the above profile was a bit confusing - on the 1.58.1 build |
Updated the issue description to remove all the stuff about dev-1 and the stuff that was just the cargo change, leaving only the parts talking about the |
Tagging this as medium priority because it is such a large regression if correct, but in a less-common combination of compiler settings, so something going awry is both expected but also it going this awry is surprising. |
We were trying to change our Cargo.toml to
to get opt on our dependencies during test but not on the crate being tested. In rustc 1.58.1, this is a performance improvement in the generated test suite. On rustc 1.59.0 this is a very substantial regression in total runtime of tests (~2 minutes CPU time on 1.58.1 to 18 minutes CPU time, not including compile time). This effect is robust with different opt-levels (swapping the 1s and 2s above) and between Debian rustc and rustup rustc.
The increase in CPU time seems to almost all be in
std
/core
, perf on the regressed binary for a single test (similar results for all tests) shows the following, butcore
itself doesn't show up at all in perf on 1.58.1.This effect is also robust against
-Zbuild-std=panic_abort
(andps aux
confirms thatcore
is getting built withopt-level=2
) and evenRUSTFLAGS="-C prefer-dynamic"
(whichldd
confirms dynamically links libstd+libtest).The PR to do this on our repo is at lightningdevkit/rust-lightning#1497 which you should be able to simply check out locally and
cargo test
with 1.58 and 1.59 (well, run it twice to skip the compile part of the test) to see the regression. Specific numbers at #97460 (comment).For those reading the comments there was some confusion about the cargo 1.57 change on the test profile, you can skip to #97460 (comment)
@rustbot modify labels: +regression-from-stable-to-stable -regression-untriaged
The text was updated successfully, but these errors were encountered: