Skip to content
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

Make create_def a side effect instead of marking the entire query as always red #115613

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
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
8 changes: 6 additions & 2 deletions compiler/rustc_hir/src/definitions.rs
Original file line number Diff line number Diff line change
@@ -346,7 +346,11 @@ impl Definitions {
}

/// Adds a definition with a parent definition.
pub fn create_def(&mut self, parent: LocalDefId, data: DefPathData) -> LocalDefId {
pub fn create_def(
&mut self,
parent: LocalDefId,
data: DefPathData,
) -> (LocalDefId, DefPathHash) {
// We can't use `Debug` implementation for `LocalDefId` here, since it tries to acquire a
// reference to `Definitions` and we're already holding a mutable reference.
debug!(
@@ -375,7 +379,7 @@ impl Definitions {
debug!("create_def: after disambiguation, key = {:?}", key);

// Create the definition.
LocalDefId { local_def_index: self.table.allocate(key, def_path_hash) }
(LocalDefId { local_def_index: self.table.allocate(key, def_path_hash) }, def_path_hash)
}

#[inline(always)]
9 changes: 9 additions & 0 deletions compiler/rustc_hir_analysis/src/lib.rs
Original file line number Diff line number Diff line change
@@ -172,6 +172,15 @@ pub fn check_crate(tcx: TyCtxt<'_>) {
}
});

// Make sure we actually have a value for static items, as they aren't cached in incremental.
// While we could just wait for codegen to invoke this, the definitions freeze below will cause
// that to ICE, because evaluating statics can create more items.
tcx.hir().par_body_owners(|item_def_id| {
if let DefKind::Static { .. } = tcx.def_kind(item_def_id) {
let _ = tcx.eval_static_initializer(item_def_id);
}
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this block necessary? The previous calls tcx.ensure().eval_static_initializer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just test code to replicate the bug I had initially, it should not land

// FIXME: Remove this when we implement creating `DefId`s
// for anon constants during their parents' typeck.
// Typeck all body owners in parallel will produce queries
5 changes: 3 additions & 2 deletions compiler/rustc_interface/src/callbacks.rs
Original file line number Diff line number Diff line change
@@ -41,8 +41,9 @@ fn track_span_parent(def_id: rustc_span::def_id::LocalDefId) {
fn track_diagnostic<R>(diagnostic: DiagInner, f: &mut dyn FnMut(DiagInner) -> R) -> R {
tls::with_context_opt(|icx| {
if let Some(icx) = icx {
if let Some(diagnostics) = icx.diagnostics {
diagnostics.lock().extend(Some(diagnostic.clone()));
if let Some(side_effects) = icx.side_effects {
let diagnostic = diagnostic.clone();
side_effects.lock().diagnostics.push(diagnostic);
}

// Diagnostics are tracked, we can ignore the dependency.
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/query/mod.rs
Original file line number Diff line number Diff line change
@@ -1113,7 +1113,6 @@ rustc_queries! {
"evaluating initializer of static `{}`",
tcx.def_path_str(key)
}
cache_on_disk_if { key.is_local() }
separate_provide_extern
feedable
}
@@ -1841,6 +1840,7 @@ rustc_queries! {
desc { |tcx| "computing visibility of `{}`", tcx.def_path_str(def_id) }
separate_provide_extern
feedable
cache_on_disk_if { def_id.is_local() }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be benchmarked and landed separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh whoops, this is not a perf thing, this was a hack to avoid having to implement

need to make feeding queries a side effect, too. At least ones that aren't written to disk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I looked into how we could design this, and I think the best way to do this is to require queries that get fed to also get cached to disk.

The only other alternative I came up with is to store all fed queries in the side effect table of their caller (somewhat hard, because we need to store all possible key/value combinations)
This also feels like it duplicates the cache-to-disk logic, and definitely does if the query is also marked as cache-to-disk.

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 think the best way to do this is to require queries that get fed to also get cached to disk.

I no longer think this. That would require thinking about HIR in incremental, at minimum due to the opt_hir_owner_nodes query, but local_def_id_to_hir_id has no real way of being cached

}

query inhabited_predicate_adt(key: DefId) -> ty::inhabitedness::InhabitedPredicate<'tcx> {
67 changes: 51 additions & 16 deletions compiler/rustc_middle/src/ty/context.rs
Original file line number Diff line number Diff line change
@@ -39,7 +39,7 @@ use rustc_hir::{HirId, Node, TraitCandidate};
use rustc_index::IndexVec;
use rustc_macros::{HashStable, TyDecodable, TyEncodable};
use rustc_query_system::cache::WithDepNode;
use rustc_query_system::dep_graph::DepNodeIndex;
use rustc_query_system::dep_graph::{DepNodeIndex, TaskDepsRef};
use rustc_query_system::ich::StableHashingContext;
use rustc_serialize::opaque::{FileEncodeResult, FileEncoder};
use rustc_session::config::CrateType;
@@ -50,10 +50,8 @@ use rustc_span::def_id::{CRATE_DEF_ID, DefPathHash, StableCrateId};
use rustc_span::symbol::{Ident, Symbol, kw, sym};
use rustc_span::{DUMMY_SP, Span};
use rustc_type_ir::TyKind::*;
use rustc_type_ir::fold::TypeFoldable;
use rustc_type_ir::lang_items::TraitSolverLangItem;
pub use rustc_type_ir::lift::Lift;
use rustc_type_ir::{CollectAndApply, Interner, TypeFlags, WithCachedTypeInfo, search_graph};
use rustc_type_ir::WithCachedTypeInfo;
use rustc_type_ir::{CollectAndApply, Interner, TypeFlags};
use tracing::{debug, instrument};

use crate::arena::Arena;
@@ -1818,34 +1816,71 @@ impl<'tcx> TyCtxtAt<'tcx> {

impl<'tcx> TyCtxt<'tcx> {
/// `tcx`-dependent operations performed for every created definition.
#[instrument(level = "trace", skip(self))]
pub fn create_def(
self,
parent: LocalDefId,
name: Symbol,
def_kind: DefKind,
) -> TyCtxtFeed<'tcx, LocalDefId> {
let data = def_kind.def_path_data(name);
// The following call has the side effect of modifying the tables inside `definitions`.
// The following create_def calls have the side effect of modifying the tables inside `definitions`.
// These very tables are relied on by the incr. comp. engine to decode DepNodes and to
// decode the on-disk cache.
//
// Any LocalDefId which is used within queries, either as key or result, either:
// - has been created before the construction of the TyCtxt;
// - has been created before the construction of the TyCtxt,
// - has been created when marking a query as green (recreating definitions it created in the actual run),
// - has been created by this call to `create_def`.
// As a consequence, this LocalDefId is always re-created before it is needed by the incr.
// comp. engine itself.
//
// This call also writes to the value of `source_span` and `expn_that_defined` queries.
// This is fine because:
// - those queries are `eval_always` so we won't miss their result changing;
// - this write will have happened before these queries are called.
Comment on lines 1835 to 1839
Copy link
Contributor

Choose a reason for hiding this comment

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

Stale comment (pre-existing).

let def_id = self.untracked.definitions.write().create_def(parent, data);

// This function modifies `self.definitions` using a side-effect.
// We need to ensure that these side effects are re-run by the incr. comp. engine.
// Depending on the forever-red node will tell the graph that the calling query
// needs to be re-evaluated.
self.dep_graph.read_index(DepNodeIndex::FOREVER_RED_NODE);
let def_id = tls::with_context(|icx| {
match icx.task_deps {
// Always gets rerun anyway, so nothing to replay
TaskDepsRef::EvalAlways => {
let def_id = self.untracked.definitions.write().create_def(parent, data).0;
trace!(?def_id, "eval always");
def_id
}
// Top-level queries like the resolver get rerun every time anyway
TaskDepsRef::Ignore => {
let def_id = self.untracked.definitions.write().create_def(parent, data).0;
trace!(?def_id, "ignore");
def_id
}
TaskDepsRef::Forbid => bug!(
"cannot create definition {parent:?}, {name:?}, {def_kind:?} without being able to register task dependencies"
),
TaskDepsRef::Allow(_) => {
let (def_id, hash) =
self.untracked.definitions.write().create_def(parent, data);
trace!(?def_id, "record side effects");

icx.side_effects.as_ref().unwrap().lock().definitions.push(DefIdInfo {
parent,
data,
hash,
});
def_id
}
TaskDepsRef::Replay { prev_side_effects, created_def_ids } => {
trace!(?created_def_ids, "replay side effects");
trace!("num_defs : {}", prev_side_effects.definitions.len());
let index = created_def_ids.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
let prev_info = &prev_side_effects.definitions[index];
let def_id = self.untracked.definitions.read().local_def_path_hash_to_def_id(
prev_info.hash,
&"should have already recreated def id in try_mark_green",
);
assert_eq!(prev_info.data, data);
assert_eq!(prev_info.parent, parent);
def_id
}
Comment on lines +1869 to +1881
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is subtle, we should definitely document it somewhere. Here, or on DefIdInfo struct?

}
});

let feed = TyCtxtFeed { tcx: self, key: def_id };
feed.def_kind(def_kind);
7 changes: 3 additions & 4 deletions compiler/rustc_middle/src/ty/context/tls.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
use std::{mem, ptr};

use rustc_data_structures::sync::{self, Lock};
use rustc_errors::DiagInner;
use thin_vec::ThinVec;
use rustc_query_system::query::QuerySideEffects;

use super::{GlobalCtxt, TyCtxt};
use crate::dep_graph::TaskDepsRef;
@@ -24,7 +23,7 @@ pub struct ImplicitCtxt<'a, 'tcx> {

/// Where to store diagnostics for the current query job, if any.
/// This is updated by `JobOwner::start` in `ty::query::plumbing` when executing a query.
pub diagnostics: Option<&'a Lock<ThinVec<DiagInner>>>,
pub side_effects: Option<&'a Lock<QuerySideEffects>>,

/// Used to prevent queries from calling too deeply.
pub query_depth: usize,
@@ -40,7 +39,7 @@ impl<'a, 'tcx> ImplicitCtxt<'a, 'tcx> {
ImplicitCtxt {
tcx,
query: None,
diagnostics: None,
side_effects: None,
query_depth: 0,
task_deps: TaskDepsRef::Ignore,
}
25 changes: 19 additions & 6 deletions compiler/rustc_query_impl/src/plumbing.rs
Original file line number Diff line number Diff line change
@@ -7,7 +7,6 @@ use std::num::NonZero;
use rustc_data_structures::stable_hasher::{Hash64, HashStable, StableHasher};
use rustc_data_structures::sync::Lock;
use rustc_data_structures::unord::UnordMap;
use rustc_errors::DiagInner;
use rustc_index::Idx;
use rustc_middle::bug;
use rustc_middle::dep_graph::{
@@ -24,14 +23,13 @@ use rustc_middle::ty::{self, TyCtxt, TyEncoder};
use rustc_query_system::dep_graph::{DepNodeParams, HasDepContext};
use rustc_query_system::ich::StableHashingContext;
use rustc_query_system::query::{
QueryCache, QueryConfig, QueryContext, QueryJobId, QueryMap, QuerySideEffects, QueryStackFrame,
force_query,
DefIdInfo, QueryCache, QueryConfig, QueryContext, QueryJobId, QueryMap, QuerySideEffects,
QueryStackFrame, force_query,
};
use rustc_query_system::{LayoutOfDepth, QueryOverflow};
use rustc_serialize::{Decodable, Encodable};
use rustc_session::Limit;
use rustc_span::def_id::LOCAL_CRATE;
use thin_vec::ThinVec;

use crate::QueryConfigRestored;

@@ -127,7 +125,7 @@ impl QueryContext for QueryCtxt<'_> {
self,
token: QueryJobId,
depth_limit: bool,
diagnostics: Option<&Lock<ThinVec<DiagInner>>>,
side_effects: Option<&Lock<QuerySideEffects>>,
compute: impl FnOnce() -> R,
) -> R {
// The `TyCtxt` stored in TLS has the same global interner lifetime
@@ -142,7 +140,7 @@ impl QueryContext for QueryCtxt<'_> {
let new_icx = ImplicitCtxt {
tcx: self.tcx,
query: Some(token),
diagnostics,
side_effects,
query_depth: current_icx.query_depth + depth_limit as usize,
task_deps: current_icx.task_deps,
};
@@ -174,6 +172,21 @@ impl QueryContext for QueryCtxt<'_> {
crate_name: self.crate_name(LOCAL_CRATE),
});
}

#[tracing::instrument(level = "trace", skip(self, side_effects))]
fn apply_side_effects(self, side_effects: QuerySideEffects) {
let dcx = self.dep_context().sess().dcx();
let QuerySideEffects { diagnostics, definitions } = side_effects;

for diagnostic in diagnostics {
dcx.emit_diagnostic(diagnostic);
}

for DefIdInfo { parent, data, hash } in definitions {
let (_def_id, h) = self.tcx.untracked().definitions.write().create_def(parent, data);
debug_assert_eq!(h, hash);
}
}
}

pub(super) fn try_mark_green<'tcx>(tcx: TyCtxt<'tcx>, dep_node: &dep_graph::DepNode) -> bool {
41 changes: 33 additions & 8 deletions compiler/rustc_query_system/src/dep_graph/graph.rs
Original file line number Diff line number Diff line change
@@ -11,7 +11,7 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_data_structures::profiling::{QueryInvocationId, SelfProfilerRef};
use rustc_data_structures::sharded::{self, Sharded};
use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
use rustc_data_structures::sync::{AtomicU32, AtomicU64, Lock, Lrc};
use rustc_data_structures::sync::{AtomicU32, AtomicU64, AtomicUsize, Lock, Lrc};
use rustc_data_structures::unord::UnordMap;
use rustc_index::IndexVec;
use rustc_macros::{Decodable, Encodable};
@@ -224,6 +224,15 @@ impl<D: Deps> DepGraph<D> {
D::with_deps(TaskDepsRef::Ignore, op)
}

pub(crate) fn with_replay<R>(
&self,
prev_side_effects: &QuerySideEffects,
created_def_ids: &AtomicUsize,
op: impl FnOnce() -> R,
) -> R {
D::with_deps(TaskDepsRef::Replay { prev_side_effects, created_def_ids }, op)
}

/// Used to wrap the deserialization of a query result from disk,
/// This method enforces that no new `DepNodes` are created during
/// query result deserialization.
@@ -278,6 +287,7 @@ impl<D: Deps> DepGraph<D> {
}

#[inline(always)]
/// A helper for `codegen_cranelift`.
pub fn with_task<Ctxt: HasDepContext<Deps = D>, A: Debug, R>(
&self,
key: DepNode,
@@ -477,6 +487,12 @@ impl<D: Deps> DepGraph<D> {
return;
}
TaskDepsRef::Ignore => return,
// We don't need to record dependencies when rerunning a query
// because we have no disk cache entry to load. The dependencies
// are preserved.
// FIXME: assert that the dependencies don't change instead of
// recording them.
TaskDepsRef::Replay { .. } => return,
TaskDepsRef::Forbid => {
// Reading is forbidden in this context. ICE with a useful error message.
panic_on_forbidden_read(data, dep_node_index)
@@ -583,6 +599,7 @@ impl<D: Deps> DepGraph<D> {
edges.push(DepNodeIndex::FOREVER_RED_NODE);
}
TaskDepsRef::Ignore => {}
TaskDepsRef::Replay { .. } => {}
TaskDepsRef::Forbid => {
panic!("Cannot summarize when dependencies are not recorded.")
}
@@ -892,7 +909,7 @@ impl<D: Deps> DepGraphData<D> {
Some(dep_node_index)
}

/// Atomically emits some loaded diagnostics.
/// Atomically emits some loaded side effects.
/// This may be called concurrently on multiple threads for the same dep node.
#[cold]
#[inline(never)]
@@ -908,14 +925,10 @@ impl<D: Deps> DepGraphData<D> {
// We were the first to insert the node in the set so this thread
// must process side effects

// Promote the previous diagnostics to the current session.
// Promote the previous side effects to the current session.
qcx.store_side_effects(dep_node_index, side_effects.clone());

let dcx = qcx.dep_context().sess().dcx();

for diagnostic in side_effects.diagnostics {
dcx.emit_diagnostic(diagnostic);
}
qcx.apply_side_effects(side_effects);
}
}
}
@@ -1288,6 +1301,18 @@ pub enum TaskDepsRef<'a> {
/// to ensure that the decoding process doesn't itself
/// require the execution of any queries.
Forbid,
/// Side effects from the previous run made available to
/// queries when they are reexecuted because their result was not
/// available in the cache. Whenever the query creates a new `DefId`,
/// it is checked against the entries in `QuerySideEffects::definitions`
/// to ensure that the new `DefId`s are the same as the ones that were
/// created the last time the query was executed.
Replay {
prev_side_effects: &'a QuerySideEffects,
/// Every new `DefId` is pushed here so we can check
/// that they match the cached ones.
created_def_ids: &'a AtomicUsize,
},
}

#[derive(Debug)]
Loading