Don't perform any new queries while reading a query result on disk #91919

Aaron1011 · 2021-12-14T16:15:51Z

In addition to being very confusing, this can cause us to add dep node edges between two queries that would not otherwise have an edge.

We now panic if any new dep node edges are created during the deserialization of a query result. This requires serializing the full AdtDef to disk, instead of just serializing the DefId and invoking the adt_def query during deserialization.

I'll probably split this up into several smaller PRs for perf runs.

rust-highfive · 2021-12-14T16:15:54Z

r? @jackh726

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

Aaron1011 · 2021-12-14T16:16:02Z

@bors try @rust-timer queue

rust-timer · 2021-12-14T16:16:04Z

Awaiting bors try build completion.

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

bors · 2021-12-14T16:16:11Z

⌛ Trying commit 193372013836ac4e5940ebfe088bdd44cdcee7e5 with merge 535f5db4e39c1538a165cd4e32d36e737c439cfd...

bors · 2021-12-14T16:29:31Z

💔 Test failed - checks-actions

Aaron1011 · 2021-12-14T20:40:33Z

@bors try @rust-timer queue

rust-timer · 2021-12-14T20:40:35Z

Awaiting bors try build completion.

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

bors · 2021-12-14T20:40:46Z

⌛ Trying commit b9c62879fd4c4a31eb2fa8606dd0e2351dba51fd with merge 861c72a96000618f65e5a7f8cf224b77be30b4fc...

bors · 2021-12-14T22:18:24Z

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

rust-timer · 2021-12-14T22:18:25Z

Queued 861c72a96000618f65e5a7f8cf224b77be30b4fc with parent 404c847, future comparison URL.

rust-timer · 2021-12-14T23:46:57Z

Finished benchmarking commit (861c72a96000618f65e5a7f8cf224b77be30b4fc): comparison url.

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

Very large improvement in instruction counts (up to -24.9% on incr-unchanged builds of issue-46449)
Very large regression in instruction counts (up to 16.1% on incr-full 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

teor2345 · 2021-12-14T23:54:08Z

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

Very large improvement in instruction counts (up to -24.9% on incr-unchanged builds of issue-46449)

Very large regression in instruction counts (up to 16.1% on incr-full builds of webrender-wrench)

Seems like an overall performance win, except for a few large outliers.

Aaron1011 · 2021-12-14T23:55:28Z

The perf results are very similar to PR #91924 which was split out from this one, so I think that PR is responsible for all of the significant performance changes.

apiraino · 2021-12-30T16:02:11Z

Beta backport declined as per compiler team discussion on Zulip

@rustbot label -beta-nominated

cjgillot · 2021-12-31T16:28:52Z

compiler/rustc_query_system/src/dep_graph/graph.rs

+        let mut deps = TaskDeps::default();
+        deps.read_allowed = false;
+        let deps = Lock::new(deps);
+        K::with_deps(Some(&deps), op)


Should we prefer to use an enum for deps:

enum TaskDepsRef<'a> { Allow(&'a Lock<TaskDeps>), Ignore, Forbid, }

Opened #92681

cjgillot · 2021-12-31T16:36:06Z

compiler/rustc_query_system/src/query/plumbing.rs

+        // are created during deserialization. See the docs of that method for more
+        // details.
+        let result = dep_graph
+            .with_query_deserialization(|| query.try_load_from_disk(tcx, prev_dep_node_index));


I think we can even forbid queries inside the invocation code. Can this call be moved around the NotYetStarted branch of try_execute_query? It's not supposed to invoke any user-provided code directly, only call into the DepGraph to setup a TaskDeps.

I tried this earlier, but there are some invocations of def_span within the query infrastructure itself. I think those could be removed in a follow-up PR, since this PR should catch almost all of the cases.

cjgillot · 2021-12-31T16:36:50Z

compiler/rustc_query_system/src/dep_graph/graph.rs

+    pub fn with_query_deserialization<OP, R>(&self, op: OP) -> R
+    where
+        OP: FnOnce() -> R,
+    {


Should we add a fast path if the dep-graph is disabled?

This should never be called, if the dep-graph is disabled, right? Maybe we could add a debug assertion that the dep-graph is indeed enabled.

michaelwoerister · 2022-01-03T10:43:04Z

This looks good to me now. @cjgillot's comments make sense to me, but I'd be fine with addressing them at a later point in time.

jackh726 · 2022-01-06T21:16:15Z

r? @michaelwoerister

michaelwoerister · 2022-01-07T16:01:45Z

Let's get this merged.

@bors r+

bors · 2022-01-07T16:01:46Z

📌 Commit 27ed52c has been approved by michaelwoerister

bors · 2022-01-08T18:32:39Z

⌛ Testing commit 27ed52c with merge a7e2e33...

bors · 2022-01-08T21:41:42Z

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing a7e2e33 to master...

rust-timer · 2022-01-09T01:58:12Z

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

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 14, 2021

rust-highfive assigned jackh726 Dec 14, 2021

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

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

This comment has been minimized.

Sign in to view

bors 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 Dec 14, 2021

This comment has been minimized.

Sign in to view

Aaron1011 force-pushed the query-recursive-read branch from 1933720 to b9c6287 Compare December 14, 2021 16:42

This was referenced Dec 14, 2021

thread 'rustc' panicked at 'called Option::unwrap() on a None value', /rustc/<snip>/compiler/rustc_hir/src/definitions.rs:452:14 #91696

Closed

Use tcx.def_path_hash in ExistentialPredicate.stable_cmp #91920

Merged

rustbot added perf-regression Performance regression. 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 Dec 14, 2021

Aaron1011 mentioned this pull request Dec 14, 2021

Fully serialize AdtDef #91924

Merged

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

rustbot removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Dec 30, 2021

cjgillot reviewed Dec 31, 2021

View reviewed changes

rust-highfive assigned michaelwoerister and unassigned cjgillot and jackh726 Jan 6, 2022

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 Jan 7, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 8, 2022

bors merged commit a7e2e33 into rust-lang:master Jan 8, 2022

rustbot added this to the 1.60.0 milestone Jan 8, 2022

rustbot removed the perf-regression Performance regression. label Jan 9, 2022

Kobzol mentioned this pull request Apr 7, 2022

ICE when checking a specific crate #91857

Closed

michaelwoerister mentioned this pull request Apr 22, 2024

Miri function identity hack: account for possible inlining #123781

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't perform any new queries while reading a query result on disk #91919

Don't perform any new queries while reading a query result on disk #91919

Aaron1011 commented Dec 14, 2021 •

edited

Loading

rust-highfive commented Dec 14, 2021

Aaron1011 commented Dec 14, 2021

rust-timer commented Dec 14, 2021

bors commented Dec 14, 2021

This comment has been minimized.

bors commented Dec 14, 2021

This comment has been minimized.

Aaron1011 commented Dec 14, 2021

rust-timer commented Dec 14, 2021

bors commented Dec 14, 2021

bors commented Dec 14, 2021

rust-timer commented Dec 14, 2021

rust-timer commented Dec 14, 2021

teor2345 commented Dec 14, 2021

Aaron1011 commented Dec 14, 2021

apiraino commented Dec 30, 2021

cjgillot Dec 31, 2021

Aaron1011 Jan 8, 2022

cjgillot Dec 31, 2021

Aaron1011 Dec 31, 2021

cjgillot Dec 31, 2021

michaelwoerister Jan 3, 2022

michaelwoerister commented Jan 3, 2022

jackh726 commented Jan 6, 2022

michaelwoerister commented Jan 7, 2022

bors commented Jan 7, 2022

bors commented Jan 8, 2022

bors commented Jan 8, 2022

rust-timer commented Jan 9, 2022

Don't perform any new queries while reading a query result on disk #91919

Don't perform any new queries while reading a query result on disk #91919

Conversation

Aaron1011 commented Dec 14, 2021 • edited Loading

rust-highfive commented Dec 14, 2021

Aaron1011 commented Dec 14, 2021

rust-timer commented Dec 14, 2021

bors commented Dec 14, 2021

This comment has been minimized.

bors commented Dec 14, 2021

This comment has been minimized.

Aaron1011 commented Dec 14, 2021

rust-timer commented Dec 14, 2021

bors commented Dec 14, 2021

bors commented Dec 14, 2021

rust-timer commented Dec 14, 2021

rust-timer commented Dec 14, 2021

teor2345 commented Dec 14, 2021

Aaron1011 commented Dec 14, 2021

apiraino commented Dec 30, 2021

cjgillot Dec 31, 2021

Choose a reason for hiding this comment

Aaron1011 Jan 8, 2022

Choose a reason for hiding this comment

cjgillot Dec 31, 2021

Choose a reason for hiding this comment

Aaron1011 Dec 31, 2021

Choose a reason for hiding this comment

cjgillot Dec 31, 2021

Choose a reason for hiding this comment

michaelwoerister Jan 3, 2022

Choose a reason for hiding this comment

michaelwoerister commented Jan 3, 2022

jackh726 commented Jan 6, 2022

michaelwoerister commented Jan 7, 2022

bors commented Jan 7, 2022

bors commented Jan 8, 2022

bors commented Jan 8, 2022

rust-timer commented Jan 9, 2022

Aaron1011 commented Dec 14, 2021 •

edited

Loading