Skip to content

Improve caching during trait evaluation #86946

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 22, 2021
Merged

Improve caching during trait evaluation #86946

merged 1 commit into from
Jul 22, 2021

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented Jul 7, 2021 •
edited
Loading

Previously, we would 'forget' that we had 'static regions in some
place during trait evaluation. This lead to us producing
EvaluatedToOkModuloRegions when we could have produced
EvaluatedToOk, causing us to perform unnecessary work.

This PR preserves 'static regions when we canonicalize a predicate for
evaluate_obligation, and when we 'freshen' a predicate during trait
evaluation. Thie ensures that evaluating a predicate containing
'static regions can produce EvaluatedToOk (assuming that we
don't end up introducing any region dependencies during evaluation).

Building off of this improved caching, we use
predicate_must_hold_considering_regions during fulfillment of
projection predicates to see if we can skip performing additional work.
We already do this for trait predicates, but doing this for projection
predicates lead to mixed performance results without the above caching
improvements.

@rust-highfive
Copy link
Contributor

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 7, 2021
@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 7, 2021
@bors
Copy link
Collaborator

bors commented Jul 7, 2021

⌛ Trying commit f2233a62c193f9d81391ce078821a92226d6562a with merge e0077e80d6baf40bb5ea72ad1b936ed2d24c76c6...

@bors
Copy link
Collaborator

bors commented Jul 7, 2021

☀️ Try build successful - checks-actions
Build commit: e0077e80d6baf40bb5ea72ad1b936ed2d24c76c6 (e0077e80d6baf40bb5ea72ad1b936ed2d24c76c6)

@rust-timer
Copy link
Collaborator

Queued e0077e80d6baf40bb5ea72ad1b936ed2d24c76c6 with parent c5e344f, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (e0077e80d6baf40bb5ea72ad1b936ed2d24c76c6): comparison url.

Summary: This change led to significant improvements 🎉 in compiler performance.

  • Moderate improvement in instruction counts (up to -2.0% on full builds of regression-31157-check)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking 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 led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 7, 2021
@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 7, 2021
@bors
Copy link
Collaborator

bors commented Jul 7, 2021

⌛ Trying commit f8ac29b10e698d1ab743b1e068d77d13459e78ef with merge 8755dc80ab36003a2c81d2ef6e4cc4c81221d6f2...

@bors
Copy link
Collaborator

bors commented Jul 8, 2021

☀️ Try build successful - checks-actions
Build commit: 8755dc80ab36003a2c81d2ef6e4cc4c81221d6f2 (8755dc80ab36003a2c81d2ef6e4cc4c81221d6f2)

@rust-timer
Copy link
Collaborator

Queued 8755dc80ab36003a2c81d2ef6e4cc4c81221d6f2 with parent d2b04f0, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (8755dc80ab36003a2c81d2ef6e4cc4c81221d6f2): comparison url.

Summary: This change led to significant improvements 🎉 in compiler performance.

  • Moderate improvement in instruction counts (up to -3.6% on full builds of hyper-2-check)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking 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 led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 8, 2021
@Aaron1011 Aaron1011 force-pushed the eval-proj-obligation branch from f8ac29b to f61df37 Compare July 8, 2021 22:46
@Aaron1011 Aaron1011 changed the title Try to evaluate projection obligations as well as trait obligations Improve caching during trait evaluation Jul 8, 2021
@Aaron1011
Copy link
Member Author

This is now ready for review.

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned nagisa Jul 8, 2021
@Aaron1011 Aaron1011 force-pushed the eval-proj-obligation branch from f61df37 to 2239ae7 Compare July 12, 2021 21:42
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 21, 2021

📌 Commit 2239ae7f5f9d879605f64833014c86eeeb6caec4 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 21, 2021
@bors
Copy link
Collaborator

bors commented Jul 21, 2021

⌛ Testing commit 2239ae7f5f9d879605f64833014c86eeeb6caec4 with merge 2466754cfcf5376eb61c757d7cabffe82d0587c6...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Collaborator

bors commented Jul 21, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 21, 2021
@Aaron1011
Improve caching during trait evaluation
3291218 
Previously, we would 'forget' that we had `'static` regions in some
place during trait evaluation. This lead to us producing
`EvaluatedToOkModuloRegions` when we could have produced
`EvaluatedToOk`, causing us to perform unnecessary work.

This PR preserves `'static` regions when we canonicalize a predicate for
`evaluate_obligation`, and when we 'freshen' a predicate during trait
evaluation. Thie ensures that evaluating a predicate containing
`'static` regions can produce `EvaluatedToOk` (assuming that we
don't end up introducing any region dependencies during evaluation).

Building off of this improved caching, we use
`predicate_must_hold_considering_regions` during fulfillment of
projection predicates to see if we can skip performing additional work.
We already do this for trait predicates, but doing this for projection
predicates lead to mixed performance results without the above caching
improvements.
@Aaron1011 Aaron1011 force-pushed the eval-proj-obligation branch from 2239ae7 to 3291218 Compare July 21, 2021 22:54
@Aaron1011
Copy link
Member Author

@bors r=nikomatsakis

@bors
Copy link
Collaborator

bors commented Jul 21, 2021

📌 Commit 3291218 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 21, 2021
@bors
Copy link
Collaborator

bors commented Jul 21, 2021

⌛ Testing commit 3291218 with merge 602150f...

@bors
Copy link
Collaborator

bors commented Jul 22, 2021

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 602150f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 22, 2021
@bors bors merged commit 602150f into rust-lang:master Jul 22, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikomatsakis nikomatsakis nikomatsakis approved these changes

Assignees

@nikomatsakis nikomatsakis

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.55.0
Development

Successfully merging this pull request may close these issues.
8 participants
@Aaron1011 @rust-highfive @rust-timer @bors @nikomatsakis @rust-log-analyzer @nagisa @rustbot