-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Build with -Zannotate-moves by default (non-stage-0 only) #148803
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
Conversation
|
This PR modifies If appropriate, please update This PR modifies If appropriate, please update |
2d5c26c to
550107b
Compare
|
Hi, just to clarify, the move annotations are represented in actual debuginfo, e.g. DWARF? Because we don't currently have an easy way of shipping that to end users (and thus we don't ship it), so doing it by default would be pretty useless. |
|
I was thinking it would primarily be useful to Rust developers doing profiling. But it would also apply to sysroot which does have shipped debuginfo, right? |
| // Enable move/copy annotations for profiler visibility if configured | ||
| // Skip stage 0 since the bootstrap compiler doesn't support this flag yet | ||
| if self.config.rust_annotate_moves.unwrap_or(true) && build_compiler_stage >= 1 { | ||
| if let Some(limit) = self.config.rust_annotate_moves_size_limit { | ||
| rustflags.arg(&format!("-Zannotate-moves={}", limit)); | ||
| } else { | ||
| rustflags.arg("-Zannotate-moves"); | ||
| } | ||
| } |
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.
Question: hold on, won't this affect stable compiler dist builds too? Or maybe I'm misunderstanding?
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.
Yeah, that should be fine. It will have no functional effect, aside from some extra debug info.
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.
There won't be any debuginfo, we strip it.
Disted rustc itself can't be profiled currently, as we don't ship enough information with it for that (we strip debuginfo). For stdlib it might be useful, in theory. Let's measure the size changes. @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Build with -Zannotate-moves by default (non-stage-0 only)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (7a0bdae): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResults (primary 3.5%, secondary 0.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.4%, secondary 0.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 476.181s -> 477.274s (0.23%) |
Yeah, by "Rust developers" I meant people checking out the git repo and building the toolchain for themselves. Those perf differences look spurious and the binary size increase is about what's expected, esp "Artifact size: 391.36 MiB -> 391.42 MiB (0.01%)". Which artifact is that? All of them? librustc_driver.so? There should be no increase to shipped artifacts if their debuginfo is stripped. |
|
The artifact size change is very close to, if not within noise. rustc-perf doesn't track artifact sizes over time like the other metrics so you have to squint at other perf runs to understand the build-to-build variation. I agree that setting the threshold to 65 is a good default. |
|
I see. Tbh I wouldn't even create a new bootstrap option for this, and just do it unconditionally when at least some debuginfo is requested for the compiler. It shouldn't make compilation time much slower, and binary size is not that important with debuginfo. |
|
I just wanted a way to control the threshold size in case you're using it to investigate something specific. But I'd be happy to remove all that if its excessive. |
|
Right, makes sense. A single flag should be enough for that, though. So I would suggest either:
What do you think? |
|
I guess 1 of those two options, but all other things being equal I think I prefer it as it is now. Removing the second option wouldn't significantly reduce the PR size or complexity, but it would map more directly to the semantics of the option itself and give full control. (I did experiment with exactly matching the option semantics with boolean-or-number, but the additional complexity of the toml deserialization didn't seem worth it.) |
| reproducible_artifacts: flags_reproducible_artifact, | ||
| reuse: build_reuse.map(PathBuf::from), | ||
| rust_analyzer_info, | ||
| rust_annotate_moves, |
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.
Let's store bool, and not Option<bool>, in the config, and set the default value to false.
Or the default could be true if any debuginfo is enabled.
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.
The option itself is a no-op unless you specify debuginfo, so there's no need to do it here.
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 see, I didn't check the implementation of the pass before. Ok, in that case I think that we can just enable it unconditionally (just hardcode passing -Zannotate-moves in bootstrap), because the binary size regression seems minimal (or it's just noise). And only allow configuring the size limit.
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.
OK updated so its always enabled and the only option is the size limit.
550107b to
60c2cbf
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
e32883f to
eeaa992
Compare
|
OK updated comment wording and change tracker. |
eeaa992 to
bc8d28e
Compare
Build rustc and tools with -Zannotate-moves by default. Adds toml config options to set the annotation size limit. This has no measurable effect on output binary size or compile time.
bc8d28e to
972498c
Compare
|
Rebased to resolve merge conflict. |
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Thank you! @bors r+ |
Build with -Zannotate-moves by default (non-stage-0 only) Build rustc and tools with -Zannotate-moves by default, both to exercise the feature and because could be useful for doing performance measurement on rustc and its tools. This has no effect on generated code, it just adds extra debug info in the form of some new inlined functions. This also adds bootstrap.toml config options ``` # rust.annotate-moves = true # rust.annotate-moves-size-limit = 65 ``` to allow this to be controlled locally. This is only added for stage 1 and later. Stage 0 (the bootstrap compiler) doesn't yet support -Zannotate-moves.
|
💔 Test failed - checks-actions |
|
The failure seems like it might be spurious. |
|
Yeah, probably runner went down. @bors retry |
|
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 217cb73 (parent) -> c199562 (this PR) Test differencesShow 4 test diffs4 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard c1995621a44398ac33ab368adbfb69753218b49e --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (c199562): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResults (primary 0.4%, secondary 0.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 474.105s -> 474.876s (0.16%) |
Build rustc and tools with -Zannotate-moves by default, both to exercise the feature and because could be useful for doing performance measurement on rustc and its tools. This has no effect on generated code, it just adds extra debug info in
the form of some new inlined functions.
This also adds bootstrap.toml config options
to allow this to be controlled locally.
This is only added for stage 1 and later. Stage 0 (the bootstrap compiler) doesn't yet support -Zannotate-moves.
#148197