Reduce the amount of untracked state in TyCtxt -- Take 2

[cjgillot]

Main part of #85153

The offending line (#85153 (comment)) is replaced by a FIXME until the possible bug and the perf concern are both resolved.

r? @Aaron1011

[cjgillot]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit 609c07e5216722be1655706bb77620c4b20dfffd with merge c092198bf63a05c1b2a7a273c62471ced18e3d23...

[bors]

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

[rust-timer]

Queued c092198bf63a05c1b2a7a273c62471ced18e3d23 with parent 2f601ef, future comparison URL.

[rust-timer]

Finished benchmarking try commit (c092198bf63a05c1b2a7a273c62471ced18e3d23): 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

[cjgillot]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

1. git checkout qresolve (switch to your branch)
2. git fetch upstream master (retrieve the latest master)
3. git rebase upstream/master -p (rebase on top of it)
4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
5. git push self qresolve --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

Auto-merging compiler/rustc_middle/src/ty/print/pretty.rs
Auto-merging compiler/rustc_middle/src/ty/mod.rs
Auto-merging compiler/rustc_middle/src/ty/context.rs
CONFLICT (content): Merge conflict in compiler/rustc_middle/src/ty/context.rs
Auto-merging compiler/rustc_middle/src/query/mod.rs
Auto-merging compiler/rustc_metadata/src/rmeta/encoder.rs
Automatic merge failed; fix conflicts and then commit the result.

[cjgillot]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit 82d52a7d9e8f17d28a95794f1c5c4b0bec7ef01c with merge 4b9cc4f754d1397c1c39dcb1398f8d1d647bbf8d...

[bors]

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

[rust-timer]

Queued 4b9cc4f754d1397c1c39dcb1398f8d1d647bbf8d with parent 595088d, future comparison URL.

[rust-timer]

Finished benchmarking try commit (4b9cc4f754d1397c1c39dcb1398f8d1d647bbf8d): 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]

@cjgillot: Are you planning to work on improving the performance further, or do you think it's okay as-is? Seeing as the regressions are small, and this is correctness-related, I think either option would be fine.

[Aaron1011]

I think it would be good to enforce this in a follow-up PR - for example, moving the 'untracked' fields behind methods, which debug-assert that we're in an eval_always query.

[rust-timer]

Finished benchmarking try commit (3ee1084b77128fa4b2b0232a3ed1aec844cf55c6): 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]

The performance regressions are unfortunate - however, they're mostly small, and we were previously skipping necessary work.

@michaelwoerister Does this look good to you?

[bors]

☔ The latest upstream changes (presumably #86695) made this pull request unmergeable. Please resolve the merge conflicts.

[michaelwoerister]

I'll take a look shortly.

[bors]

☔ The latest upstream changes (presumably #86694) made this pull request unmergeable. Please resolve the merge conflicts.

[michaelwoerister]

Yes, this looks correct to me. At some point I'd like to come up with a better argument than

// Accessing the DefPath is ok, since it is part of DefPathHash.

You can either belief that or not :)

I'm wondering if this interacts with rust-lang/compiler-team#437 (cc @jyn514).

Regarding the performance regressions: removing the eval_always attribute from many of those smaller queries might affect performance because (IIRC) we don't add dependency edges to eval_always since they would never be used anyway. You could try adding back eval_always (with a comment that it's a performance optimization) and do another perf run to find out.

[jyn514]

@michaelwoerister It would actually make it a good deal easier I think - it makes everything a lot more granular instead of needing access to the full definitions every time.

Note that MCP still isn't accepted :/ and I'm not sure it will be until the resolver is tracked by incremental, which I think is still a long way off.

[Aaron1011]

r=me unless you want to try @michaelwoerister's suggestion of adding back eval_always.

[cjgillot]

r=me unless you want to try @michaelwoerister's suggestion of adding back eval_always.

Overall, I am more inclined to remove eval_always rather than adding them.
@bors r=Aaron1011

[bors]

📌 Commit 9f6d7e7 has been approved by Aaron1011

[bors]

⌛ Testing commit 9f6d7e7 with merge 7256855...

[bors]

☀️ Test successful - checks-actions
Approved by: Aaron1011
Pushing 7256855 to master...