Skip to content
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

Use a fresh InferCtxt when we 'speculatively' evaluate predicates #91183

Closed
wants to merge 2 commits into from

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented Nov 24, 2021

The existing InferCtxt may already have in-progress projections
for some of the predicates we may (recursively evaluate). This can
cause us to incorrect produce (and cache!) EvaluatedToRecur, leading
us to incorrectly mark a predicate as unimplemented.

We now create a fresh InferCtxt for each sub-obligation that we
'speculatively' evaluate. This causes us to miss out on some
legitimate caching opportunities, but ensures that our speculative
evaluation cannot pollute any of the caches from the 'real' InferCtxt.
The evaluation can still update global caches, but our global caches
don't have any notion of 'in progress', so this is fine.

Fixes #90662

@rust-highfive
Copy link
Collaborator

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 Nov 24, 2021
@Aaron1011
Copy link
Member Author

I'm hoping to remove this 'speculative' evaluation entirely in #89831. However, that PR is blocked on several other PRs. In the meantime, this PR will make the existing logic more correct, and allow affected users to update to the latest nightly.

@jackh726
Copy link
Member

LGTM. r=me with CI green and a comment added on why we use a fresh InferCtxt

Maybe we should also do a perf run just in case.

@jackh726
Copy link
Member

r? @jackh726

@rust-highfive rust-highfive assigned jackh726 and unassigned nagisa Nov 24, 2021
@rust-log-analyzer

This comment has been minimized.

@camelid
Copy link
Member

camelid commented Nov 24, 2021

@bors rollup=never (for perf reasons)

@Aaron1011
Copy link
Member Author

Blocked on #91205

@camelid camelid added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 25, 2021
The existing `InferCtxt` may already have in-progress projections
for some of the predicates we may (recursively evaluate). This can
cause us to incorrect produce (and cache!) `EvaluatedToRecur`, leading
us to incorrectly mark a predicate as unimplemented.

We now create a fresh `InferCtxt` for each sub-obligation that we
'speculatively' evaluate. This causes us to miss out on some
legitimate caching opportunities, but ensures that our speculative
evaluation cannot pollute any of the caches from the 'real' `InferCtxt`.
The evaluation can still update *global* caches, but our global caches
don't have any notion of 'in progress', so this is fine.

Fixes rust-lang#90662
@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 Nov 26, 2021
@bors
Copy link
Contributor

bors commented Nov 26, 2021

⌛ Trying commit 04964d7 with merge 267787aa5ef8c1a8030445f1baded8c450b61d2a...

@bors
Copy link
Contributor

bors commented Nov 26, 2021

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

@rust-timer
Copy link
Collaborator

Queued 267787aa5ef8c1a8030445f1baded8c450b61d2a with parent 1e79d79, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (267787aa5ef8c1a8030445f1baded8c450b61d2a): comparison url.

Summary: This change led to very large relevant regressions 😿 in compiler performance.

  • Very large regression in instruction counts (up to 7.0% on incr-unchanged builds of deeply-nested)

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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 26, 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 Nov 27, 2021
@bors
Copy link
Contributor

bors commented Nov 27, 2021

⌛ Trying commit 931a3a6 with merge 48ed0538da3299e6f9820d237251c584e3cf3546...

@bors
Copy link
Contributor

bors commented Nov 27, 2021

💥 Test timed out

@Aaron1011
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Nov 28, 2021

⌛ Trying commit 931a3a6 with merge ce10115f5e07e18e55f328a29a2c47fab24e1338...

@bors
Copy link
Contributor

bors commented Nov 28, 2021

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

@rust-timer
Copy link
Collaborator

Queued ce10115f5e07e18e55f328a29a2c47fab24e1338 with parent 58f9efd, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ce10115f5e07e18e55f328a29a2c47fab24e1338): comparison url.

Summary: This change led to very large relevant regressions 😿 in compiler performance.

  • Very large regression in instruction counts (up to 6.4% on incr-unchanged builds of deeply-nested)

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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@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 Nov 28, 2021
@Aaron1011
Copy link
Member Author

Aaron1011 commented Nov 28, 2021

After some further thought, I think this strategy is insufficient to complete fix the issue. We could still end up hitting a spurious ProjectionCacheEntry::InProgress, causing us to cache an evaluation error in the global tcx.evaluation_cache.

I think we need to avoid speculative evaluation entirely. PR #89831 remove speculative evaluation (among other things), but it causes a significant performance hit. Since this bug is currently preventing code from compiling, we might want to accept the performance hit for the time being.

@jackh726
Copy link
Member

Closing since this is getting removed anyways

@jackh726 jackh726 closed this Dec 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in 1.56.0 - Trait bound not satisfied error even though the impl exists
9 participants