-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Remove DepKind::CrateMetadata and pre-allocation of DepNodes #80602
Conversation
r? @lcnr (rust-highfive has picked a reviewer for you, use r? to override) |
@rustbot label T-compiler A-incr-comp |
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. |
@bors try @rust-timer queue |
Awaiting bors try build completion. |
⌛ Trying commit a3c3735cefaf039ec7f45e46f858ba2d641c5138 with merge a9d18ad03c95138870091e989d84f332af0a0b23... |
The job Click to see the possible cause of the failure (guessed by this bot)
|
a3c3735
to
de92295
Compare
I really need to automate |
de92295
to
ef71435
Compare
The command seems to have been lost. |
Awaiting bors try build completion. |
⌛ Trying commit ef71435e28c6743397d22957685a397b1e419da7 with merge 6da8d0299f5b2280b306e89ebfd7cb6902fcab5f... |
☀️ Try build successful - checks-actions |
Queued 6da8d0299f5b2280b306e89ebfd7cb6902fcab5f with parent 929f66a, future comparison URL. @rustbot label: +S-waiting-on-perf |
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 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 |
Perf is neutral, which is good. |
Interesting idea! I hope I'll be able to review this next week. |
There was a problem hiding this 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.
65da797
to
0525d28
Compare
Addressed comments and rebased to fix a conflict. |
☔ The latest upstream changes (presumably #78452) made this pull request unmergeable. Please resolve the merge conflicts. |
0525d28
to
3cbe0ad
Compare
There was a problem hiding this 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.
☔ 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.
9bc5aa9
to
8e7cbc2
Compare
Thank you for the review @michaelwoerister! Addressed your comments. |
@bors r=michaelwoerister |
📌 Commit 8e7cbc2 has been approved by |
☀️ Test successful - checks-actions |
Remove much of the special-case handling around crate metadata
dependency tracking by replacing
DepKind::CrateMetadata
and thepre-allocation of corresponding
DepNodes
with on-demand invocationof the
crate_hash
query.