-
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
incr.comp.: Introduce the concept of anonymous DepNodes and use them for trait selection #42769
Conversation
if self.current_node().is_some() { | ||
let source = self.make_node(v); | ||
self.add_edge_from_current_node(|current| (source, current)) | ||
match self.task_stack.last_mut() { |
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.
Wait, this can't be right: We are ignoring reads from anonymous tasks :/
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.
What do you mean?
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.
The OpenTask::Anon
variant is missing from the match, so reads during anonymous tasks are just dropped. I should have made the match exhaustive indeed :)
I'm in the process of fixing this but there are some complications (especially around
rust/src/librustc/traits/fulfill.rs
Line 628 in bd32b1b
self.dep_graph.read(data.dep_node(tcx)); |
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.
Oh, I see what you mean. Indeed, this is broken. Ha, score one for my persnicketiness about being exhaustive. =)
src/librustc/dep_graph/shadow.rs
Outdated
let forbidden_edge = if !ENABLED { | ||
None | ||
} else { | ||
match env::var("RUST_FORBID_DEP_GRAPH_EDGE") { |
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.
Is there no value to this check anymore? Does it not make sense in the new environment?
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.
(It still seems fairly useful to me =)
reads.push(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.
Nit: exhaustive plz =)
@michaelwoerister Looks like this is waiting on some changes from you. |
@Mark-Simulacrum Yes, this PR is currently on hold because of an issue with caching |
I'm closing this in favor of #43028 and a subsequent PR that will introduce anonymous nodes without using them for trait selection yet. |
incr.comp.: Deduplicate some DepNodes and introduce anonymous DepNodes This is a parallel PR to the pending #42769. It implements most of what is possible in terms of DepNode re-opening without having anonymous DepNodes yet (#42298). r? @nikomatsakis
This PR implements "anonymous"
DepNodes
, which areDepNodes
the identity of which is not known beforehand and is automatically computed from its set of input edges. These nodes allow for fine-grained dependency tracking in cases like trait selection where it is hard to come up with a goodDepNode
ID.This is an important step towards implementing #42298.
r? @nikomatsakis