Skip to content

incr.comp.: Switch to red/green change tracking, remove legacy system. #44901

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 12 commits into from
Oct 4, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
69 changes: 50 additions & 19 deletions src/librustc/dep_graph/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,15 @@ pub enum DepNodeColor {
Green(DepNodeIndex)
}

impl DepNodeColor {
pub fn is_green(self) -> bool {
match self {
DepNodeColor::Red => false,
DepNodeColor::Green(_) => true,
}
}
}

struct DepGraphData {
/// The old, initial encoding of the dependency graph. This will soon go
/// away.
Expand Down Expand Up @@ -94,6 +103,9 @@ struct DepGraphData {
work_products: RefCell<FxHashMap<WorkProductId, WorkProduct>>,

dep_node_debug: RefCell<FxHashMap<DepNode, String>>,

// Used for testing, only populated when -Zquery-dep-graph is specified.
loaded_from_cache: RefCell<FxHashMap<DepNodeIndexNew, bool>>,
}

impl DepGraph {
Expand All @@ -108,6 +120,7 @@ impl DepGraph {
current: RefCell::new(CurrentDepGraph::new()),
previous: prev_graph,
colors: RefCell::new(FxHashMap()),
loaded_from_cache: RefCell::new(FxHashMap()),
})),
fingerprints: Rc::new(RefCell::new(FxHashMap())),
}
Expand Down Expand Up @@ -256,16 +269,9 @@ impl DepGraph {
data.current.borrow_mut().push_anon_task();
let result = op();
let dep_node_index_legacy = data.edges.borrow_mut().pop_anon_task(dep_kind);
let (new_dep_node, dep_node_index_new) = data.current
.borrow_mut()
.pop_anon_task(dep_kind);
if let Some(new_dep_node) = new_dep_node {
assert!(data.colors
.borrow_mut()
.insert(new_dep_node, DepNodeColor::Red)
.is_none());
}

let dep_node_index_new = data.current
.borrow_mut()
.pop_anon_task(dep_kind);
(result, DepNodeIndex {
legacy: dep_node_index_legacy,
new: dep_node_index_new,
Expand Down Expand Up @@ -594,6 +600,25 @@ impl DepGraph {
}
}).unwrap_or(false)
}

pub fn mark_loaded_from_cache(&self, dep_node: DepNodeIndex, state: bool) {
debug!("mark_loaded_from_cache({:?}, {})",
self.data.as_ref().unwrap().current.borrow().nodes[dep_node.new],
state);

self.data
.as_ref()
.unwrap()
.loaded_from_cache
.borrow_mut()
.insert(dep_node.new, state);
}

pub fn was_loaded_from_cache(&self, dep_node: &DepNode) -> Option<bool> {
let data = self.data.as_ref().unwrap();
let dep_node_index = data.current.borrow().node_to_node_index[dep_node];
data.loaded_from_cache.borrow().get(&dep_node_index).cloned()
}
}

/// A "work product" is an intermediate result that we save into the
Expand Down Expand Up @@ -630,11 +655,6 @@ impl DepGraph {
#[derive(Clone, Debug, RustcEncodable, RustcDecodable)]
pub struct WorkProduct {
pub cgu_name: String,
/// Extra hash used to decide if work-product is still suitable;
/// note that this is *not* a hash of the work-product itself.
/// See documentation on `WorkProduct` type for an example.
pub input_hash: u64,

/// Saved files associated with this CGU
pub saved_files: Vec<(OutputType, String)>,
}
Expand All @@ -644,15 +664,26 @@ pub(super) struct CurrentDepGraph {
edges: IndexVec<DepNodeIndexNew, Vec<DepNodeIndexNew>>,
node_to_node_index: FxHashMap<DepNode, DepNodeIndexNew>,

anon_id_seed: Fingerprint,
Copy link
Contributor

Choose a reason for hiding this comment

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

this feels like it merits a nice comment

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed :)


task_stack: Vec<OpenTask>,
}

impl CurrentDepGraph {
fn new() -> CurrentDepGraph {
use std::time::{SystemTime, UNIX_EPOCH};

let duration = SystemTime::now().duration_since(UNIX_EPOCH).unwrap();
let nanos = duration.as_secs() * 1_000_000_000 +
duration.subsec_nanos() as u64;
let mut stable_hasher = StableHasher::new();
nanos.hash(&mut stable_hasher);

CurrentDepGraph {
nodes: IndexVec::new(),
edges: IndexVec::new(),
node_to_node_index: FxHashMap(),
anon_id_seed: stable_hasher.finish(),
task_stack: Vec::new(),
}
}
Expand Down Expand Up @@ -696,14 +727,14 @@ impl CurrentDepGraph {
});
}

fn pop_anon_task(&mut self, kind: DepKind) -> (Option<DepNode>, DepNodeIndexNew) {
fn pop_anon_task(&mut self, kind: DepKind) -> DepNodeIndexNew {
let popped_node = self.task_stack.pop().unwrap();

if let OpenTask::Anon {
read_set: _,
reads
} = popped_node {
let mut fingerprint = Fingerprint::zero();
let mut fingerprint = self.anon_id_seed;
let mut hasher = StableHasher::new();

for &read in reads.iter() {
Expand All @@ -725,9 +756,9 @@ impl CurrentDepGraph {
};

if let Some(&index) = self.node_to_node_index.get(&target_dep_node) {
(None, index)
index
} else {
(Some(target_dep_node), self.alloc_node(target_dep_node, reads))
self.alloc_node(target_dep_node, reads)
}
} else {
bug!("pop_anon_task() - Expected anonymous task to be popped")
Expand Down
14 changes: 12 additions & 2 deletions src/librustc/ty/maps/plumbing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,10 @@ macro_rules! define_maps {
dep_node_index)
}

debug!("ty::queries::{}::try_get_with(key={:?}) - running try_mark_green",
stringify!($name),
key);

if let Some(dep_node_index) = tcx.dep_graph.try_mark_green(tcx, &dep_node) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious, why not make try_mark_green check the cache, so we can collapse these two ifs into one? They seem sort of like fundamentally the same case (marked green already vs can mark green now)

Copy link
Member Author

Choose a reason for hiding this comment

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

Wouldn't that mean that we always load the result from the cache, even if it is never asked for?

debug_assert!(tcx.dep_graph.is_green(dep_node_index));
profq_msg!(tcx, ProfileQueriesMsg::CacheHit);
Expand Down Expand Up @@ -363,6 +367,10 @@ macro_rules! define_maps {
})
})?;

if tcx.sess.opts.debugging_opts.query_dep_graph {
tcx.dep_graph.mark_loaded_from_cache(dep_node_index, true);
}

let value = QueryValue::new(result, dep_node_index, diagnostics);

Ok((&tcx.maps
Expand Down Expand Up @@ -394,6 +402,10 @@ macro_rules! define_maps {

let ((result, dep_node_index), diagnostics) = res;

if tcx.sess.opts.debugging_opts.query_dep_graph {
tcx.dep_graph.mark_loaded_from_cache(dep_node_index, false);
}

let value = QueryValue::new(result, dep_node_index, diagnostics);

Ok(((&tcx.maps
Expand All @@ -406,8 +418,6 @@ macro_rules! define_maps {
dep_node_index))
}



pub fn try_get(tcx: TyCtxt<'a, $tcx, 'lcx>, span: Span, key: $K)
-> Result<$V, DiagnosticBuilder<'a>> {
match Self::try_get_with(tcx, span, key) {
Expand Down
1 change: 1 addition & 0 deletions src/librustc_incremental/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,4 @@ pub use persist::save_work_products;
pub use persist::in_incr_comp_dir;
pub use persist::prepare_session_directory;
pub use persist::finalize_session_directory;
pub use persist::delete_workproduct_files;
40 changes: 18 additions & 22 deletions src/librustc_incremental/persist/load.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ fn load_data(sess: &Session, path: &Path) -> Option<Vec<u8>> {
/// variants that represent inputs (HIR and imported Metadata).
fn does_still_exist(tcx: TyCtxt, dep_node: &DepNode) -> bool {
match dep_node.kind {
DepKind::Krate |
DepKind::Hir |
DepKind::HirBody |
DepKind::InScopeTraits |
Expand Down Expand Up @@ -258,33 +259,28 @@ fn transitive_dirty_nodes(serialized_dep_graph: &SerializedDepGraph,
/// otherwise no longer applicable.
fn reconcile_work_products<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
work_products: Vec<SerializedWorkProduct>,
clean_work_products: &FxHashSet<WorkProductId>) {
_clean_work_products: &FxHashSet<WorkProductId>) {
debug!("reconcile_work_products({:?})", work_products);
for swp in work_products {
if !clean_work_products.contains(&swp.id) {
debug!("reconcile_work_products: dep-node for {:?} is dirty", swp);
delete_dirty_work_product(tcx, swp);
} else {
let mut all_files_exist = true;
for &(_, ref file_name) in swp.work_product.saved_files.iter() {
let path = in_incr_comp_dir_sess(tcx.sess, file_name);
if !path.exists() {
all_files_exist = false;

if tcx.sess.opts.debugging_opts.incremental_info {
eprintln!("incremental: could not find file for \
up-to-date work product: {}", path.display());
}
let mut all_files_exist = true;
for &(_, ref file_name) in swp.work_product.saved_files.iter() {
let path = in_incr_comp_dir_sess(tcx.sess, file_name);
if !path.exists() {
all_files_exist = false;

if tcx.sess.opts.debugging_opts.incremental_info {
eprintln!("incremental: could not find file for \
up-to-date work product: {}", path.display());
}
}
}

if all_files_exist {
debug!("reconcile_work_products: all files for {:?} exist", swp);
tcx.dep_graph.insert_previous_work_product(&swp.id, swp.work_product);
} else {
debug!("reconcile_work_products: some file for {:?} does not exist", swp);
delete_dirty_work_product(tcx, swp);
}
if all_files_exist {
debug!("reconcile_work_products: all files for {:?} exist", swp);
tcx.dep_graph.insert_previous_work_product(&swp.id, swp.work_product);
} else {
debug!("reconcile_work_products: some file for {:?} does not exist", swp);
delete_dirty_work_product(tcx, swp);
}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/librustc_incremental/persist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,4 @@ pub use self::load::load_dep_graph_new;
pub use self::save::save_dep_graph;
pub use self::save::save_work_products;
pub use self::work_product::save_trans_partition;
pub use self::work_product::delete_workproduct_files;
5 changes: 1 addition & 4 deletions src/librustc_incremental/persist/work_product.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,9 @@ use std::fs as std_fs;
pub fn save_trans_partition(sess: &Session,
dep_graph: &DepGraph,
cgu_name: &str,
partition_hash: u64,
files: &[(OutputType, PathBuf)]) {
debug!("save_trans_partition({:?},{},{:?})",
debug!("save_trans_partition({:?},{:?})",
cgu_name,
partition_hash,
files);
if sess.opts.incremental.is_none() {
return;
Expand Down Expand Up @@ -57,7 +55,6 @@ pub fn save_trans_partition(sess: &Session,

let work_product = WorkProduct {
cgu_name: cgu_name.to_string(),
input_hash: partition_hash,
saved_files,
};

Expand Down
72 changes: 28 additions & 44 deletions src/librustc_trans/assert_module_sources.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,47 +27,32 @@
//! the HIR doesn't change as a result of the annotations, which might
//! perturb the reuse results.

use rustc::dep_graph::{DepNode, DepConstructor};
use rustc::ty::TyCtxt;
use syntax::ast;

use {ModuleSource, ModuleTranslation};

use rustc::ich::{ATTR_PARTITION_REUSED, ATTR_PARTITION_TRANSLATED};

const MODULE: &'static str = "module";
const CFG: &'static str = "cfg";

#[derive(Debug, PartialEq, Clone, Copy)]
pub enum Disposition { Reused, Translated }

impl ModuleTranslation {
pub fn disposition(&self) -> (String, Disposition) {
let disposition = match self.source {
ModuleSource::Preexisting(_) => Disposition::Reused,
ModuleSource::Translated(_) => Disposition::Translated,
};
enum Disposition { Reused, Translated }

(self.name.clone(), disposition)
}
}

pub(crate) fn assert_module_sources<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
modules: &[(String, Disposition)]) {
pub(crate) fn assert_module_sources<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) {
let _ignore = tcx.dep_graph.in_ignore();

if tcx.sess.opts.incremental.is_none() {
return;
}

let ams = AssertModuleSource { tcx: tcx, modules: modules };
let ams = AssertModuleSource { tcx };
for attr in &tcx.hir.krate().attrs {
ams.check_attr(attr);
}
}

struct AssertModuleSource<'a, 'tcx: 'a> {
tcx: TyCtxt<'a, 'tcx, 'tcx>,
modules: &'a [(String, Disposition)],
tcx: TyCtxt<'a, 'tcx, 'tcx>
}

impl<'a, 'tcx> AssertModuleSource<'a, 'tcx> {
Expand All @@ -86,32 +71,31 @@ impl<'a, 'tcx> AssertModuleSource<'a, 'tcx> {
}

let mname = self.field(attr, MODULE);
let mtrans = self.modules.iter().find(|&&(ref name, _)| name == mname.as_str());
let mtrans = match mtrans {
Some(m) => m,
None => {
debug!("module name `{}` not found amongst:", mname);
for &(ref name, ref disposition) in self.modules {
debug!("module named `{}` with disposition {:?}",
name,
disposition);
}

self.tcx.sess.span_err(
attr.span,
&format!("no module named `{}`", mname));
return;
}
};
let dep_node = DepNode::new(self.tcx,
DepConstructor::CompileCodegenUnit(mname.as_str()));

let mtrans_disposition = mtrans.1;
if disposition != mtrans_disposition {
self.tcx.sess.span_err(
attr.span,
&format!("expected module named `{}` to be {:?} but is {:?}",
mname,
disposition,
mtrans_disposition));
if let Some(loaded_from_cache) = self.tcx.dep_graph.was_loaded_from_cache(&dep_node) {
match (disposition, loaded_from_cache) {
(Disposition::Reused, false) => {
self.tcx.sess.span_err(
attr.span,
&format!("expected module named `{}` to be Reused but is Translated",
mname));
}
(Disposition::Translated, true) => {
self.tcx.sess.span_err(
attr.span,
&format!("expected module named `{}` to be Translated but is Reused",
mname));
}
(Disposition::Reused, true) |
(Disposition::Translated, false) => {
// These are what we would expect.
}
}
} else {
self.tcx.sess.span_err(attr.span, &format!("no module named `{}`", mname));
}
}

Expand Down
4 changes: 1 addition & 3 deletions src/librustc_trans/back/symbol_export.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,7 @@ pub fn provide_local(providers: &mut Providers) {
providers.is_exported_symbol = |tcx, id| {
// FIXME(#42293) needs red/green to not break a bunch of incremental
// tests
tcx.dep_graph.with_ignore(|| {
Copy link
Member

Choose a reason for hiding this comment

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

:D

(you can probably delete the FIXME above this as well)

Copy link
Member Author

Choose a reason for hiding this comment

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

:) Yes.

tcx.exported_symbol_ids(id.krate).contains(&id)
})
tcx.exported_symbol_ids(id.krate).contains(&id)
};

providers.exported_symbols = |tcx, cnum| {
Expand Down
Loading