Skip to content

incr.comp.: Introduce ensure and ensure typeck_tables_of #45228

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 4 commits into from
Oct 15, 2017
Merged
Show file tree
Hide file tree
Changes from 2 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
42 changes: 42 additions & 0 deletions src/librustc/ty/maps/plumbing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,48 @@ macro_rules! define_maps {
}
}

/// Ensure that either this query has all green inputs or been executed.
/// Executing query::ensure(D) is considered a read of the dep-node D.
///
/// This function is particularly useful when executing passes for their
/// side-effects -- e.g., in order to report errors for erroneous programs.
///
/// Note: The optimization is only available during incr. comp.
pub fn ensure(tcx: TyCtxt<'a, $tcx, 'lcx>, key: $K) -> () {
let dep_node = Self::to_dep_node(tcx, &key);

// Ensuring an "input" or anonymous query makes no sense
assert!(!dep_node.kind.is_anon());
assert!(!dep_node.kind.is_input());
use dep_graph::DepNodeColor;
match tcx.dep_graph.node_color(&dep_node) {
Some(DepNodeColor::Green(dep_node_index)) => {
profq_msg!(tcx, ProfileQueriesMsg::CacheHit);
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't do any profq_msg! calls in this method for now. We'll have to talk to @matthewhammer at some point about how to properly integrate the new tracking system with the query profiler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll pull them out. Thanks.

tcx.dep_graph.read_index(dep_node_index);
}
Some(DepNodeColor::Red) => {
let _ = tcx.$name(key);
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment here saying something like:

The DepNode being DepNodeColor::Red means that this query has been already executed before. We are still calling the proper again in order to issue the appropriate dep_graph.read() call. The performance cost this introduces should be negligible because we'll immediately hit the in-memory cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Just a moment.

}
None => {
// Huh
if !tcx.dep_graph.is_fully_enabled() {
let _ = tcx.$name(key);
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nikomatsakis Open question: Should the preceding 4 lines be in try_mark_green?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reference, try_mark_green will unwrap a None here if tcx.dep_graph.is_fully_enabled() would return False.

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather keep the explicit panic in try_mark_green, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll leave the code here as it is then.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also expected those lines to be in try_mark_green, but I trust @michaelwoerister's intuitions here. =) That said, I think we could consider trying to extract some of the common code between this function and try_get_with. I had thought it might make sense to extract a helper function that returns Some(dep_node_index) if either: the node color is green or can be made green, and None otherwise. This function here could invoke tcx.$name(key) if None is returned, and try_get_with can do whatever it does now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it! I'll implement that refactor as a follow on PR, unless this round of testing does not go well and I have to fix something.

match tcx.dep_graph.try_mark_green(tcx, &dep_node) {
Some(dep_node_index) => {
debug_assert!(tcx.dep_graph.is_green(dep_node_index));
profq_msg!(tcx, ProfileQueriesMsg::CacheHit);
tcx.dep_graph.read_index(dep_node_index);
}
None => {
let _ = tcx.$name(key);
}
}
}
}
}

fn compute_result(tcx: TyCtxt<'a, $tcx, 'lcx>, key: $K) -> $V {
let provider = tcx.maps.providers[key.map_crate()].$name;
provider(tcx.global_tcx(), key)
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_typeck/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -728,7 +728,7 @@ fn typeck_item_bodies<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, crate_num: CrateNum
debug_assert!(crate_num == LOCAL_CRATE);
Ok(tcx.sess.track_errors(|| {
for body_owner_def_id in tcx.body_owners() {
tcx.typeck_tables_of(body_owner_def_id);
ty::maps::queries::typeck_tables_of::ensure(tcx, body_owner_def_id);
}
})?)
}
Expand Down