-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Make more passes incremental #51487
Make more passes incremental #51487
Conversation
This comment has been minimized.
This comment has been minimized.
Thanks, @Zoxc! I'll probably find some time to review this later this week. |
This comment has been minimized.
This comment has been minimized.
src/librustc/hir/map/mod.rs
Outdated
@@ -608,6 +609,80 @@ impl<'hir> Map<'hir> { | |||
&self.forest.krate.attrs | |||
} | |||
|
|||
pub fn visit_module_item_likes<V>(&self, module: DefId, visitor: V) |
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.
You can call this just visit_module_items
. "item-likes" is a term that includes hir::Item
, hir::ImplItem
, and hir::TraitItem
, the last two of which are not top-level items.
☔ The latest upstream changes (presumably #51550) made this pull request unmergeable. Please resolve the merge conflicts. |
Just to confirm, the new queries work at the module- instead of the item-level in order to make tracking less granular, right? Or is there another reason? |
let node_id = self.as_local_node_id(module).unwrap(); | ||
|
||
// Read the module so we'll be re-executed if new items appear in the module | ||
self.read(node_id); |
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 doesn't actually work as intended yet (see #40876). We should try and see if we can make this work properly.
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.
I was thinking of adding a HIRModule
dep-node. That would reduce the dep nodes used by the passes too.
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.
I was thinking of adding a HIRModule dep-node.
So that the hash of the HIRModule
dep-node would be a combination of the hashes of the module's items?
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.
Yeah.
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.
OK, maybe. I've just opened #51982 though, which should fix the problem. If that works out, we can use the existing solution in this PR.
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.
#51982 seems to work out just fine.
Yeah, I want to avoid most the overhead of queries for these cheaper passes. |
OK, makes sense. We might want to apply this approach to other passes too. |
src/librustc/hir/map/mod.rs
Outdated
}; | ||
|
||
// We can't krate() since that adds a dependency on the whole crate | ||
self.forest.krate.visit_all_item_likes(&mut filter); |
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 seems a bit roundabout to me. It always visits all items in the entire crate and then filters out everything except the items in the given module, right? But we have the list of contained items right there in hir::Mod::item_ids
.
Ping from triage @Zoxc! It's been a while since we heard from you, will you have time to work on this again? |
Ping from triage @Zoxc! It's been a while since we heard from you, any progress? |
Ping from triage, @Zoxc: we haven't heard from you in a while, so we are closing this PR for now. Thanks for your contribution and please re-open this PR if you work on it again. |
@bors try |
⌛ Trying commit aa3c7d719e20264d6439ac4986a83baa3f126fc6 with merge 6c428b0ab0dfe9643bb5b996b304b8f813ba26e6... |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☀️ Test successful - status-travis |
@rust-timer build 6c428b0ab0dfe9643bb5b996b304b8f813ba26e6 |
Success: Queued 6c428b0ab0dfe9643bb5b996b304b8f813ba26e6 with parent 0e6f898, comparison URL. |
⌛ Testing commit e69c2c7 with merge 267ee5bff2c343dd2d59c70a49da62364cd47935... |
💔 Test failed - checks-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Hm, Apple builders seem to not be happy but at a glance this doesn’t regress CI time comparing to other PRs. Let’s try one more time before summoning someone more knowledgeable from infra... @bors retry |
Make more passes incremental r? @michaelwoerister
☀️ Test successful - checks-travis, status-appveyor |
woah, this is a noticeable hit. 🎉 Thanks for your work! |
Make privacy checking, intrinsic checking and liveness checking incremental Blocked on rust-lang#51487 r? @michaelwoerister
Make privacy checking, intrinsic checking and liveness checking incremental Blocked on rust-lang#51487 r? @michaelwoerister
Make privacy checking, intrinsic checking and liveness checking incremental Blocked on rust-lang#51487 r? @michaelwoerister
Make privacy checking, intrinsic checking and liveness checking incremental Blocked on rust-lang#51487 r? @michaelwoerister
Make privacy checking, intrinsic checking and liveness checking incremental Blocked on rust-lang#51487 r? @michaelwoerister
Make privacy checking, intrinsic checking and liveness checking incremental Blocked on #51487 r? @michaelwoerister
r? @michaelwoerister