Make dead code check a query. #93466

cjgillot · 2022-01-29T21:30:08Z

Dead code check is run for each invocation of the compiler, even if no modifications were involved.
This PR makes dead code check a query keyed on the module. This allows to skip the check when a module has not changed.
To perform this, a query live_symbols_and_ignored_derived_traits is introduced to encapsulate the global analysis of finding live symbols. The second query check_mod_deathness outputs diagnostics for each module based on this first query's results.

rust-highfive · 2022-01-29T21:30:11Z

r? @nagisa

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

cjgillot · 2022-01-30T12:06:43Z

@bors try @rust-timer queue

rust-timer · 2022-01-30T12:06:44Z

Awaiting bors try build completion.

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

bors · 2022-01-30T12:06:51Z

⌛ Trying commit d5edd89788d774568e72164be10a85003618e085 with merge 8d9357a1dbad121b8b946ee45f26ec538118ca50...

bors · 2022-01-30T13:24:32Z

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

rust-timer · 2022-01-30T13:24:34Z

Queued 8d9357a1dbad121b8b946ee45f26ec538118ca50 with parent a00e130, future comparison URL.

rust-timer · 2022-01-30T14:56:35Z

Finished benchmarking commit (8d9357a1dbad121b8b946ee45f26ec538118ca50): comparison url.

Summary: This benchmark run shows 69 relevant improvements 🎉 but 19 relevant regressions 😿 to instruction counts.

Average relevant regression: 0.9%
Average relevant improvement: -0.8%
Largest improvement in instruction counts: -3.1% on incr-unchanged builds of match-stress-enum check
Largest regression in instruction counts: 1.5% on incr-patched: dummy fn builds of unused-warnings check

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

nagisa · 2022-01-30T20:32:20Z

Could you please add a description to the PR and the commit describing the motivation behind this change (i.e. why is this change being made)?

cjgillot · 2022-01-30T20:41:11Z

@nagisa added one.

nagisa

LGTM overall. r=me with the two suggestions applied.

nagisa · 2022-01-30T20:36:14Z

compiler/rustc_interface/src/passes.rs

@@ -999,7 +999,11 @@ fn analysis(tcx: TyCtxt<'_>, (): ()) -> Result<()> {
                        tcx.ensure().check_private_in_public(());
                    },
                    {
-                        sess.time("death_checking", || rustc_passes::dead::check_crate(tcx));
+                        sess.time("death_checking", || {
+                            tcx.hir().par_for_each_module(|module| {


This par_ has no effect right now because we don't build with parallel_compiler, right?

Yes. When we are not in parallel_compiler, for_each_module and par_for_each_module are actually the same code.

compiler/rustc_middle/src/query/mod.rs

compiler/rustc_passes/src/dead.rs

nagisa · 2022-01-30T20:54:05Z

compiler/rustc_passes/src/dead.rs

@@ -726,6 +728,9 @@ impl<'tcx> Visitor<'tcx> for DeadVisitor<'tcx> {
    }

    fn visit_item(&mut self, item: &'tcx hir::Item<'tcx>) {
+        if let hir::ItemKind::Mod(..) = item.kind {


Hm, this looks slightly awkward to me. I wonder if it wouldn't be slightly more straightforward if visit_mod was overriden to be empty and then call walk_mod(visitor, module, module_id) when entering the visitor in check_mod_deathness?

That said, no strong opinion. Feel free to ignore, if you current approach is better.

nagisa · 2022-01-30T21:05:55Z

Hm, as for the perf results, it seems like the improvements are almost universally for the incr-unchanged case (naturally), and all the regressions come from incr-changed ones. From the looks of it, the additional cost may be in serializing the query results or some similar query system overhead (for e.g. here just the query to compute live items takes as much time as the death_checking did before on its own). I'm not familiar with the query system enough to suggest if there's a good way to avoid this…

nagisa · 2022-01-30T22:29:55Z

@cjgillot will you be looking into the perf regressions? I would not necessarily consider them hard blockers to landing this PR, but having a well researched description (since mine above is broadly just a guess) of why the perf hit is occurring in the first place would be nice.

@bors try @rust-timer queue

(not expecting significant changes in results, but the structure of the change will change the output of the timings somewhat and its better to be looking at the “current” version)

rust-timer · 2022-01-30T22:29:57Z

Awaiting bors try build completion.

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

bors · 2022-01-30T22:30:03Z

⌛ Trying commit 2c388378967e0ae6acbae938354c3e5d3f8e6c40 with merge 0c4e6240c88e78143766255d9e71641b59b94632...

rust-timer · 2022-01-30T22:30:11Z

Awaiting bors try build completion.

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

bors · 2022-01-31T00:03:29Z

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

rust-timer · 2022-01-31T00:03:31Z

Queued 0c4e6240c88e78143766255d9e71641b59b94632 with parent 8c7f2bf, future comparison URL.

cjgillot · 2022-01-31T11:50:27Z

The logic itself is not changed, so I lean towards query overhead.

Without digging very deep in the perf results:

the regression only appear for some incr-full and incr-patched loads : full is neutral, incr-unchanged is green;
the time taken by live_symbols_and_ignored_derived_traits + check_mod_deathness is of the same order of magnitude as former death_checking.

My guess is query system overhead: result hashing and dependency bookkeeping are responsible for the regression. Both are possible (live_symbols return value may be sizeable, and the former implementation did not keep a log of dependencies).

nagisa · 2022-01-31T12:08:24Z

I just realised what could be the at least partially the cause.

query live_symbols_and_ignored_derived_traits(_: ()) -> (FxHashSet<_>, FxHashMap<_, _>

to the best of my knowledge means that every time tcx.live_symbols_and_ignored_derived_traits() is called, its cached result will be cloned to produce an owned value result.

The check_mod_deathness then invokes this query for every module, thus resulting in N clones of the somewhat sizable list live symbols.

I wonder if storing the live symbols into TyCtxt and returning a &'tcx FxHash{Set,Map} would help to get rid of the regression.

cjgillot · 2022-01-31T15:50:08Z

@bors try @rust-timer queue

rust-timer · 2022-01-31T15:50:10Z

Awaiting bors try build completion.

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

bors · 2022-01-31T15:50:17Z

⌛ Trying commit b615df8d38887490c1b5955a654bd768ccbf9086 with merge fe3d394eec18b66eb89c81e6048e3bb4480c71b5...

bors · 2022-01-31T17:19:16Z

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

rust-timer · 2022-01-31T17:19:18Z

Queued fe3d394eec18b66eb89c81e6048e3bb4480c71b5 with parent 86f5e17, future comparison URL.

rust-timer · 2022-01-31T23:07:00Z

Finished benchmarking commit (fe3d394eec18b66eb89c81e6048e3bb4480c71b5): comparison url.

Summary: This benchmark run shows 67 relevant improvements 🎉 but 8 relevant regressions 😿 to instruction counts.

Average relevant regression: 1.0%
Average relevant improvement: -0.8%
Largest improvement in instruction counts: -3.3% on incr-unchanged builds of match-stress-enum check
Largest regression in instruction counts: 1.5% on incr-patched: dummy fn builds of unused-warnings check

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

nagisa · 2022-02-01T00:52:57Z

Looks great now. r=me with a cleaned up history.

cjgillot · 2022-02-01T12:47:40Z

@bors r=nagisa

bors · 2022-02-01T12:47:41Z

📌 Commit 4e7d47b has been approved by nagisa

bors · 2022-02-02T02:29:35Z

⌛ Testing commit 4e7d47b with merge d5f9c40...

bors · 2022-02-02T05:44:08Z

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing d5f9c40 to master...

rust-timer · 2022-02-02T12:32:44Z

Finished benchmarking commit (d5f9c40): comparison url.

Summary: This benchmark run shows 69 relevant improvements 🎉 but 12 relevant regressions 😿 to instruction counts.

Average relevant regression: 0.9%
Average relevant improvement: -0.8%
Largest improvement in instruction counts: -3.1% on incr-unchanged builds of match-stress-enum check
Largest regression in instruction counts: 1.6% on incr-patched: dummy fn builds of unused-warnings check

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

Zoxc · 2022-02-03T19:38:58Z

This PR does seem to regress, in instruction counts, both incr-full and incr-patched, which makes sense given that dead code checking runs on the entire crate and you can't easily offset the query overhead by skipping parts of its computation. It does help incr-unchanged, but that isn't a realistic workload and it shouldn't be prioritized over incr-patched.

nagisa · 2022-02-03T21:17:34Z

that isn't a realistic workload

Is it, really? Wouldn't running cargo test after the IDE cargo checks the crate for inline diagnostics be an example of a recurrent incr-unchanged workload?

Zoxc · 2022-02-03T23:12:49Z

Switching between cargo check and cargo build using the same incremental directory might be considered unmodified. cargo test adds tests and implicitly imports the test crate so I don't think it would qualify.

cargo check, cargo build and cargo test have separate incremental directories, so from the perspective of the incremental system there's no mixing of commands, which is mostly a good thing, since it only keeps computations from the latest.

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 29, 2022

rust-highfive assigned nagisa Jan 29, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 29, 2022

This comment has been minimized.

Sign in to view

cjgillot force-pushed the query-dead branch from 909c81c to d5edd89 Compare January 30, 2022 11:55

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 30, 2022

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jan 30, 2022

nagisa approved these changes Jan 30, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

cjgillot force-pushed the query-dead branch from fd6a7af to 2c38837 Compare January 30, 2022 21:41

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 30, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 31, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 31, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 31, 2022

Make dead code check a query.

4e7d47b

cjgillot force-pushed the query-dead branch from b615df8 to 4e7d47b Compare February 1, 2022 12:11

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 Feb 1, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 2, 2022

bors merged commit d5f9c40 into rust-lang:master Feb 2, 2022

rustbot added this to the 1.60.0 milestone Feb 2, 2022

cjgillot deleted the query-dead branch February 6, 2022 10:55

matthiaskrgr mentioned this pull request Aug 11, 2023

ICE: assertion failed left: '(Projection, AssocConst)' right: ' (ty::Opaque, DefKind::OpaqueTy) | (ty::Projection | ty::Inherent, DefKind::AssocTy) | (ty::Weak, DefKind::TyAlias { .. }) #114744

Closed

Make dead code check a query. #93466

Make dead code check a query. #93466

Conversation

cjgillot commented Jan 29, 2022 • edited Loading

rust-highfive commented Jan 29, 2022

This comment has been minimized.

cjgillot commented Jan 30, 2022

rust-timer commented Jan 30, 2022

bors commented Jan 30, 2022

bors commented Jan 30, 2022

rust-timer commented Jan 30, 2022

rust-timer commented Jan 30, 2022

nagisa commented Jan 30, 2022

cjgillot commented Jan 30, 2022

nagisa left a comment

Choose a reason for hiding this comment

nagisa Jan 30, 2022 • edited Loading

Choose a reason for hiding this comment

cjgillot Jan 30, 2022

Choose a reason for hiding this comment

nagisa Jan 30, 2022

Choose a reason for hiding this comment

nagisa commented Jan 30, 2022

This comment has been minimized.

nagisa commented Jan 30, 2022 • edited Loading

rust-timer commented Jan 30, 2022

bors commented Jan 30, 2022

rust-timer commented Jan 30, 2022

bors commented Jan 31, 2022

rust-timer commented Jan 31, 2022

cjgillot commented Jan 31, 2022

nagisa commented Jan 31, 2022 • edited Loading

cjgillot commented Jan 31, 2022

rust-timer commented Jan 31, 2022

bors commented Jan 31, 2022

bors commented Jan 31, 2022

rust-timer commented Jan 31, 2022

rust-timer commented Jan 31, 2022

nagisa commented Feb 1, 2022

cjgillot commented Feb 1, 2022

bors commented Feb 1, 2022

bors commented Feb 2, 2022

bors commented Feb 2, 2022

rust-timer commented Feb 2, 2022

Zoxc commented Feb 3, 2022

nagisa commented Feb 3, 2022

Zoxc commented Feb 3, 2022

cjgillot commented Jan 29, 2022 •

edited

Loading

nagisa Jan 30, 2022 •

edited

Loading

nagisa commented Jan 30, 2022 •

edited

Loading

nagisa commented Jan 31, 2022 •

edited

Loading