Skip to content

Simplification of query forcing #85319

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.

Sign up for GitHub

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 5 commits into from
May 30, 2021
Merged

Simplification of query forcing #85319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c2c59ae
Move key recovering into force_query.
cjgillot Nov 2, 2020
91444af
Refactor try_mark_previous_green.
cjgillot Nov 1, 2020
eb82187
Make the fast path faster.
cjgillot May 12, 2021
c95a568
Remove def_path_str.
cjgillot May 12, 2021
f3ed997
Move reconstruct test inwards.
cjgillot May 29, 2021
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
2 changes: 1 addition & 1 deletion compiler/rustc_query_impl/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use rustc_middle::ty::query::{query_keys, query_storage, query_stored, query_val
use rustc_middle::ty::query::{Providers, QueryEngine};
use rustc_middle::ty::{self, TyCtxt};
use rustc_serialize::opaque;
use rustc_span::{Span, DUMMY_SP};
use rustc_span::Span;

#[macro_use]
mod plumbing;
Expand Down
22 changes: 3 additions & 19 deletions compiler/rustc_query_impl/src/plumbing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use rustc_data_structures::sync::Lock;
use rustc_data_structures::thin_vec::ThinVec;
use rustc_errors::Diagnostic;
use rustc_serialize::opaque;
use rustc_span::def_id::{DefId, LocalDefId};
use rustc_span::def_id::LocalDefId;

#[derive(Copy, Clone)]
pub struct QueryCtxt<'tcx> {
Expand All @@ -25,6 +25,7 @@ pub struct QueryCtxt<'tcx> {
impl<'tcx> std::ops::Deref for QueryCtxt<'tcx> {
type Target = TyCtxt<'tcx>;

#[inline]
fn deref(&self) -> &Self::Target {
&self.tcx
}
Expand All @@ -42,10 +43,6 @@ impl HasDepContext for QueryCtxt<'tcx> {
}

impl QueryContext for QueryCtxt<'tcx> {
fn def_path_str(&self, def_id: DefId) -> String {
self.tcx.def_path_str(def_id)
}

fn current_query_job(&self) -> Option<QueryJobId<Self::DepKind>> {
tls::with_related_context(**self, |icx| icx.query)
}
Expand Down Expand Up @@ -457,20 +454,7 @@ macro_rules! define_queries {
}

fn force_from_dep_node(tcx: QueryCtxt<'_>, dep_node: &DepNode) -> bool {
if is_anon {
return false;
}

if !can_reconstruct_query_key() {
return false;
}

if let Some(key) = recover(*tcx, dep_node) {
force_query::<queries::$name<'_>, _>(tcx, key, DUMMY_SP, *dep_node);
return true;
}

false
force_query::<queries::$name<'_>, _>(tcx, dep_node)
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 directly take a function pointer to this rather than (hoping) that LLVM sees through this layer of indirection?

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 get a higher-ranked subtype error when trying to take a function pointer.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, that seems unfortunate. Feel free to re-run perf and/or r=me as I said.

}

fn try_load_from_on_disk_cache(tcx: QueryCtxt<'_>, dep_node: &DepNode) {
Expand Down
231 changes: 113 additions & 118 deletions compiler/rustc_query_system/src/dep_graph/graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,117 @@ impl<K: DepKind> DepGraph<K> {
}
}

fn try_mark_parent_green<Ctxt: QueryContext<DepKind = K>>(
&self,
tcx: Ctxt,
data: &DepGraphData<K>,
parent_dep_node_index: SerializedDepNodeIndex,
dep_node: &DepNode<K>,
) -> Option<()> {
let dep_dep_node_color = data.colors.get(parent_dep_node_index);
let dep_dep_node = &data.previous.index_to_node(parent_dep_node_index);

match dep_dep_node_color {
Some(DepNodeColor::Green(_)) => {
// This dependency has been marked as green before, we are
// still fine and can continue with checking the other
// dependencies.
debug!(
"try_mark_previous_green({:?}) --- found dependency {:?} to \
be immediately green",
dep_node, dep_dep_node,
);
return Some(());
}
Some(DepNodeColor::Red) => {
// We found a dependency the value of which has changed
// compared to the previous compilation session. We cannot
// mark the DepNode as green and also don't need to bother
// with checking any of the other dependencies.
debug!(
"try_mark_previous_green({:?}) - END - dependency {:?} was immediately red",
dep_node, dep_dep_node,
);
return None;
}
None => {}
}

// We don't know the state of this dependency. If it isn't
// an eval_always node, let's try to mark it green recursively.
if !dep_dep_node.kind.is_eval_always() {
debug!(
"try_mark_previous_green({:?}) --- state of dependency {:?} ({}) \
is unknown, trying to mark it green",
dep_node, dep_dep_node, dep_dep_node.hash,
);

let node_index =
self.try_mark_previous_green(tcx, data, parent_dep_node_index, dep_dep_node);
if node_index.is_some() {
debug!(
"try_mark_previous_green({:?}) --- managed to MARK dependency {:?} as green",
dep_node, dep_dep_node
);
return Some(());
}
}

// We failed to mark it green, so we try to force the query.
debug!(
"try_mark_previous_green({:?}) --- trying to force dependency {:?}",
dep_node, dep_dep_node
);
if !tcx.try_force_from_dep_node(dep_dep_node) {
// The DepNode could not be forced.
debug!(
"try_mark_previous_green({:?}) - END - dependency {:?} could not be forced",
dep_node, dep_dep_node
);
return None;
}

let dep_dep_node_color = data.colors.get(parent_dep_node_index);

match dep_dep_node_color {
Some(DepNodeColor::Green(_)) => {
debug!(
"try_mark_previous_green({:?}) --- managed to FORCE dependency {:?} to green",
dep_node, dep_dep_node
);
return Some(());
}
Some(DepNodeColor::Red) => {
debug!(
"try_mark_previous_green({:?}) - END - dependency {:?} was red after forcing",
dep_node, dep_dep_node
);
return None;
}
None => {}
}

if !tcx.dep_context().sess().has_errors_or_delayed_span_bugs() {
panic!("try_mark_previous_green() - Forcing the DepNode should have set its color")
}

// If the query we just forced has resulted in
// some kind of compilation error, we cannot rely on
// the dep-node color having been properly updated.
// This means that the query system has reached an
// invalid state. We let the compiler continue (by
// returning `None`) so it can emit error messages
// and wind down, but rely on the fact that this
// invalid state will not be persisted to the
// incremental compilation cache because of
// compilation errors being present.
debug!(
"try_mark_previous_green({:?}) - END - dependency {:?} resulted in compilation error",
dep_node, dep_dep_node
);
return None;
}

/// Try to mark a dep-node which existed in the previous compilation session as green.
fn try_mark_previous_green<Ctxt: QueryContext<DepKind = K>>(
&self,
Expand All @@ -512,123 +623,7 @@ impl<K: DepKind> DepGraph<K> {
let prev_deps = data.previous.edge_targets_from(prev_dep_node_index);

for &dep_dep_node_index in prev_deps {
let dep_dep_node_color = data.colors.get(dep_dep_node_index);

match dep_dep_node_color {
Some(DepNodeColor::Green(_)) => {
// This dependency has been marked as green before, we are
// still fine and can continue with checking the other
// dependencies.
debug!(
"try_mark_previous_green({:?}) --- found dependency {:?} to \
be immediately green",
dep_node,
data.previous.index_to_node(dep_dep_node_index)
);
}
Some(DepNodeColor::Red) => {
// We found a dependency the value of which has changed
// compared to the previous compilation session. We cannot
// mark the DepNode as green and also don't need to bother
// with checking any of the other dependencies.
debug!(
"try_mark_previous_green({:?}) - END - dependency {:?} was \
immediately red",
dep_node,
data.previous.index_to_node(dep_dep_node_index)
);
return None;
}
None => {
let dep_dep_node = &data.previous.index_to_node(dep_dep_node_index);

// We don't know the state of this dependency. If it isn't
// an eval_always node, let's try to mark it green recursively.
if !dep_dep_node.kind.is_eval_always() {
debug!(
"try_mark_previous_green({:?}) --- state of dependency {:?} ({}) \
is unknown, trying to mark it green",
dep_node, dep_dep_node, dep_dep_node.hash,
);

let node_index = self.try_mark_previous_green(
tcx,
data,
dep_dep_node_index,
dep_dep_node,
);
if node_index.is_some() {
debug!(
"try_mark_previous_green({:?}) --- managed to MARK \
dependency {:?} as green",
dep_node, dep_dep_node
);
continue;
}
}

// We failed to mark it green, so we try to force the query.
debug!(
"try_mark_previous_green({:?}) --- trying to force \
dependency {:?}",
dep_node, dep_dep_node
);
if tcx.try_force_from_dep_node(dep_dep_node) {
let dep_dep_node_color = data.colors.get(dep_dep_node_index);

match dep_dep_node_color {
Some(DepNodeColor::Green(_)) => {
debug!(
"try_mark_previous_green({:?}) --- managed to \
FORCE dependency {:?} to green",
dep_node, dep_dep_node
);
}
Some(DepNodeColor::Red) => {
debug!(
"try_mark_previous_green({:?}) - END - \
dependency {:?} was red after forcing",
dep_node, dep_dep_node
);
return None;
}
None => {
if !tcx.dep_context().sess().has_errors_or_delayed_span_bugs() {
panic!(
"try_mark_previous_green() - Forcing the DepNode \
should have set its color"
)
} else {
// If the query we just forced has resulted in
// some kind of compilation error, we cannot rely on
// the dep-node color having been properly updated.
// This means that the query system has reached an
// invalid state. We let the compiler continue (by
// returning `None`) so it can emit error messages
// and wind down, but rely on the fact that this
// invalid state will not be persisted to the
// incremental compilation cache because of
// compilation errors being present.
debug!(
"try_mark_previous_green({:?}) - END - \
dependency {:?} resulted in compilation error",
dep_node, dep_dep_node
);
return None;
}
}
}
} else {
// The DepNode could not be forced.
debug!(
"try_mark_previous_green({:?}) - END - dependency {:?} \
could not be forced",
dep_node, dep_dep_node
);
return None;
}
}
}
self.try_mark_parent_green(tcx, data, dep_dep_node_index, dep_node)?
}

// If we got here without hitting a `return` that means that all
Expand Down Expand Up @@ -797,7 +792,7 @@ impl<K: DepKind> DepGraph<K> {
}
}

fn next_virtual_depnode_index(&self) -> DepNodeIndex {
pub(crate) fn next_virtual_depnode_index(&self) -> DepNodeIndex {
let index = self.virtual_dep_node_index.fetch_add(1, Relaxed);
DepNodeIndex::from_u32(index)
}
Expand Down
4 changes: 0 additions & 4 deletions compiler/rustc_query_system/src/query/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ use crate::dep_graph::{DepNode, DepNodeIndex, HasDepContext, SerializedDepNodeIn
use rustc_data_structures::sync::Lock;
use rustc_data_structures::thin_vec::ThinVec;
use rustc_errors::Diagnostic;
use rustc_span::def_id::DefId;
use rustc_span::Span;

/// Description of a frame in the query stack.
Expand Down Expand Up @@ -64,9 +63,6 @@ impl QueryStackFrame {
}

pub trait QueryContext: HasDepContext {
/// Get string representation from DefPath.
fn def_path_str(&self, def_id: DefId) -> String;

/// Get the query information from the TLS context.
fn current_query_job(&self) -> Option<QueryJobId<Self::DepKind>>;

Expand Down
Loading