Re-introduce concept of projection cache 'completion'

[Aaron1011]

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]

r? @wesleywiser

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

[Aaron1011]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit 58de3037235c48765dcdfb6530cb316ac025ec5b with merge fa1210416b5caa99212b19d594ef3da8ae3132f1...

[bors]

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

[rust-timer]

Queued fa1210416b5caa99212b19d594ef3da8ae3132f1 with parent 0446743, future comparison URL.

[rust-timer]

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

[nayato]

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]

@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]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

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

[jackh726]

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]

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

[rust-timer]

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

[rust-timer]

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

[bors]

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

[rust-timer]

Queued 02aeffa72b11c0501c63a17d3e3f27c833ef2cbf with parent d3848cb, future comparison URL.

[rust-timer]

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

[Aaron1011]

@bors r=jackh726

[bors]

📌 Commit 40ef1d3 has been approved by jackh726

[bors]

⌛ Testing commit 40ef1d3 with merge d6cffe4...

[bors]

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

[rust-timer]

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]

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]

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.

[pnkfelix]

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

[apiraino]

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