-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Make compare_predicate_entailment
no longer a query
#101615
Conversation
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 112458bd85c9e8d2692233dce436912c88f35521 with merge 89b3f54f5de96e6fd11b7d992d9b64f335997aa5... |
💔 Test failed - checks-actions |
112458b
to
d2cd1b7
Compare
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit d2cd1b7c1d284dbf502e09f666c64150ce2abad2 with merge ee778fd79f1ba1246047b1f652c6ef0a135c1e49... |
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
☀️ Try build successful - checks-actions |
Queued ee778fd79f1ba1246047b1f652c6ef0a135c1e49 with parent 98f3001, future comparison URL. |
Finished benchmarking commit (ee778fd79f1ba1246047b1f652c6ef0a135c1e49): comparison URL. Overall result: ❌✅ regressions and improvements - no action neededBenchmarking 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. @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
Footnotes |
d2cd1b7
to
2e1da58
Compare
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 2e1da580dd11d4fdf8763638f771e41dfcfebd58 with merge 46ec702e9dd4258388a00180e4de0d629dfdad33... |
2e1da58
to
62164aa
Compare
@bors try |
Finished benchmarking commit (0531a017ad453e9f1bc2ab9193c17642e1ece6f2): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking 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. @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
Footnotes |
62164aa
to
bec079d
Compare
@bors try @rust-timer queue r? @oli-obk just because you know this code, please feel free to reassign |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit bec079d with merge 58c417521d7df5a75c6abc60cbba95b4e4b49f2b... |
compare_predicates_and_trait_impl_trait_tys
compare_predicate_entailment
no longer a query
☀️ Try build successful - checks-actions |
Queued 58c417521d7df5a75c6abc60cbba95b4e4b49f2b with parent 395e56f, future comparison URL. |
Finished benchmarking commit (58c417521d7df5a75c6abc60cbba95b4e4b49f2b): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking 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. @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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. Footnotes |
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (1ce5198): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
Footnotes |
Make
compare_predicate_entailment
so it's no longer a query (again), and splits out the new logic (that equates the return types to infer RPITITs) into its own query. This means that this new query (now calledcollect_trait_impl_trait_tys
) is no longer executed for non-RPITIT cases.This should improve perf (#101224 (comment)), though in practice we see that these some crates remain from the primary regressions list on the original report... They are all <= 0.43% regression and seemingly only on the incr-full scenario for all of them.
I am at a loss for what might be causing this regression other than what I fixed here, since we don't introduce much new non-RPITIT logic except for some
def_kind
query calls in some places, for example, like projection. Maybe that's it?Originally this PR was opened to test enabling
cache_on_disk
(62164aaaa11) but that didn't turn out to be very useful (#101615 (comment)), so that led me to just split the query (and rename the PR).