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

cjgillot · 2021-06-02T19:02:22Z

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 · 2021-06-02T19:02:40Z

@bors try @rust-timer queue

rust-timer · 2021-06-02T19:02:42Z

Awaiting bors try build completion.

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

bors · 2021-06-02T19:02:49Z

⌛ Trying commit 609c07e5216722be1655706bb77620c4b20dfffd with merge c092198bf63a05c1b2a7a273c62471ced18e3d23...

bors · 2021-06-02T19:49:58Z

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

rust-timer · 2021-06-02T19:50:00Z

Queued c092198bf63a05c1b2a7a273c62471ced18e3d23 with parent 2f601ef, future comparison URL.

rust-timer · 2021-06-02T22:32:40Z

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 · 2021-06-04T19:18:04Z

@bors try @rust-timer queue

rust-timer · 2021-06-04T19:18:05Z

Awaiting bors try build completion.

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

bors · 2021-06-04T19:18:10Z

🔒 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:

git checkout qresolve (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
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 · 2021-06-04T19:26:11Z

@bors try @rust-timer queue

rust-timer · 2021-06-04T19:26:13Z

Awaiting bors try build completion.

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

bors · 2021-06-04T19:26:21Z

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

bors · 2021-06-04T20:05:47Z

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

rust-timer · 2021-06-04T20:05:49Z

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

rust-timer · 2021-06-04T22:00:38Z

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 · 2021-06-05T17:48:20Z

@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 · 2021-06-05T17:50:35Z

compiler/rustc_middle/src/hir/map/mod.rs

@@ -932,9 +940,15 @@ impl<'hir> intravisit::Map<'hir> for Map<'hir> {
 pub(super) fn index_hir<'tcx>(tcx: TyCtxt<'tcx>, (): ()) -> &'tcx IndexedHir<'tcx> {
    let _prof_timer = tcx.sess.prof.generic_activity("build_hir_map");

+    // We can access untracked state since we are an eval_always query.


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 · 2021-06-20T12:39:50Z

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 · 2021-06-20T20:09:30Z

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 · 2021-06-30T16:50:36Z

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

michaelwoerister · 2021-07-05T09:24:50Z

I'll take a look shortly.

bors · 2021-07-06T11:46:21Z

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

michaelwoerister · 2021-07-06T12:14:54Z

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 · 2021-07-06T12:45:54Z

@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 · 2021-07-06T19:48:21Z

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

cjgillot · 2021-07-11T15:37:03Z

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 · 2021-07-11T15:37:04Z

📌 Commit 9f6d7e7 has been approved by Aaron1011

bors · 2021-07-11T16:09:23Z

⌛ Testing commit 9f6d7e7 with merge 7256855...

bors · 2021-07-11T18:49:56Z

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

rust-highfive assigned Aaron1011 Jun 2, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 2, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 2, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 2, 2021

Aaron1011 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 3, 2021

cjgillot force-pushed the qresolve branch from 609c07e to 70de004 Compare June 4, 2021 19:14

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 4, 2021

cjgillot force-pushed the qresolve branch from 70de004 to 82d52a7 Compare June 4, 2021 19:25

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jun 4, 2021

Aaron1011 reviewed Jun 5, 2021

View reviewed changes

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jun 20, 2021

cjgillot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 22, 2021

cjgillot force-pushed the qresolve branch from 592ee34 to 1b1d0ca Compare June 30, 2021 06:17

Aaron1011 assigned michaelwoerister Jul 1, 2021

cjgillot force-pushed the qresolve branch from 1b1d0ca to dc6a8ee Compare July 3, 2021 09:41

cjgillot added 2 commits July 6, 2021 19:22

Make resolutions a query.

071a047

Correct comments about untracked accesses.

9f6d7e7

cjgillot force-pushed the qresolve branch from dc6a8ee to 9f6d7e7 Compare July 6, 2021 18:51

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. labels Jul 11, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 11, 2021

bors merged commit 7256855 into rust-lang:master Jul 11, 2021

rustbot added this to the 1.55.0 milestone Jul 11, 2021

This was referenced Jul 11, 2021

Encode spans relative to the enclosing item #84373

Merged

Encode spans relative to the enclosing item -- enable on nightly #84762

Merged

Move OnDiskCache to rustc_query_impl. #86698

Merged

cjgillot deleted the qresolve branch July 11, 2021 19:15

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

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

Uh oh!

Conversation

cjgillot commented Jun 2, 2021

Uh oh!

cjgillot commented Jun 2, 2021

Uh oh!

rust-timer commented Jun 2, 2021

Uh oh!

bors commented Jun 2, 2021

Uh oh!

bors commented Jun 2, 2021

Uh oh!

rust-timer commented Jun 2, 2021

Uh oh!

rust-timer commented Jun 2, 2021

Uh oh!

cjgillot commented Jun 4, 2021

Uh oh!

rust-timer commented Jun 4, 2021

Uh oh!

bors commented Jun 4, 2021

Uh oh!

cjgillot commented Jun 4, 2021

Uh oh!

rust-timer commented Jun 4, 2021

Uh oh!

bors commented Jun 4, 2021

Uh oh!

bors commented Jun 4, 2021

Uh oh!

rust-timer commented Jun 4, 2021

Uh oh!

rust-timer commented Jun 4, 2021

Uh oh!

Aaron1011 commented Jun 5, 2021

Uh oh!

Aaron1011 Jun 5, 2021

Choose a reason for hiding this comment

Uh oh!

rust-timer commented Jun 20, 2021

Uh oh!

Aaron1011 commented Jun 20, 2021

Uh oh!

bors commented Jun 30, 2021

Uh oh!

michaelwoerister commented Jul 5, 2021

Uh oh!

bors commented Jul 6, 2021

Uh oh!

michaelwoerister commented Jul 6, 2021

Uh oh!

jyn514 commented Jul 6, 2021

Uh oh!

Aaron1011 commented Jul 6, 2021

Uh oh!

cjgillot commented Jul 11, 2021

Uh oh!

bors commented Jul 11, 2021

Uh oh!

bors commented Jul 11, 2021

Uh oh!

bors commented Jul 11, 2021

Uh oh!

Uh oh!