Remove concept of 'completion' from the projection cache

[Aaron1011]

Fixes #88910

When we initially store a NormalizedTy in the projection cache,
we discard all obligations that we can (while ensuring that we
don't cause any issues with incremental compilation).

Marking a projection cache entry as 'completed' discards all
obligations associated with it. This can only cause problems,
since any obligations stored in the cache are there for a reason
(e.g. they evaluate to EvaluatedToOkModuloRegions).

This commit removes complete and complete_normalized entirely.

[rust-highfive]

r? @wesleywiser

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

[Aaron1011]

I've verified locally that this fixes #88910. However, the substrate-node-template project is enormous (it has 834 dependencies, and the crate itself takes a few minutes to compile on my machine), so I haven't attempted a minimization yet.

This PR can only ever cause us to preserve more sub-obligations in the cache, so this can't cause any correctness issues.

[Aaron1011]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit 055651d with merge aefb79dfee71c0899b706198d9d9f4ca30ca395d...

[bors]

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

[rust-timer]

Queued aefb79dfee71c0899b706198d9d9f4ca30ca395d with parent c3c0f80, future comparison URL.

[rust-timer]

Finished benchmarking commit (aefb79dfee71c0899b706198d9d9f4ca30ca395d): comparison url.

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

• Very small improvement in instruction counts (up to -0.3% on incr-patched: println builds of deep-vector)
• Moderate regression in instruction counts (up to 0.9% on full builds of wg-grammar)

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

[wesleywiser]

The reasoning and code look good to me and the perf regression seems acceptable since this closes an incremental issue, but I would like to make sure wg-traits is aware this is happening. r=me with an acknowledgement from them.

cc @nikomatsakis @jackh726

[jackh726]

This looks fine to me; these obligations can get cached in the fulfill code anyways (which is probably why there isn't a big regression).

Though I don't think this change in particular will cause any regressions, I do just want to register a slight worry, particularly seeing issues like #88969 pop up. I suppose this mostly just stems from the fact that we still don't have a test for #85868. The more we mess with this code, the more I'd really like a regression test.

[jackh726]

@bors r=wesleywiser,jackh726

[bors]

📌 Commit 055651d has been approved by wesleywiser,jackh726

[bors]

⌛ Testing commit 055651d with merge e0c38af...

[bors]

☀️ Test successful - checks-actions
Approved by: wesleywiser,jackh726
Pushing e0c38af to master...

[rust-timer]

Finished benchmarking commit (e0c38af): comparison url.

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

• Small improvement in instruction counts (up to -0.4% on incr-full builds of ctfe-stress-4)
• Moderate regression in instruction counts (up to 0.8% on full builds of wg-grammar)

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

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

[pnkfelix]

Visited for weekly performance triage. All wg-grammar-* consistently regressed by about 0.75% (both full and incr-full). The builds for regression-31157-check regressed by about 0.36%, which isn't as bad.

Under our new significance metrics, the ctfe-stress-4 changes are not considered signficant.

Seems like something might be worth looking into here. (But did not hit very many benchmarks, so time might be better spent elsewhere. Definitely don't spend too much time on it.)