Refactor fingerprint reconstruction #89343

Mark-Simulacrum · 2021-09-28T23:24:02Z

This PR replaces can_reconstruct_query_key with fingerprint_style, which returns the style of the fingerprint for that query. This allows us to avoid trying to extract a DefId (or equivalent) from keys which are reconstructible because they're () but not as DefIds.

This is done with the goal of fixing -Zdump-dep-graph, which seems to have broken a while ago (I didn't try to bisect). Currently even on a fn main() {} file it'll ICE (you need to also pass -Zquery-dep-graph for it to work at all), and this patch indirectly fixes the cause of that ICE. This also adds a test for it continuing to work.

rust-highfive · 2021-09-28T23:24:05Z

r? @matthewjasper

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

Mark-Simulacrum · 2021-09-28T23:25:01Z

cc @rust-lang/wg-incr-comp -- I'm not sure if folks recall the intent of has_params, which might be helpful to determine if this is the "right" thing to do.

compiler/rustc_middle/src/ty/query.rs

cjgillot · 2021-09-29T17:16:58Z

IIUC, the crash happens when trying to debug-print a DepNode whch parameter () by mistakenly trying to extract a DefId from a Fingerprint which is not a proper DefPathHash. The bug is therefore in rustc_middle::dep_graph::dep_node::DepNodeExt::extract_def_id which tests can_reconstruct_query_key() to know whether the query key is a CrateNum/LocalDefId/DefId. I think this PR hides the problem rather than solving it. The correct fix would be to create a separate has_def_path_hash field in DepKindStruct which tests whether the query key is any of the three, and equivalently if a DefId can be extracted from the fingerprint.

Mark-Simulacrum · 2021-09-29T17:22:06Z

Hm, that seems like a good thing to tackle, I agree. I think this PR is still desirable (or we should just remove has_params), since it seems like the meaning of that is not too clear right now.

I'll have to dig into the 2 manually declared queries with has_params: false, and see if that's significant for them...

Mark-Simulacrum · 2021-10-02T16:44:31Z

Alright, replaced pretty much the whole PR with different patch -- and extracted #89466 for the base macro changes which are not actually needed for this PR now.

I looked into fully removing has_params but didn't end up making that change. The only uses left are the debug assert in DepKind::new_no_params and the debug_node method. The latter could be removed with no loss, I believe -- just a difference in how nodes look (printed as foo today, foo() with that condition dropped). Since has_params is true currently for all but Null and TraitSelect DepKinds (neither of which is a proper query), it's probably the case that this could be refactored. But seems like future work.

cjgillot · 2021-10-02T16:47:54Z

compiler/rustc_middle/src/dep_graph/dep_node.rs

+        match kind.fingerprint_style() {
+            FingerprintStyle::Opaque => Err(()),
+            FingerprintStyle::Unit => {
+                if !kind.has_params {


FingerprintStyle::Unit should now be equivalent to has_params == false, isn't it? We should be able to get rid of has_params.

Not currently. A query with () key currently "has_params"; the only two things to not have params are Null and TraitSelect which are not generated through the normal macro infrastructure. I dropped the "fix" for that from this branch for now, though I can re-add it, it's not big (b2b53bb9a4f2f5e1f96f8f01ad7b378f5eb1b4d1).

cjgillot · 2021-10-02T22:12:04Z

@bors r+

bors · 2021-10-02T22:12:05Z

📌 Commit 63aaf88 has been approved by cjgillot

…cjgillot Refactor fingerprint reconstruction This PR replaces can_reconstruct_query_key with fingerprint_style, which returns the style of the fingerprint for that query. This allows us to avoid trying to extract a DefId (or equivalent) from keys which *are* reconstructible because they're () but not as DefIds. This is done with the goal of fixing -Zdump-dep-graph, which seems to have broken a while ago (I didn't try to bisect). Currently even on a `fn main() {}` file it'll ICE (you need to also pass -Zquery-dep-graph for it to work at all), and this patch indirectly fixes the cause of that ICE. This also adds a test for it continuing to work.

bors · 2021-10-05T13:19:02Z

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

Keys can be reconstructed from fingerprints that are not DefPathHash, but then we cannot extract a DefId from them.

Mark-Simulacrum · 2021-10-07T02:23:19Z

@bors r=cjgillot rollup=iffy

bors · 2021-10-07T02:23:20Z

📌 Commit 85c4fd81799de01e8eb3a84f96a13baafc8de407 has been approved by cjgillot

bors · 2021-10-09T09:41:05Z

⌛ Testing commit 85c4fd81799de01e8eb3a84f96a13baafc8de407 with merge 8f2565b630f71928671276797f4659fcc679d795...

bors · 2021-10-09T10:00:50Z

💔 Test failed - checks-actions

Mark-Simulacrum · 2021-10-09T12:14:15Z

@bors r=cjgillot rollup=iffy

bors · 2021-10-09T12:14:17Z

📌 Commit 415a9a2 has been approved by cjgillot

bors · 2021-10-09T13:13:10Z

⌛ Testing commit 415a9a2 with merge 15491d7...

bors · 2021-10-09T16:01:24Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 15491d7 to master...

rust-timer · 2021-10-09T17:34:13Z

Finished benchmarking commit (15491d7): 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

Debug logging during incremental compilation had been broken for some time, until rust-lang#89343 fixed it (among other things). Add a test so this is less likely to break without being noticed. This test is nearly a copy of the `src/test/ui/rustc-rust-log.rs` test, but tests debug logging in the incremental compliation code paths.

…t, r=Mark-Simulacrum Add test for debug logging during incremental compilation Debug logging during incremental compilation had been broken for some time, until rust-lang#89343 fixed it (among other things). Add a test so this is less likely to break without being noticed. This test is nearly a copy of the `src/test/ui/rustc-rust-log.rs` test, but tests debug logging in the incremental compliation code paths.

rust-highfive assigned matthewjasper Sep 28, 2021

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

Mark-Simulacrum commented Sep 28, 2021

View reviewed changes

compiler/rustc_middle/src/ty/query.rs Outdated Show resolved Hide resolved

cjgillot self-assigned this Sep 29, 2021

Mark-Simulacrum unassigned matthewjasper Sep 29, 2021

cjgillot 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 Sep 29, 2021

Mark-Simulacrum force-pushed the no-args-queries branch from b2b53bb to 0d1ff33 Compare October 2, 2021 16:22

Mark-Simulacrum changed the title ~~Fix -Zdump-dep-graph~~ Refactor fingerprint reconstruction Oct 2, 2021

Mark-Simulacrum force-pushed the no-args-queries branch 2 times, most recently from 175631d to 3d73c4d Compare October 2, 2021 16:31

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

This comment has been minimized.

Sign in to view

Mark-Simulacrum force-pushed the no-args-queries branch 2 times, most recently from 9173a20 to 63aaf88 Compare October 2, 2021 16:44

cjgillot reviewed Oct 2, 2021

View reviewed changes

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 Oct 2, 2021

Mark-Simulacrum mentioned this pull request Oct 3, 2021

Add dep-graph collector rust-lang/rustc-perf#1046

Merged

Manishearth mentioned this pull request Oct 3, 2021

Rollup of 6 pull requests #89481

Closed

Query the fingerprint style during key reconstruction

6f78eed

Keys can be reconstructed from fingerprints that are not DefPathHash, but then we cannot extract a DefId from them.

Mark-Simulacrum force-pushed the no-args-queries branch from 63aaf88 to 85c4fd8 Compare October 7, 2021 02:21

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 7, 2021

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 9, 2021

Add a test that -Zquery-dep-graph -Zdump-dep-graph works

415a9a2

Mark-Simulacrum force-pushed the no-args-queries branch from 85c4fd8 to 415a9a2 Compare October 9, 2021 12:14

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 Oct 9, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 9, 2021

bors merged commit 15491d7 into rust-lang:master Oct 9, 2021

rustbot added this to the 1.57.0 milestone Oct 9, 2021

Mark-Simulacrum deleted the no-args-queries branch October 9, 2021 17:02

tgnottingham mentioned this pull request Oct 18, 2021

Add test for debug logging during incremental compilation #90029

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor fingerprint reconstruction #89343

Refactor fingerprint reconstruction #89343

Mark-Simulacrum commented Sep 28, 2021 •

edited

Loading

rust-highfive commented Sep 28, 2021

Mark-Simulacrum commented Sep 28, 2021

cjgillot commented Sep 29, 2021

Mark-Simulacrum commented Sep 29, 2021

This comment has been minimized.

Mark-Simulacrum commented Oct 2, 2021

cjgillot Oct 2, 2021

Mark-Simulacrum Oct 2, 2021 •

edited

Loading

cjgillot commented Oct 2, 2021

bors commented Oct 2, 2021

bors commented Oct 5, 2021

Mark-Simulacrum commented Oct 7, 2021

bors commented Oct 7, 2021

bors commented Oct 9, 2021

This comment has been minimized.

bors commented Oct 9, 2021

Mark-Simulacrum commented Oct 9, 2021

bors commented Oct 9, 2021

bors commented Oct 9, 2021

bors commented Oct 9, 2021

rust-timer commented Oct 9, 2021

Refactor fingerprint reconstruction #89343

Refactor fingerprint reconstruction #89343

Conversation

Mark-Simulacrum commented Sep 28, 2021 • edited Loading

rust-highfive commented Sep 28, 2021

Mark-Simulacrum commented Sep 28, 2021

cjgillot commented Sep 29, 2021

Mark-Simulacrum commented Sep 29, 2021

This comment has been minimized.

Mark-Simulacrum commented Oct 2, 2021

cjgillot Oct 2, 2021

Choose a reason for hiding this comment

Mark-Simulacrum Oct 2, 2021 • edited Loading

Choose a reason for hiding this comment

cjgillot commented Oct 2, 2021

bors commented Oct 2, 2021

bors commented Oct 5, 2021

Mark-Simulacrum commented Oct 7, 2021

bors commented Oct 7, 2021

bors commented Oct 9, 2021

This comment has been minimized.

bors commented Oct 9, 2021

Mark-Simulacrum commented Oct 9, 2021

bors commented Oct 9, 2021

bors commented Oct 9, 2021

bors commented Oct 9, 2021

rust-timer commented Oct 9, 2021

Mark-Simulacrum commented Sep 28, 2021 •

edited

Loading

Mark-Simulacrum Oct 2, 2021 •

edited

Loading