-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
fast-reject: add depth check #133566
fast-reject: add depth check #133566
Conversation
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
fast-reject: add counter to avoid hangs alternative approach to rust-lang#133524 r? `@compiler-errors`
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
if this ends up neutral then r=me |
Finished benchmarking commit (96e2f40): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 1.0%, secondary -2.9%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary 4.9%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 792.58s -> 793.229s (0.08%) |
@@ -273,6 +278,12 @@ impl<I: Interner, const INSTANTIATE_LHS_WITH_INFER: bool, const INSTANTIATE_RHS_ | |||
| ty::Placeholder(_) => {} | |||
}; | |||
|
|||
self.num_non_trivial_types += 1; |
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.
Maybe move this into a wrapper around types_may_unify
to only pay the cost when recursing?
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 biggest one is typenum, tbh I would not be surprised if typenum actually relies on fast reject with large enough types
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.
Oh... yea I think we can treat typenum "regressions below 100% are fine" if sufficiently motivated. Which this PR is.
39841a4
to
f6f4396
Compare
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
fast-reject: add cache slightly modified version of rust-lang#133524 I originally wanted to simply bail after recursion for a certain amount of times, however, looking at the number of steps taken while compiling different crates we get the following results[^1]: typenum ```rust 1098842 counts ( 1) 670511 (61.0%, 61.0%): dropping after 1 ( 2) 358785 (32.7%, 93.7%): dropping after 0 ( 3) 25191 ( 2.3%, 96.0%): dropping after 2 ( 4) 10912 ( 1.0%, 97.0%): dropping after 4 ( 5) 6461 ( 0.6%, 97.5%): dropping after 3 ( 6) 5239 ( 0.5%, 98.0%): dropping after 5 ( 7) 2528 ( 0.2%, 98.3%): dropping after 8 ( 8) 2188 ( 0.2%, 98.5%): dropping after 1094 ( 9) 2097 ( 0.2%, 98.6%): dropping after 6 ( 10) 1179 ( 0.1%, 98.7%): dropping after 34 ( 11) 1148 ( 0.1%, 98.9%): dropping after 7 ( 12) 822 ( 0.1%, 98.9%): dropping after 10 ``` bitmaps ```rust 533346 counts ( 1) 526166 (98.7%, 98.7%): dropping after 1 ( 2) 4562 ( 0.9%, 99.5%): dropping after 0 ( 3) 2072 ( 0.4%, 99.9%): dropping after 1024 ( 4) 305 ( 0.1%,100.0%): dropping after 2 ( 5) 106 ( 0.0%,100.0%): dropping after 4 ( 6) 30 ( 0.0%,100.0%): dropping after 8 ( 7) 18 ( 0.0%,100.0%): dropping after 3 ( 8) 17 ( 0.0%,100.0%): dropping after 44 ( 9) 15 ( 0.0%,100.0%): dropping after 168 ( 10) 8 ( 0.0%,100.0%): dropping after 14 ( 11) 7 ( 0.0%,100.0%): dropping after 13 ( 12) 7 ( 0.0%,100.0%): dropping after 24 ``` stage 2 compiler is mostly trivial, but has a few cases where we get >5000 ```rust 12987156 counts ( 1) 9280476 (71.5%, 71.5%): dropping after 0 ( 2) 2277841 (17.5%, 89.0%): dropping after 1 ( 3) 724888 ( 5.6%, 94.6%): dropping after 2 ( 4) 204005 ( 1.6%, 96.2%): dropping after 4 ( 5) 146537 ( 1.1%, 97.3%): dropping after 3 ( 6) 64287 ( 0.5%, 97.8%): dropping after 5 ( 7) 43938 ( 0.3%, 98.1%): dropping after 6 ( 8) 43758 ( 0.3%, 98.4%): dropping after 8 ( 9) 27220 ( 0.2%, 98.7%): dropping after 7 ( 10) 17374 ( 0.1%, 98.8%): dropping after 9 ( 11) 16015 ( 0.1%, 98.9%): dropping after 10 ( 12) 12855 ( 0.1%, 99.0%): dropping after 12 ( 13) 10494 ( 0.1%, 99.1%): dropping after 11 ( 14) 7553 ( 0.1%, 99.2%): dropping after 14 ``` Given that we have crates which frequently rely on fairly deep recursion, actually using a cache seems better than using an arbitrary cutoff here. Having an impl which is large enough to trigger a cutoff instead of getting rejected noticeably impacts perf, so just using a cache in these cases seems better to me. Does not matter too much in the end, we only have to make sure we don't regress crates which don't recurse deeply. [^1]: i've incremented a counter in the place I now call `if cache.get(&(lhs, rhs))` and then printed it on drop r? `@compiler-errors`
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (613c078): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary -1.3%, secondary 3.8%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 773.306s -> 773.019s (-0.04%) |
f6f4396
to
d2c948f
Compare
fast-reject: add cache slightly modified version of rust-lang#133524 I originally wanted to simply bail after recursion for a certain amount of times, however, looking at the number of steps taken while compiling different crates we get the following results[^1]: typenum ```rust 1098842 counts ( 1) 670511 (61.0%, 61.0%): dropping after 1 ( 2) 358785 (32.7%, 93.7%): dropping after 0 ( 3) 25191 ( 2.3%, 96.0%): dropping after 2 ( 4) 10912 ( 1.0%, 97.0%): dropping after 4 ( 5) 6461 ( 0.6%, 97.5%): dropping after 3 ( 6) 5239 ( 0.5%, 98.0%): dropping after 5 ( 7) 2528 ( 0.2%, 98.3%): dropping after 8 ( 8) 2188 ( 0.2%, 98.5%): dropping after 1094 ( 9) 2097 ( 0.2%, 98.6%): dropping after 6 ( 10) 1179 ( 0.1%, 98.7%): dropping after 34 ( 11) 1148 ( 0.1%, 98.9%): dropping after 7 ( 12) 822 ( 0.1%, 98.9%): dropping after 10 ``` bitmaps ```rust 533346 counts ( 1) 526166 (98.7%, 98.7%): dropping after 1 ( 2) 4562 ( 0.9%, 99.5%): dropping after 0 ( 3) 2072 ( 0.4%, 99.9%): dropping after 1024 ( 4) 305 ( 0.1%,100.0%): dropping after 2 ( 5) 106 ( 0.0%,100.0%): dropping after 4 ( 6) 30 ( 0.0%,100.0%): dropping after 8 ( 7) 18 ( 0.0%,100.0%): dropping after 3 ( 8) 17 ( 0.0%,100.0%): dropping after 44 ( 9) 15 ( 0.0%,100.0%): dropping after 168 ( 10) 8 ( 0.0%,100.0%): dropping after 14 ( 11) 7 ( 0.0%,100.0%): dropping after 13 ( 12) 7 ( 0.0%,100.0%): dropping after 24 ``` stage 2 compiler is mostly trivial, but has a few cases where we get >5000 ```rust 12987156 counts ( 1) 9280476 (71.5%, 71.5%): dropping after 0 ( 2) 2277841 (17.5%, 89.0%): dropping after 1 ( 3) 724888 ( 5.6%, 94.6%): dropping after 2 ( 4) 204005 ( 1.6%, 96.2%): dropping after 4 ( 5) 146537 ( 1.1%, 97.3%): dropping after 3 ( 6) 64287 ( 0.5%, 97.8%): dropping after 5 ( 7) 43938 ( 0.3%, 98.1%): dropping after 6 ( 8) 43758 ( 0.3%, 98.4%): dropping after 8 ( 9) 27220 ( 0.2%, 98.7%): dropping after 7 ( 10) 17374 ( 0.1%, 98.8%): dropping after 9 ( 11) 16015 ( 0.1%, 98.9%): dropping after 10 ( 12) 12855 ( 0.1%, 99.0%): dropping after 12 ( 13) 10494 ( 0.1%, 99.1%): dropping after 11 ( 14) 7553 ( 0.1%, 99.2%): dropping after 14 ``` Given that we have crates which frequently rely on fairly deep recursion, actually using a cache seems better than using an arbitrary cutoff here. Having an impl which is large enough to trigger a cutoff instead of getting rejected noticeably impacts perf, so just using a cache in these cases seems better to me. Does not matter too much in the end, we only have to make sure we don't regress crates which don't recurse deeply. [^1]: i've incremented a counter in the place I now call `if cache.get(&(lhs, rhs))` and then printed it on drop r? `@compiler-errors`
0dc67dc
to
59afc76
Compare
59afc76
to
3465ce5
Compare
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
fast-reject: add cache slightly modified version of rust-lang#133524 I originally wanted to simply bail after recursion for a certain amount of times, however, looking at the number of steps taken while compiling different crates we get the following results[^1]: typenum ```rust 1098842 counts ( 1) 670511 (61.0%, 61.0%): dropping after 1 ( 2) 358785 (32.7%, 93.7%): dropping after 0 ( 3) 25191 ( 2.3%, 96.0%): dropping after 2 ( 4) 10912 ( 1.0%, 97.0%): dropping after 4 ( 5) 6461 ( 0.6%, 97.5%): dropping after 3 ( 6) 5239 ( 0.5%, 98.0%): dropping after 5 ( 7) 2528 ( 0.2%, 98.3%): dropping after 8 ( 8) 2188 ( 0.2%, 98.5%): dropping after 1094 ( 9) 2097 ( 0.2%, 98.6%): dropping after 6 ( 10) 1179 ( 0.1%, 98.7%): dropping after 34 ( 11) 1148 ( 0.1%, 98.9%): dropping after 7 ( 12) 822 ( 0.1%, 98.9%): dropping after 10 ``` bitmaps ```rust 533346 counts ( 1) 526166 (98.7%, 98.7%): dropping after 1 ( 2) 4562 ( 0.9%, 99.5%): dropping after 0 ( 3) 2072 ( 0.4%, 99.9%): dropping after 1024 ( 4) 305 ( 0.1%,100.0%): dropping after 2 ( 5) 106 ( 0.0%,100.0%): dropping after 4 ( 6) 30 ( 0.0%,100.0%): dropping after 8 ( 7) 18 ( 0.0%,100.0%): dropping after 3 ( 8) 17 ( 0.0%,100.0%): dropping after 44 ( 9) 15 ( 0.0%,100.0%): dropping after 168 ( 10) 8 ( 0.0%,100.0%): dropping after 14 ( 11) 7 ( 0.0%,100.0%): dropping after 13 ( 12) 7 ( 0.0%,100.0%): dropping after 24 ``` stage 2 compiler is mostly trivial, but has a few cases where we get >5000 ```rust 12987156 counts ( 1) 9280476 (71.5%, 71.5%): dropping after 0 ( 2) 2277841 (17.5%, 89.0%): dropping after 1 ( 3) 724888 ( 5.6%, 94.6%): dropping after 2 ( 4) 204005 ( 1.6%, 96.2%): dropping after 4 ( 5) 146537 ( 1.1%, 97.3%): dropping after 3 ( 6) 64287 ( 0.5%, 97.8%): dropping after 5 ( 7) 43938 ( 0.3%, 98.1%): dropping after 6 ( 8) 43758 ( 0.3%, 98.4%): dropping after 8 ( 9) 27220 ( 0.2%, 98.7%): dropping after 7 ( 10) 17374 ( 0.1%, 98.8%): dropping after 9 ( 11) 16015 ( 0.1%, 98.9%): dropping after 10 ( 12) 12855 ( 0.1%, 99.0%): dropping after 12 ( 13) 10494 ( 0.1%, 99.1%): dropping after 11 ( 14) 7553 ( 0.1%, 99.2%): dropping after 14 ``` Given that we have crates which frequently rely on fairly deep recursion, actually using a cache seems better than using an arbitrary cutoff here. Having an impl which is large enough to trigger a cutoff instead of getting rejected noticeably impacts perf, so just using a cache in these cases seems better to me. Does not matter too much in the end, we only have to make sure we don't regress crates which don't recurse deeply. [^1]: i've incremented a counter in the place I now call `if cache.get(&(lhs, rhs))` and then printed it on drop r? `@compiler-errors`
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (2b0fb20): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (secondary 4.5%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary 4.7%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 772.629s -> 773.479s (0.11%) |
I think we need to eat this perf regression to make the trait solver feasible to land :> r=me |
@bors r=compiler-errors rollup=never |
☀️ Test successful - checks-actions |
Finished benchmarking commit (bd36e69): comparison URL. Overall result: ❌ regressions - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 0.7%, secondary -4.4%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 768.592s -> 768.435s (-0.02%) |
Only a small regression, multiple strategies were tried here and in other PRs to get to this result — and it's a necessary hit.
from #133566 (comment) @rustbot label: +perf-regression-triaged |
slightly modified version of #133524
I tried a few alternatives:
typenum
bitmaps
stage 2 compiler is mostly trivial, but has a few cases where we get >5000
r? @compiler-errors
Footnotes
i've incremented a counter in the place I now decrement the depth at and then printed it on drop ↩