Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove DepKind::CrateMetadata and pre-allocation of DepNodes #80602

Merged
merged 3 commits into from
Jan 16, 2021

Conversation

tgnottingham
Copy link
Contributor

@tgnottingham tgnottingham commented Jan 1, 2021

Remove much of the special-case handling around crate metadata
dependency tracking by replacing DepKind::CrateMetadata and the
pre-allocation of corresponding DepNodes with on-demand invocation
of the crate_hash query.

@rust-highfive
Copy link
Collaborator

r? @lcnr

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 1, 2021
@tgnottingham
Copy link
Contributor Author

@rustbot label T-compiler A-incr-comp

r? @michaelwoerister

@rustbot rustbot added A-incr-comp Area: Incremental compilation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 1, 2021
@tgnottingham
Copy link
Contributor Author

This also removes an optimization around registration of the crate metadata dependency, but I'm happy to add it back if necessary. Locally, the perf impact seemed to be small, so I think the simplification may be worth it.

This also relates to logic affecting #62649. That issue has a lot of confusion around it, and this change will help make reasoning about it easier. I will follow-up with that issue at some point.

@Aaron1011
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Contributor

bors commented Jan 1, 2021

⌛ Trying commit a3c3735cefaf039ec7f45e46f858ba2d641c5138 with merge a9d18ad03c95138870091e989d84f332af0a0b23...

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
skip untracked path cpu-usage.csv during rustfmt invocations
skip untracked path src/doc/book/ during rustfmt invocations
skip untracked path src/doc/rust-by-example/ during rustfmt invocations
skip untracked path src/llvm-project/ during rustfmt invocations
Diff in /checkout/compiler/rustc_middle/src/ty/context.rs at line 37:
 use rustc_errors::ErrorReported;
 use rustc_hir as hir;
 use rustc_hir::def::{DefKind, Res};
-use rustc_hir::def_id::{CrateNum, DefId, DefIdMap, LocalDefId};
 use rustc_hir::def_id::LOCAL_CRATE;
+use rustc_hir::def_id::{CrateNum, DefId, DefIdMap, LocalDefId};
 use rustc_hir::intravisit::Visitor;
 use rustc_hir::intravisit::Visitor;
 use rustc_hir::lang_items::LangItem;
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt" "--config-path" "/checkout" "--edition" "2018" "--unstable-features" "--skip-children" "--check" "/checkout/compiler/rustc_middle/src/ty/context.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.
Build completed unsuccessfully in 0:00:18

@tgnottingham
Copy link
Contributor Author

I really need to automate x.py fmt.

@cjgillot
Copy link
Contributor

cjgillot commented Jan 2, 2021

The command seems to have been lost.
@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Contributor

bors commented Jan 2, 2021

⌛ Trying commit ef71435e28c6743397d22957685a397b1e419da7 with merge 6da8d0299f5b2280b306e89ebfd7cb6902fcab5f...

@bors
Copy link
Contributor

bors commented Jan 2, 2021

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

@rust-timer
Copy link
Collaborator

Queued 6da8d0299f5b2280b306e89ebfd7cb6902fcab5f with parent 929f66a, future comparison URL.

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

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 2, 2021
@rust-timer
Copy link
Collaborator

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 2, 2021
@tgnottingham
Copy link
Contributor Author

Perf is neutral, which is good.

@michaelwoerister
Copy link
Member

Interesting idea! I hope I'll be able to review this next week.

Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR effectively replaces the creation of a dedicated DepNode for each crate by using the DepNode corresponding to the crate_hash query. Since the former behaviour based the greenness of the crate DepNode on this very crate_hash, there should be no change in the incremental behaviour.

@tgnottingham
Copy link
Contributor Author

Addressed comments and rebased to fix a conflict.

@bors
Copy link
Contributor

bors commented Jan 8, 2021

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

Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great find, @tgnottingham! It lets us get rid of quite a bit of special casing indeed. r=me with the comment below addressed.

compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/ty/context.rs Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Jan 12, 2021

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

Remove much of the special-case handling around crate metadata
dependency tracking by replacing `DepKind::CrateMetadata` and the
pre-allocation of corresponding `DepNodes` with on-demand invocation
of the `crate_hash` query.
@tgnottingham
Copy link
Contributor Author

Thank you for the review @michaelwoerister! Addressed your comments.

@pnkfelix
Copy link
Member

@bors r=michaelwoerister

@bors
Copy link
Contributor

bors commented Jan 15, 2021

📌 Commit 8e7cbc2 has been approved by michaelwoerister

@bors 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 15, 2021
@bors
Copy link
Contributor

bors commented Jan 15, 2021

⌛ Testing commit 8e7cbc2 with merge fcbd305...

@bors
Copy link
Contributor

bors commented Jan 16, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 16, 2021
@bors bors merged commit fcbd305 into rust-lang:master Jan 16, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 16, 2021
@tgnottingham tgnottingham deleted the cratemetadata_you_aint_special branch January 20, 2021 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-incr-comp Area: Incremental compilation merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.