incr.comp.: Remove DepGraph::write() and its callers #42192
Conversation
src/librustc/ty/maps.rs
Outdated
| } | ||
|
|
||
| pub fn try_get(tcx: TyCtxt<'a, $tcx, 'lcx>, span: Span, key: $K) | ||
| -> Result<$V, CycleError<'a, $tcx>> { | ||
| tcx.dep_graph.read(Self::to_dep_node(&key)); |
There was a problem hiding this comment.
I feel like we should move this read() into the body of try_get_with; basically into the most "fundamental" spot. (In that case, you could only execute it if the cache lookup succeeds, which I think makes the overall logic of this code cleaner anyhow.)
There was a problem hiding this comment.
Hm, but a force does not give you access to the value...
|
(r=me with that change) |
aidanhs
added
the
S-waiting-on-author
| @@ -180,6 +182,20 @@ impl<'tcx> Value<'tcx> for ty::SymbolName { | |||
| } | |||
| } | |||
|
|
|||
| struct QueryMap<D: QueryDescription> { | |||
| phantom: PhantomData<D>, | |||
| map: FxHashMap<D::Key, D::Value>, | |||
There was a problem hiding this comment.
Can we just use FxHashMap directly in the macro and perhaps even avoid having a trait for Key/Value altogether?
There was a problem hiding this comment.
I like the idea but that would complicate the define_map_struct macro. Since all of this will be refactored in the near future anyway, I think it's OK to do this in a later PR.
| -> Self { | ||
| Maps { | ||
| providers, | ||
| query_stack: RefCell::new(vec![]), | ||
| $($name: RefCell::new(DepTrackingMap::new(dep_graph.clone()))),* | ||
| $($name: RefCell::new(QueryMap::new())),* |
There was a problem hiding this comment.
I'm wondering if there's a way to get rid of the RefCell's here - perhaps a type with a very simple foolproof API based on Cell - something that requires a trait implemented for Copy types and Rc (which can't run arbitrary code when cloning), perhaps? It would probably have to be in its own module to prevent accidental bypass.
There was a problem hiding this comment.
The advantage being that we would get rid of the runtime check? Sounds intriguing :)
There was a problem hiding this comment.
I always expected it to be possible, it's just a matter of reducing the footprint to introduce a sound abstraction.
michaelwoerister
force-pushed
the
no_deptracking_map_in_queries
branch
from
09064a7
to
d68ccf6
Compare
michaelwoerister
force-pushed
the
no_deptracking_map_in_queries
branch
from
d68ccf6
to
c150301
Compare
|
@bors r=nikomatsakis Added a comment about the |
|
📌 Commit c150301 has been approved by |
…, r=nikomatsakis incr.comp.: Remove DepGraph::write() and its callers After months of yak shaving, we are finally there `:)` The existence of `DepGraph::write()` was one of the two main ways for introducing cycles into the dep-graph -- something we need to avoid in the future. The other way, re-opening nodes to add more edges, is next on the list. r? @nikomatsakis
|
☀️ Test successful - status-appveyor, status-travis |
After months of yak shaving, we are finally there
:)The existence of
DepGraph::write()was one of the two main ways for introducing cycles into the dep-graph -- something we need to avoid in the future. The other way, re-opening nodes to add more edges, is next on the list.r? @nikomatsakis