Skip to content

incr.comp.: Remove DepGraph::write() and its callers #42192

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 0 additions & 19 deletions src/librustc/dep_graph/dep_tracking_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
use hir::def_id::DefId;
use rustc_data_structures::fx::FxHashMap;
use std::cell::RefCell;
use std::collections::hash_map::Entry;
use std::ops::Index;
use std::hash::Hash;
use std::marker::PhantomData;
Expand Down Expand Up @@ -50,29 +49,11 @@ impl<M: DepTrackingMapConfig> DepTrackingMap<M> {
self.graph.read(dep_node);
}

/// Registers a (synthetic) write to the key `k`. Usually this is
/// invoked automatically by `insert`.
fn write(&self, k: &M::Key) {
let dep_node = M::to_dep_node(k);
self.graph.write(dep_node);
}

pub fn get(&self, k: &M::Key) -> Option<&M::Value> {
self.read(k);
self.map.get(k)
}

pub fn insert(&mut self, k: M::Key, v: M::Value) {
self.write(&k);
let old_value = self.map.insert(k, v);
assert!(old_value.is_none());
}

pub fn entry(&mut self, k: M::Key) -> Entry<M::Key, M::Value> {
self.write(&k);
self.map.entry(k)
}

pub fn contains_key(&self, k: &M::Key) -> bool {
self.read(k);
self.map.contains_key(k)
Expand Down
6 changes: 0 additions & 6 deletions src/librustc/dep_graph/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,12 +117,6 @@ impl DepGraph {
}
}

pub fn write(&self, v: DepNode<DefId>) {
if self.data.thread.is_enqueue_enabled() {
self.data.thread.enqueue(DepMessage::Write(v));
}
}

/// Indicates that a previous work product exists for `v`. This is
/// invoked during initial start-up based on what nodes are clean
/// (and what files exist in the incr. directory).
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/ty/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -689,7 +689,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
export_map: resolutions.export_map,
fulfilled_predicates: RefCell::new(fulfilled_predicates),
hir: hir,
maps: maps::Maps::new(dep_graph, providers),
maps: maps::Maps::new(providers),
mir_passes,
freevars: RefCell::new(resolutions.freevars),
maybe_unused_trait_imports: resolutions.maybe_unused_trait_imports,
Expand Down
39 changes: 26 additions & 13 deletions src/librustc/ty/maps.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use dep_graph::{DepGraph, DepNode, DepTrackingMap, DepTrackingMapConfig};
use dep_graph::{DepNode, DepTrackingMapConfig};
use hir::def_id::{CrateNum, CRATE_DEF_INDEX, DefId, LOCAL_CRATE};
use hir::def::Def;
use hir;
Expand All @@ -27,9 +27,11 @@ use ty::fast_reject::SimplifiedType;
use util::nodemap::{DefIdSet, NodeSet};

use rustc_data_structures::indexed_vec::IndexVec;
use rustc_data_structures::fx::FxHashMap;
use std::cell::{RefCell, RefMut};
use std::fmt::Debug;
use std::hash::Hash;
use std::marker::PhantomData;
use std::mem;
use std::collections::BTreeMap;
use std::ops::Deref;
Expand Down Expand Up @@ -180,6 +182,20 @@ impl<'tcx> Value<'tcx> for ty::SymbolName {
}
}

struct QueryMap<D: QueryDescription> {
phantom: PhantomData<D>,
map: FxHashMap<D::Key, D::Value>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use FxHashMap directly in the macro and perhaps even avoid having a trait for Key/Value altogether?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

impl<M: QueryDescription> QueryMap<M> {
fn new() -> QueryMap<M> {
QueryMap {
phantom: PhantomData,
map: FxHashMap(),
}
}
}

pub struct CycleError<'a, 'tcx: 'a> {
span: Span,
cycle: RefMut<'a, [(Span, Query<'tcx>)]>,
Expand Down Expand Up @@ -463,13 +479,12 @@ macro_rules! define_maps {
}

impl<$tcx> Maps<$tcx> {
pub fn new(dep_graph: DepGraph,
providers: IndexVec<CrateNum, Providers<$tcx>>)
pub fn new(providers: IndexVec<CrateNum, Providers<$tcx>>)
-> Self {
Maps {
providers,
query_stack: RefCell::new(vec![]),
$($name: RefCell::new(DepTrackingMap::new(dep_graph.clone()))),*
$($name: RefCell::new(QueryMap::new())),*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The advantage being that we would get rid of the runtime check? Sounds intriguing :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always expected it to be possible, it's just a matter of reducing the footprint to introduce a sound abstraction.

}
}
}
Expand Down Expand Up @@ -521,7 +536,7 @@ macro_rules! define_maps {
key,
span);

if let Some(result) = tcx.maps.$name.borrow().get(&key) {
if let Some(result) = tcx.maps.$name.borrow().map.get(&key) {
return Ok(f(result));
}

Expand All @@ -539,21 +554,19 @@ macro_rules! define_maps {
provider(tcx.global_tcx(), key)
})?;

Ok(f(tcx.maps.$name.borrow_mut().entry(key).or_insert(result)))
Ok(f(tcx.maps.$name.borrow_mut().map.entry(key).or_insert(result)))
}

pub fn try_get(tcx: TyCtxt<'a, $tcx, 'lcx>, span: Span, key: $K)
-> Result<$V, CycleError<'a, $tcx>> {
// We register the `read` here, but not in `force`, since
// `force` does not give access to the value produced (and thus
// we actually don't read it).
tcx.dep_graph.read(Self::to_dep_node(&key));
Self::try_get_with(tcx, span, key, Clone::clone)
}

pub fn force(tcx: TyCtxt<'a, $tcx, 'lcx>, span: Span, key: $K) {
// FIXME(eddyb) Move away from using `DepTrackingMap`
// so we don't have to explicitly ignore a false edge:
// we can't observe a value dependency, only side-effects,
// through `force`, and once everything has been updated,
// perhaps only diagnostics, if those, will remain.
let _ignore = tcx.dep_graph.in_ignore();
match Self::try_get_with(tcx, span, key, |_| ()) {
Ok(()) => {}
Err(e) => tcx.report_cycle(e)
Expand Down Expand Up @@ -644,7 +657,7 @@ macro_rules! define_map_struct {
tcx: $tcx,
input: $input,
output: ($($output)*
$(#[$attr])* $($pub)* $name: RefCell<DepTrackingMap<queries::$name<$tcx>>>,)
$(#[$attr])* $($pub)* $name: RefCell<QueryMap<queries::$name<$tcx>>>,)
}
};

Expand Down