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

Preserve most sub-obligations in the projection cache #85868

Merged
merged 2 commits into from
Sep 2, 2021

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented May 31, 2021

Fixes #85360

When we evaluate a projection predicate, we may produce sub-obligations. During trait evaluation, evaluating these sub-obligations might cause us to produce EvaluatedToOkModuloRegions.

When we cache the result of projection in our projection cache, we try to throw away some of the sub-obligations, so that we don't need to re-evaluate/process them the next time we need to perform this particular projection. However, we may end up throwing away predicates that will (recursively) evaluate to EvaluatedToOkModuloRegions. If we do, then the result of evaluating a predicate will depend on the state of the predicate cache - this is global untracked state, which interacts badly with incremental compilation.

To fix this, we now only discard global predicates that evaluate to EvaluatedToOk. This ensures that any predicates that (may) evaluate to EvaluatedToOkModuloRegions are kept in the cache, and influence the results of any queries which perform this projection.

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(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 May 31, 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 May 31, 2021
@bors
Copy link
Contributor

bors commented May 31, 2021

⌛ Trying commit a3a287b0bfe095e1fd9bd4292a0e94f738a94cce with merge fa0462d22926332548181c1cd602c85dbb20fec8...

@bors
Copy link
Contributor

bors commented May 31, 2021

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

@rust-timer
Copy link
Collaborator

Queued fa0462d22926332548181c1cd602c85dbb20fec8 with parent 6a3dce9, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (fa0462d22926332548181c1cd602c85dbb20fec8): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

@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 Jun 25, 2021
@bors
Copy link
Contributor

bors commented Jun 25, 2021

⌛ Trying commit a3a287b0bfe095e1fd9bd4292a0e94f738a94cce with merge d246c813ecc8de89cc5a05b9479dac8195a3e251...

@bors
Copy link
Contributor

bors commented Jun 25, 2021

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

@rust-timer
Copy link
Collaborator

Queued d246c813ecc8de89cc5a05b9479dac8195a3e251 with parent d4e7cb3, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (d246c813ecc8de89cc5a05b9479dac8195a3e251): comparison url.

Summary: This change led to significant regressions 😿 in compiler performance.

  • Very large regression in instruction counts (up to 21.4% on full builds of deeply-nested-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. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

@rustbot
Copy link
Collaborator

rustbot commented Jun 25, 2021

Error: Label perf-regression can only be set by Rust team members

Please let @rust-lang/release know if you're having trouble with this bot.

@JohnTitor JohnTitor removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 28, 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 12, 2021
@bors
Copy link
Contributor

bors commented Jul 12, 2021

⌛ Trying commit fbfe5baaf5d5faff7450209c02a699ad25948230 with merge 7d3788b67103a424bf6612495afedb80c4fe8b10...

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 2, 2021
@bors
Copy link
Contributor

bors commented Sep 2, 2021

⌛ Testing commit 611191f with merge 371f3cd...

@bors
Copy link
Contributor

bors commented Sep 2, 2021

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 371f3cd to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 2, 2021
@bors bors merged commit 371f3cd into rust-lang:master Sep 2, 2021
@rustbot rustbot added this to the 1.56.0 milestone Sep 2, 2021
@pnkfelix
Copy link
Member

pnkfelix commented Sep 8, 2021

@nikomatsakis: This PR is now ready for review.

This performance is worse than the two other approaches (#86896 and #86871), but this is less hacky, and more clearly correct.

As expected, this was flagged as causing mixed performance changes in instruction counts during weekly performance triage. Since these changes in performance were anticipating and discussed above, I'm marking as already-triaged.

Update: whoops, it was already marked accordingly. Sorry for the noise!

@apiraino
Copy link
Contributor

apiraino commented Sep 9, 2021

It's already in current beta (Zulip comment)

@rustbot label -beta-nominated

@rustbot rustbot removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 9, 2021
@jackh726
Copy link
Member

jackh726 commented Sep 9, 2021

Do we want to nominate for stable? How concerned are we about this incremental issue? @rust-lang/compiler

wesleywiser added a commit to wesleywiser/rust that referenced this pull request Dec 4, 2021
Adds the minimial repro test case from rust-lang#85360. The fix for rust-lang#85360 was
supposed to be rust-lang#85868 however the repro was resolved in the 2021-07-05
nightly while rust-lang#85360 didn't land until 2021-09-03. The reason for that
is d34a3a4 **also** resolves that
issue.

To test if rust-lang#85868 actually fixes rust-lang#85360, I reverted
d34a3a4 and found that rust-lang#85868 does
indeed resolve rust-lang#85360.

With that question resolved, add a test case to our incremental test
suite for the original Ok(EvaluatedToOkModuloRegions) ICE.

Thanks to @lqd for helping track this down!
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 6, 2021
Add test for evaluate_obligation: Ok(EvaluatedToOkModuloRegions) ICE

Adds the minimial repro test case from rust-lang#85360. The fix for rust-lang#85360 was
supposed to be rust-lang#85868 however the repro was resolved in the 2021-07-05
nightly while rust-lang#85868 didn't land until 2021-09-03. The reason for that
is d34a3a4 **also** resolves that
issue.

To test if rust-lang#85868 actually fixes rust-lang#85360, I reverted
d34a3a4 and found that rust-lang#85868 does
indeed resolve rust-lang#85360.

With that question resolved, add a test case to our incremental test
suite for the original Ok(EvaluatedToOkModuloRegions) ICE.

Thanks to `@lqd` for helping track this down!
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 6, 2021
Add test for evaluate_obligation: Ok(EvaluatedToOkModuloRegions) ICE

Adds the minimial repro test case from rust-lang#85360. The fix for rust-lang#85360 was
supposed to be rust-lang#85868 however the repro was resolved in the 2021-07-05
nightly while rust-lang#85868 didn't land until 2021-09-03. The reason for that
is d34a3a4 **also** resolves that
issue.

To test if rust-lang#85868 actually fixes rust-lang#85360, I reverted
d34a3a4 and found that rust-lang#85868 does
indeed resolve rust-lang#85360.

With that question resolved, add a test case to our incremental test
suite for the original Ok(EvaluatedToOkModuloRegions) ICE.

Thanks to ``@lqd`` for helping track this down!
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 7, 2021
Add test for evaluate_obligation: Ok(EvaluatedToOkModuloRegions) ICE

Adds the minimial repro test case from rust-lang#85360. The fix for rust-lang#85360 was
supposed to be rust-lang#85868 however the repro was resolved in the 2021-07-05
nightly while rust-lang#85868 didn't land until 2021-09-03. The reason for that
is d34a3a4 **also** resolves that
issue.

To test if rust-lang#85868 actually fixes rust-lang#85360, I reverted
d34a3a4 and found that rust-lang#85868 does
indeed resolve rust-lang#85360.

With that question resolved, add a test case to our incremental test
suite for the original Ok(EvaluatedToOkModuloRegions) ICE.

Thanks to ```@lqd``` for helping track this down!
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 7, 2021
Add test for evaluate_obligation: Ok(EvaluatedToOkModuloRegions) ICE

Adds the minimial repro test case from rust-lang#85360. The fix for rust-lang#85360 was
supposed to be rust-lang#85868 however the repro was resolved in the 2021-07-05
nightly while rust-lang#85868 didn't land until 2021-09-03. The reason for that
is d34a3a4 **also** resolves that
issue.

To test if rust-lang#85868 actually fixes rust-lang#85360, I reverted
d34a3a4 and found that rust-lang#85868 does
indeed resolve rust-lang#85360.

With that question resolved, add a test case to our incremental test
suite for the original Ok(EvaluatedToOkModuloRegions) ICE.

Thanks to ````@lqd```` for helping track this down!
Mark-Simulacrum pushed a commit to Mark-Simulacrum/rust that referenced this pull request Jan 5, 2022
Adds the minimial repro test case from rust-lang#85360. The fix for rust-lang#85360 was
supposed to be rust-lang#85868 however the repro was resolved in the 2021-07-05
nightly while rust-lang#85360 didn't land until 2021-09-03. The reason for that
is d34a3a4 **also** resolves that
issue.

To test if rust-lang#85868 actually fixes rust-lang#85360, I reverted
d34a3a4 and found that rust-lang#85868 does
indeed resolve rust-lang#85360.

With that question resolved, add a test case to our incremental test
suite for the original Ok(EvaluatedToOkModuloRegions) ICE.

Thanks to @lqd for helping track this down!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

Incremental compilation error with evaluate_obligation: Ok(EvaluatedToOkModuloRegions)