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

Re-introduce concept of projection cache 'completion' #89831

Merged
merged 1 commit into from
Dec 19, 2021

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented Oct 12, 2021

Instead of clearing out the cache entirely, we store
the intermediate evaluation result into the cache entry.
This accomplishes several things:

  • We avoid the performance hit associated with re-evaluating
    the sub-obligations
  • We avoid causing issues with incremental compilation, since
    the final evaluation result is always the same
  • We avoid affecting other uses of the same InferCtxt which
    might care about 'side effects' from processing the sub-obligations
    (e,g. region constraints). Only code that is specifically aware
    of the new 'complete' code is affected

Authored to address issue #89195

@rust-highfive
Copy link
Collaborator

r? @wesleywiser

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

bors commented Oct 12, 2021

⌛ Trying commit 58de3037235c48765dcdfb6530cb316ac025ec5b with merge fa1210416b5caa99212b19d594ef3da8ae3132f1...

@bors
Copy link
Contributor

bors commented Oct 12, 2021

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

@rust-timer
Copy link
Collaborator

Queued fa1210416b5caa99212b19d594ef3da8ae3132f1 with parent 0446743, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (fa1210416b5caa99212b19d594ef3da8ae3132f1): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

  • Very large improvement in instruction counts (up to -6.0% on incr-unchanged builds of regression-31157)
  • Very large regression in instruction counts (up to 19.6% 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 Oct 13, 2021
@nayato
Copy link

nayato commented Oct 13, 2021

This fixes enormous regression we've seen introduced seemingly with #85868.
Debug build before before #85868: 3 min
Same build after #85868: 12+ h - at which point we don't wait for it anymore and abort it.
Same build with this PR: 3.5 min

@Aaron1011
Copy link
Member Author

@nayato: I'm glad to hear it! I came up with this as a fix for #89195, which appears to have the same underlying cause as your issue.

Unfortunately, part of the issue with #89195 seems to be be the 'eager' evaluation and discarding logic that I added in #85868 (and that I've now removed from this PR). When there are a large number of projections with a large number of sub-obligations, this appears to blow up compilation time. Unfortunately, removing that logic appears to have a large negative impact on the deeply-nested benchmark.

@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 Oct 13, 2021
@bors
Copy link
Contributor

bors commented Oct 13, 2021

⌛ Trying commit 6971dea4036a802fe9d7a838b8d3e655fee7d70f with merge 38dc5847c0733779aaaa9e3de4408165ea7a2b05...

@jackh726
Copy link
Member

Perf aside here, I would have extreme reservations about landing this PR without proper tests for the original incremental issues that required us to remove a lot of the caching code.

@bors
Copy link
Contributor

bors commented Oct 13, 2021

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

@rust-timer
Copy link
Collaborator

Queued 38dc5847c0733779aaaa9e3de4408165ea7a2b05 with parent 81117ff, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (38dc5847c0733779aaaa9e3de4408165ea7a2b05): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

  • Very large improvement in instruction counts (up to -6.0% on incr-unchanged builds of regression-31157)
  • Very large regression in instruction counts (up to 19.8% 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 Oct 13, 2021
@wesleywiser

This comment has been minimized.

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 14, 2021
@Aaron1011

This comment has been minimized.

@wesleywiser

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Dec 18, 2021

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

@rust-timer
Copy link
Collaborator

Queued 02aeffa72b11c0501c63a17d3e3f27c833ef2cbf with parent d3848cb, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (02aeffa72b11c0501c63a17d3e3f27c833ef2cbf): comparison url.

Summary: This change led to small relevant mixed results 🤷 in compiler performance.

  • Small improvement in instruction counts (up to -1.2% on full builds of regression-31157)
  • Small regression in instruction counts (up to 0.5% on incr-unchanged builds of webrender-wrench)

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 Dec 18, 2021
Instead of clearing out the cache entirely, we store
the intermediate evaluation result into the cache entry.
This accomplishes several things:

* We avoid the performance hit associated with re-evaluating
  the sub-obligations
* We avoid causing issues with incremental compilation, since
  the final evaluation result is always the same
* We avoid affecting other uses of the same `InferCtxt` which
  might care about 'side effects' from processing the sub-obligations
  (e,g. region constraints). Only code that is specifically aware
   of the new 'complete' code is affected
@Aaron1011
Copy link
Member Author

@bors r=jackh726

@bors
Copy link
Contributor

bors commented Dec 19, 2021

📌 Commit 40ef1d3 has been approved by jackh726

@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. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 19, 2021
@bors
Copy link
Contributor

bors commented Dec 19, 2021

⌛ Testing commit 40ef1d3 with merge d6cffe4...

@bors
Copy link
Contributor

bors commented Dec 19, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 19, 2021
@bors bors merged commit d6cffe4 into rust-lang:master Dec 19, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 19, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d6cffe4): comparison url.

Summary: This benchmark run did not return any relevant changes.

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

@rustbot label: -perf-regression

@matze
Copy link

matze commented Dec 21, 2021

Any chance this can make it into 1.58? It fixes severe build times we have since 1.56 in a tonic/tower-based application of ours and essentially holding us back from upgrading to edition 2021 and a more recent rustls.

@jackh726
Copy link
Member

jackh726 commented Dec 21, 2021

I'll nominate this for stable and beta.

Personally, I think is at least as risky as #90423, and that got stable declined. But a beta backport might be okay here. I'll nominate both for discussion.

@jackh726 jackh726 added beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Dec 21, 2021
@pnkfelix
Copy link
Member

Discussed in T-compiler meeting. Deferring decision on whether to backport (any of beta/stable) until next week, at least.

@apiraino
Copy link
Contributor

Discussed again today and the decision is to not baclport this on both beta or stable channels Zulip topic

@rustbot label -beta-nominated -stable-nominated

@rustbot rustbot removed beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Dec 30, 2021
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. 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.