[perf] Specialize common (1, 1) case for arg unification.#158865
Conversation
|
cc @Kobzol for perf run |
|
@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.
[perf] Specialize common `(1, 1)` case for arg unification.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (08a2c83): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking 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. @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)Results (primary -0.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 3.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 491.018s -> 489.64s (-0.28%) |
|
@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.
[perf] Specialize common `(1, 1)` case for arg unification.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (160aab0): comparison URL. Overall result: ❌✅ regressions and improvements - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -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 perf run didn't have relevant results for this metric. CyclesResults (primary -0.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: 487.206s -> 495.006s (1.60%) |
|
Having the I'm planning to remove it from this PR. If I've misread the data, I'm happy to be told so and can put it back :) |
bb64fd1 to
f09561b
Compare
This comment has been minimized.
This comment has been minimized.
[perf] Specialize common `(1, 1)` case for arg unification. While `DeepRejectCtxt::args_may_unify_inner` might be called with different numbers of obligation and impl args, the `(1, 1)` length case is very common: single-generic-param ADTs, trait references whose only generic is `Self`, etc. Offer a fast path for that simple case, so that we avoid invoking `iter::zip(...).all(predicate)` iterator machinery given that it isn't necessary there. We considered, and ultimately rejected, adding a separate specialization for the `(0, 0)` case since benchmarks indicated it wasn't cost-effective. r? @oli-obk **AI disclosure:** The optimization opportunity here was discovered as part of a systematic probe for missed optimizations utilizing both traditional and AI tools. The code here was initially prototyped and vetted by AI tools, followed by additional manual work. I secured approval in advance from the designated reviewer. I stand behind the quality of the code I'm submitting, and I vouch it's as good or better compared to if I had written every line by my own hand.
|
@bors yield |
|
Auto build was cancelled. Cancelled workflows: The next pull request likely to be tested is #158958. |
|
@bors p=6 scheduling |
This comment has been minimized.
This comment has been minimized.
[perf] Specialize common `(1, 1)` case for arg unification. While `DeepRejectCtxt::args_may_unify_inner` might be called with different numbers of obligation and impl args, the `(1, 1)` length case is very common: single-generic-param ADTs, trait references whose only generic is `Self`, etc. Offer a fast path for that simple case, so that we avoid invoking `iter::zip(...).all(predicate)` iterator machinery given that it isn't necessary there. We considered, and ultimately rejected, adding a separate specialization for the `(0, 0)` case since benchmarks indicated it wasn't cost-effective. r? @oli-obk **AI disclosure:** The optimization opportunity here was discovered as part of a systematic probe for missed optimizations utilizing both traditional and AI tools. The code here was initially prototyped and vetted by AI tools, followed by additional manual work. I secured approval in advance from the designated reviewer. I stand behind the quality of the code I'm submitting, and I vouch it's as good or better compared to if I had written every line by my own hand.
|
💔 Test for dd4afd8 failed: CI. Failed job:
|
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@bors retry network failure |
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 561ea2b (parent) -> 3664b37 (this PR) Test differencesShow 2 test diffs2 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 3664b37017c529cad0f0ed259769f15743c21a6a --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 (3664b37): 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 (secondary 6.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.9%, secondary 9.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.1%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 489.147s -> 487.967s (-0.24%) |

View all comments
While
DeepRejectCtxt::args_may_unify_innermight be called with different numbers of obligation and impl args, the(1, 1)length case is very common: single-generic-param ADTs, trait references whose only generic isSelf, etc.Offer a fast path for that simple case, so that we avoid invoking
iter::zip(...).all(predicate)iterator machinery given that it isn't necessary there.We considered, and ultimately rejected, adding a separate specialization for the
(0, 0)case since benchmarks indicated it wasn't cost-effective.r? @oli-obk
AI disclosure: The optimization opportunity here was discovered as part of a systematic probe for missed optimizations utilizing both traditional and AI tools. The code here was initially prototyped and vetted by AI tools, followed by additional manual work. I secured approval in advance from the designated reviewer. I stand behind the quality of the code I'm submitting, and I vouch it's as good or better compared to if I had written every line by my own hand.