-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Enable line-only debuginfo by default. #54459
Conversation
Related issue: #52179 |
@bors try for comparison artifacts |
⌛ Trying commit 6c4d7a31fdcf0a1488b003fd5fa4d3815c77efab with merge 5fd88ccbf4d20c70960c1b0e76fe82325e23a35e... |
☀️ Test successful - status-travis |
The debuginfo-only-std option was added in #38984 due to #37477 (backtraces super slow so I recall that when preparing #45523 I found that backtraces, while much faster with malloc/free, were still noticeably slow with malloc/free. To that end I tested this out:
Ok great! Looks like there's no slowdown here at all. Testing out a random test case with an ICE we can also see: Current Nightly
This PR
Sure enough there's some nice line numbers in there! Furthermore these both take roughly the same amount of time (maybe 100ms difference at most). All in all this seems fine by me, all the underlying bugs that motivated this decision originally have since been fixed, so it should be fine to do for our releases at least. One final thing to consider is that debuginfo-lines takes more time to build (as it's more "gunk" for LLVM to chew through). Do we want this turned on by default for all developers? Or only turned on for our releases? |
@alexcrichton Whoa, nice, thanks for the writeup! |
Looking at just the CI build for this PR and a recent successful build, it looks like for CI this adds about 10-15 minutes to build time. |
Wow, 10-15 minutes? I wouldn't have expected that. |
I want to see if the resulting compiler is also slower. @bors try |
We should already have a try build ready for this PR. |
FWIW hacking |
@rust-timer build 5fd88ccbf4d20c70960c1b0e76fe82325e23a35e |
Success: Queued 5fd88ccbf4d20c70960c1b0e76fe82325e23a35e with parent e7b5ba8, comparison URL. |
@alexcrichton @michaelwoerister I wonder how hard it would be to track down where we spend those extra dozen minutes - is it in |
Hm, 10-15 minutes of build time seems like it might be too long. Could we perhaps save some time by only enabling this in stage 2 and on dist builds by default on CI? We can leave it on by default for local builds, I think. |
@Mark-Simulacrum Heh, I wanted to say the stage 2 thing myself, but then I thought that maybe this is a bug, and not the expected outcome. |
Hm, the perf build showed essentially no instruction difference but some possible wall time losses so I'd presume the slowdown lies somewhere outside of the core compiler - maybe copying/reading files is taking slightly longer due to increased size? @nnethercote, is there a chance you have some ideas about how we could try to narrow down the problem? |
The comparison URL results look like noise to me. (I mostly ignore the As for the 10-15 minutes difference, I am skeptical about how reliable those time measurements are. I recently had a PR that was tested for landing and failed because one job hit the 3 hour timeout limit. Then on the subsequent run it only took 2.5 hours or so. So I would suggest remeasuring multiple times to get more confidence. |
I did two complete builds with the only difference being
Test command:
Without line debuginfo, this took
(Unfortunately, this also built a tiny bit of LLVM -- 73 files -- as I only noticed after the fact.) With line debuginfo, this took
In particular, stage1 compiler artifacts went up from
to
So, looks like line debuginfo makes the compiler around 10% slower. That's more than what I expected. Build log without line debuginfo
Build log with line debuginfo
|
Ping from triage @eddyb / @alexcrichton! It' been a while since this PR has been updated. How do you want to move forward with it? |
Ping from triage @eddyb! We haven't heard from you in a while so I'm closing PR for, per our guidelines. Thanks for your contributions and please feel free to re-open this PR in the future. |
@alexcrichton Yes, I'd like to land this, but only if the slowdown is considered acceptable, or someone else investigates it. |
@michaelwoerister Could this be from linking? Have we experimented with lld for linking |
Hard to tell without actual numbers.
The dist builds on CI already use LLD. I don't know about the other builders. |
Could we merge this and see if it really affects CI build times as negatively as it looked in that one build? |
@michaelwoerister And revert it if we see a statistically significant impact? |
6c4d7a3
to
5256d3e
Compare
@eddyb Yes. If it causes trouble, we can still back it out. |
May concerns on compilation performance have been addressed. Since the remaining concerns are about CI build times: r? @alexcrichton |
I don't think we have a great grasp on what the CI times here are, and we're already severely bleeding for CI times right now as most builds are pushing 2.5 hours when we ideally shoot for 2 hours. I would personally rather get build times under control before we continue to land minor build time regressions like this. We can make exceptions of course, but this seems like a nice-to-have rather than a hard requirement of something. |
And some Windows builds are regularly exceeding 3 hours. |
Ping from triage. @alexcrichton any updates on this? is this blocked on anything? |
I personally feel the same way still. I don't think we have budget on CI to land this yet |
Ping from triage: based on #54459 (comment) I'll close this for now. Let's revisit if our CI situation changes. |
Previously, only official builds (stable/beta/nightly) had
debuginfo-lines
enabled, but it was in conjunction withdebuginfo-only-std
, meaning that the compiler had no debuginfo.In my experience, when you have a panic/crash in
rustc
, line debuginfo is necessary to get a readable backtrace, and we would've saved ourselves some pain and time, had we done this earlier.At least we can try to explore and discuss the tradeoffs involved here.
r? @alexcrichton cc @rust-lang/infra @rust-lang/compiler