-
Notifications
You must be signed in to change notification settings - Fork 13k
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 all occurrences of "DepNode re-opening" #42298
Comments
It might also turn out that we do want to support re-opening nodes and just ensure cycle freedom via a runtime check. For things like symbol names that we don't want to cache on-disk, it seems a bit excessive to have one node per monomorphic instance (strategy 1), it's a bit complicated to have one node for all instances (strategy 2), and the anonymous node strategy might end up with the same number of nodes as strategy 1. We could also have simplified cycle-check for cases like this: Every kind of DepNode is assigned to a compiler phase (or something like that) and we just disallow adding edges from phase |
I think I had planned to handle this by moving to anonymous nodes. I think we should be able to add that as a concept into the existing dep-graph without any particular problem, right? |
It is (potentially) true that we might still want to be able to "re-open" nodes, but I'm not sure. I'd rather pursue using anonymous nodes and see how far that takes us. |
I think so. Not sure about the implementation details.
Sure. However, as in the example above, there might be cases where anonymous nodes would not improve performance compared to maximal granularity. It's something we should keep an eye on. |
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
incr.comp.: Assert that no DepNode is re-opened (see issue #42298). This PR removes the last occurrence of DepNode re-opening and adds an assertion that prevents our doing so in the future too. The DepGraph should no be guaranteed to be cycle free. r? @nikomatsakis EDIT: Closes #42298
In order for the new red/green dependency tracking to work and for our compiler developers' sanity, we need incremental compilation's dependency graph to not contain any cycles. Since
DepGraph::write()
has been removed in #42192, we are close to guaranteeing this "by construction". The only other way of introducing cycles into the dependency graph is by "re-opening" a node:Currently, re-opening is needed for supporting
DepNode
merging: Many tasks share the sameDepNode
. For example,DepNode::ItemSignature
is used for many things that conceptually are part of an item's signature. Another example is trait selection where we don't want to track at full accuracy since that would cause too much overhead.There are three basic ways of avoiding node re-opening:
DepNode
fully accurate so that two distinct tasks use two distinctDepNodes
instead of using the same one. This increases tracking overhead.DepNode
. This decreases laziness since not all parts of a result might be needed. It might also lead to result with manyOption
fields in them because some parts of it are only valid for a subset of query keys (e.g.impl_trait_ref
is only valid for trait impls but if it was part of a largeritem_signature
query, we would have to provide it for any kind of item that has a signature).DepNodes
which allows node merging in a safe way that keeps the graph free of cycles. Anonymous nodes can only be used for things the result of which we do not cache and using them is slightly more expensive than using a regular node.DepNode
kinds that will need un-merging are the following (in addition to others probably):There's also
TransTraitCaches
which still usesDepTrackingMap
directly.It remains to be clarified which strategy to use exactly in which case.
cc @nikomatsakis and @eddyb
The text was updated successfully, but these errors were encountered: