Avoid excessive memcpys with the new solver - #160005
Conversation
|
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor These commits modify the If this was unintentional then you should revert the changes before this PR is merged. Some changes occurred in compiler/rustc_attr_parsing cc @jdonszelmann, @JonathanBrouwer The parser was modified, potentially altering the grammar of (stable) Rust cc @fmease |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Avoid excessive `memcpy`s with the new solver
This comment has been minimized.
This comment has been minimized.
06dbf32 to
bc91096
Compare
Done. The perf run seems to be stuck. I re-pushed; let's try again. @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Avoid excessive `memcpy`s with the new solver
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (3f08afa): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)This perf run didn't have relevant results for this metric. CyclesResults (secondary -0.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 486.74s -> 486.287s (-0.09%) |
In `try_evaluate_obligations` we call `drain_pending` with a `|_, _| true` closure. This closure is then used in a call to `partition`, which is a very complicated way of moving all the elements into a new vector that is identical to the old one. Each of those moves involves a `memcpy` because the element type is large. This commit changes the code to avoid the useless partition and just take the vector directly, for a ~40% instruction count reduction when doing `cargo check`.
`PendingObligations` is a `ThinVec` containing a pair `(PredicateObligation<'tcx>, Option<GoalStalledOn<TyCtxt<'tcx>>>)`. This pair is currently 136 bytes. This is more than 128 bytes, which means that LLVM uses (slow) `memcpy` to move the elements. This commit shrinks `GoalStalledOn` by changing its two `Vec` fields to `ThinVec`. This shrinks the pair from 136 bytes to 104 bytes, under the `memcpy` threshold. The use of `ThinVec` in `GoalStalledOn` forces us to use `ThinVec` in a number of other places. Some of these required the thin-vec `unstable` feature for `may_dangle` support. The commit also adds some size assertions, to prevent backsliding.
bc91096 to
94a8720
Compare
|
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. |
|
@bors r+ |
This comment has been minimized.
This comment has been minimized.
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 e7b5955 (parent) -> e19d321 (this PR) Test differencesShow 25 test diffs25 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard e19d321c06479c6fd77533582b0d5a86651f1be3 --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 (e19d321): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Our benchmarks found a performance regression caused by this PR. Next Steps:
@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)Results (primary -0.8%, secondary 4.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.0%, secondary 1.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 490.22s -> 490.165s (-0.01%) |

View all comments
Cachegrind says that rustc with the new solver spends an enormous amount of time running
memcpywhen compilingnacl-0.5.3,ijson-0.1.6, andnvml-wrapper-sys-0.9.1. DHAT's copy mode says there are two places responsible for these excessivememcpys. This PR fixes them, for big performance wins on those crates. Details in the individual commits.r? @lcnr