Repository navigation
Fix ordering for default impl check in the new solver - #161268
Conversation
The second commit of rust-lang#160605 moved the `default impl` check later, for better performance, which introduced a regression. This commit moves the check a little earlier, so it is after the `args_may_unify` call (thus retaining the perf benefit) but before the `probe_trait_candidate` (which has side-effects). The check is now duplicated in three `GoalKind::consider_impl_candidate` methods, which is unfortunate, but it fits in with the existing duplicated code in those methods. And it means another copy of the check (in `try_assemble_bounds_via_registered_opaques`) can be removed. Fixes rust-lang#160994.
|
|
LLM disclosure: an LLM helped with analysis and review of this PR. I wrote all the code and text myself. |
|
@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.
Fix ordering for `default impl` check in the new solver
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (71c4692): comparison URL. Overall result: ❌ regressions - 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)Results (primary 2.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis perf run didn't have relevant results for this metric. Binary sizeResults (primary -0.0%, secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 458.344s -> 457.305s (-0.23%) |
There was a problem hiding this comment.
one other option: could we change the for_each_relevant_impl and for_each_blanket_impl queries to only return non-default impls and have a separate for_each_default_impl?
r=me on this change itself, even if I quite dislike default impls to negatively impact perf here
|
Let's merge this because it fixes a clear problem; the @bors r=lcnr |
This comment has been minimized.
This comment has been minimized.
Fix ordering for `default impl` check in the new solver The second commit of #160605 moved the `default impl` check later, for better performance, which introduced a regression. This commit moves the check a little earlier, so it is after the `args_may_unify` call (thus retaining the perf benefit) but before the `probe_trait_candidate` (which has side-effects). The check is now duplicated in three `GoalKind::consider_impl_candidate` methods, which is unfortunate, but it fits in with the existing duplicated code in those methods. And it means another copy of the check (in `try_assemble_bounds_via_registered_opaques`) can be removed. Fixes #160994. r? @lcnr
|
💔 Test for e17ef52 failed: CI. Failed job:
|
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@bors retry |
|
@bors p=6 scheduling |
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 124c16e (parent) -> 3009fdd (this PR) Test differencesShow 40 test diffsStage 1
Stage 2
Additionally, 34 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 3009fdd3756436133ed9ea6b165a655c28f6857a --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 (3009fdd): comparison URL. Overall result: ✅ improvements - 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)Results (primary -3.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -3.2%, secondary -5.1%)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: 467.816s -> 468.178s (0.08%) |
View all comments
The second commit of #160605 moved the
default implcheck later, for better performance, which introduced a regression. This commit moves the check a little earlier, so it is after theargs_may_unifycall (thus retaining the perf benefit) but before theprobe_trait_candidate(which has side-effects).The check is now duplicated in three
GoalKind::consider_impl_candidatemethods, which is unfortunate, but it fits in with the existing duplicated code in those methods. And it means another copy of the check (intry_assemble_bounds_via_registered_opaques) can be removed.Fixes #160994.
r? @lcnr