Skip to content

incr.comp.: Make sure we don't lose unused green results from the query cache. #46111

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 1 commit into from
Nov 25, 2017
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
33 changes: 33 additions & 0 deletions src/librustc/dep_graph/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -659,6 +659,39 @@ impl DepGraph {
}).unwrap_or(false)
}

// This method loads all on-disk cacheable query results into memory, so
// they can be written out to the new cache file again. Most query results
// will already be in memory but in the case where we marked something as
// green but then did not need the value, that value will never have been
// loaded from disk.
//
// This method will only load queries that will end up in the disk cache.
// Other queries will not be executed.
pub fn exec_cache_promotions<'a, 'tcx>(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>) {
let green_nodes: Vec<DepNode> = {
let data = self.data.as_ref().unwrap();
data.colors.borrow().iter().filter_map(|(dep_node, color)| match color {
DepNodeColor::Green(_) => {
if dep_node.cache_on_disk(tcx) {
Some(*dep_node)
} else {
None
}
}
DepNodeColor::Red => {
// We can skip red nodes because a node can only be marked
// as red if the query result was recomputed and thus is
// already in memory.
None
}
}).collect()
};

for dep_node in green_nodes {
dep_node.load_from_on_disk_cache(tcx);
}
}

pub fn mark_loaded_from_cache(&self, dep_node_index: DepNodeIndex, state: bool) {
debug!("mark_loaded_from_cache({:?}, {})",
self.data.as_ref().unwrap().current.borrow().nodes[dep_node_index],
Expand Down
4 changes: 4 additions & 0 deletions src/librustc/ty/maps/on_disk_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,10 @@ impl<'sess> OnDiskCache<'sess> {

encoder.encode_tagged(PREV_DIAGNOSTICS_TAG, &diagnostics)?;

// Load everything into memory so we can write it out to the on-disk
// cache. The vast majority of cacheable query results should already
// be in memory, so this should be a cheap operation.
tcx.dep_graph.exec_cache_promotions(tcx);

// Encode query results
let mut query_result_index = EncodedQueryResultIndex::new();
Expand Down
55 changes: 55 additions & 0 deletions src/librustc/ty/maps/plumbing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -900,3 +900,58 @@ pub fn force_from_dep_node<'a, 'gcx, 'lcx>(tcx: TyCtxt<'a, 'gcx, 'lcx>,

true
}


// FIXME(#45015): Another piece of boilerplate code that could be generated in
// a combined define_dep_nodes!()/define_maps!() macro.
macro_rules! impl_load_from_cache {
($($dep_kind:ident => $query_name:ident,)*) => {
impl DepNode {
// Check whether the query invocation corresponding to the given
// DepNode is eligible for on-disk-caching.
pub fn cache_on_disk(&self, tcx: TyCtxt) -> bool {
use ty::maps::queries;
use ty::maps::QueryDescription;

match self.kind {
$(DepKind::$dep_kind => {
let def_id = self.extract_def_id(tcx).unwrap();
queries::$query_name::cache_on_disk(def_id)
})*
_ => false
}
}

// This is method will execute the query corresponding to the given
// DepNode. It is only expected to work for DepNodes where the
// above `cache_on_disk` methods returns true.
// Also, as a sanity check, it expects that the corresponding query
// invocation has been marked as green already.
pub fn load_from_on_disk_cache(&self, tcx: TyCtxt) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I must be confused, but in what sense does this load from the on-disk cache? It appears to re-execute the query.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I guess the point is that it will re-execute the query, which will force us to load. The reason this wrapper exists, then, is to do the assertions? Wouldn't there otherwise be some existing method we could use?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I guess the point is that it will re-execute the query, which will force us to load.

Yes

The reason this wrapper exists, then, is to do the assertions?

No, it maps from DepNode to query. There's also force_from_dep_node, which also maps from DepNode to query, but it assumes that the thing being forced has not been seen yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, that sounds roughly like what I meant.

match self.kind {
$(DepKind::$dep_kind => {
debug_assert!(tcx.dep_graph
.node_color(self)
.map(|c| c.is_green())
.unwrap_or(false));

let def_id = self.extract_def_id(tcx).unwrap();
let _ = tcx.$query_name(def_id);
})*
_ => {
bug!()
}
}
}
}
}
}

impl_load_from_cache!(
TypeckTables => typeck_tables_of,
MirOptimized => optimized_mir,
UnsafetyCheckResult => unsafety_check_result,
BorrowCheck => borrowck,
MirBorrowCheck => mir_borrowck,
MirConstQualif => mir_const_qualif,
);