Skip to content

Commit

Permalink
Auto merge of #77871 - Julian-Wollersberger:less-query-context, r=oli…
Browse files Browse the repository at this point in the history
…-obk

Make fewer types generic over QueryContext

While trying to refactor `rustc_query_system::query::QueryContext` to make it dyn-safe, I noticed some smaller things:
* QueryConfig doesn't need to be generic over QueryContext
* ~~The `kind` field on QueryJobId is unused~~
* Some unnecessary where clauses
* Many types in `job.rs` where generic over `QueryContext` but only needed `QueryContext::Query`.
  If handle_cycle_error() could be refactored to not take `error: CycleError<CTX::Query>`, all those bounds could be removed as well.

Changing `find_cycle_in_stack()` in job.rs to not take a `tcx` argument is the only functional change here. Everything else is just updating type signatures. (aka compile-error driven development ^^)

~~Currently there is a weird bug where memory usage suddenly skyrockets when running UI tests. I'll investigate that tomorrow.
A perf run probably won't make sense before that is fixed.~~

EDIT: `kind` actually is used by `Eq`, and re-adding it fixed the memory issue.
  • Loading branch information
bors committed Oct 22, 2020
2 parents 6b9fbf2 + 52cedca commit 500ddc5
Show file tree
Hide file tree
Showing 8 changed files with 216 additions and 174 deletions.
16 changes: 9 additions & 7 deletions compiler/rustc_middle/src/ty/query/plumbing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ impl QueryContext for TyCtxt<'tcx> {

fn try_collect_active_jobs(
&self,
) -> Option<FxHashMap<QueryJobId<Self::DepKind>, QueryJobInfo<Self>>> {
) -> Option<FxHashMap<QueryJobId<Self::DepKind>, QueryJobInfo<Self::DepKind, Self::Query>>>
{
self.queries.try_collect_active_jobs()
}

Expand Down Expand Up @@ -353,7 +354,7 @@ macro_rules! define_queries_inner {
$(pub type $name<$tcx> = $V;)*
}

$(impl<$tcx> QueryConfig<TyCtxt<$tcx>> for queries::$name<$tcx> {
$(impl<$tcx> QueryConfig for queries::$name<$tcx> {
type Key = $($K)*;
type Value = $V;
type Stored = <
Expand All @@ -372,7 +373,7 @@ macro_rules! define_queries_inner {
type Cache = query_storage!([$($modifiers)*][$($K)*, $V]);

#[inline(always)]
fn query_state<'a>(tcx: TyCtxt<$tcx>) -> &'a QueryState<TyCtxt<$tcx>, Self::Cache> {
fn query_state<'a>(tcx: TyCtxt<$tcx>) -> &'a QueryState<crate::dep_graph::DepKind, <TyCtxt<$tcx> as QueryContext>::Query, Self::Cache> {
&tcx.queries.$name
}

Expand Down Expand Up @@ -454,7 +455,7 @@ macro_rules! define_queries_inner {
#[inline(always)]
#[must_use]
pub fn $name(self, key: query_helper_param_ty!($($K)*))
-> <queries::$name<$tcx> as QueryConfig<TyCtxt<$tcx>>>::Stored
-> <queries::$name<$tcx> as QueryConfig>::Stored
{
self.at(DUMMY_SP).$name(key.into_query_param())
})*
Expand Down Expand Up @@ -493,7 +494,7 @@ macro_rules! define_queries_inner {
$($(#[$attr])*
#[inline(always)]
pub fn $name(self, key: query_helper_param_ty!($($K)*))
-> <queries::$name<$tcx> as QueryConfig<TyCtxt<$tcx>>>::Stored
-> <queries::$name<$tcx> as QueryConfig>::Stored
{
get_query::<queries::$name<'_>, _>(self.tcx, self.span, key.into_query_param())
})*
Expand Down Expand Up @@ -527,7 +528,8 @@ macro_rules! define_queries_struct {
fallback_extern_providers: Box<Providers>,

$($(#[$attr])* $name: QueryState<
TyCtxt<$tcx>,
crate::dep_graph::DepKind,
<TyCtxt<$tcx> as QueryContext>::Query,
<queries::$name<$tcx> as QueryAccessors<TyCtxt<'tcx>>>::Cache,
>,)*
}
Expand All @@ -548,7 +550,7 @@ macro_rules! define_queries_struct {

pub(crate) fn try_collect_active_jobs(
&self
) -> Option<FxHashMap<QueryJobId<crate::dep_graph::DepKind>, QueryJobInfo<TyCtxt<'tcx>>>> {
) -> Option<FxHashMap<QueryJobId<crate::dep_graph::DepKind>, QueryJobInfo<crate::dep_graph::DepKind, <TyCtxt<$tcx> as QueryContext>::Query>>> {
let mut jobs = FxHashMap::default();

$(
Expand Down
5 changes: 2 additions & 3 deletions compiler/rustc_middle/src/ty/query/profiling_support.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@ use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::profiling::SelfProfiler;
use rustc_hir::def_id::{CrateNum, DefId, DefIndex, LocalDefId, CRATE_DEF_INDEX, LOCAL_CRATE};
use rustc_hir::definitions::DefPathData;
use rustc_query_system::query::QueryCache;
use rustc_query_system::query::QueryState;
use rustc_query_system::query::{QueryCache, QueryContext, QueryState};
use std::fmt::Debug;
use std::io::Write;

Expand Down Expand Up @@ -231,7 +230,7 @@ where
pub(super) fn alloc_self_profile_query_strings_for_query_cache<'tcx, C>(
tcx: TyCtxt<'tcx>,
query_name: &'static str,
query_state: &QueryState<TyCtxt<'tcx>, C>,
query_state: &QueryState<crate::dep_graph::DepKind, <TyCtxt<'tcx> as QueryContext>::Query, C>,
string_cache: &mut QueryKeyStringCache,
) where
C: QueryCache,
Expand Down
18 changes: 10 additions & 8 deletions compiler/rustc_middle/src/ty/query/stats.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
use crate::ty::query::queries;
use crate::ty::TyCtxt;
use rustc_hir::def_id::{DefId, LOCAL_CRATE};
use rustc_query_system::query::QueryCache;
use rustc_query_system::query::QueryState;
use rustc_query_system::query::{QueryAccessors, QueryContext};
use rustc_query_system::query::{QueryAccessors, QueryCache, QueryContext, QueryState};

use std::any::type_name;
use std::hash::Hash;
use std::mem;
#[cfg(debug_assertions)]
use std::sync::atomic::Ordering;
Expand Down Expand Up @@ -38,10 +37,12 @@ struct QueryStats {
local_def_id_keys: Option<usize>,
}

fn stats<CTX: QueryContext, C: QueryCache>(
name: &'static str,
map: &QueryState<CTX, C>,
) -> QueryStats {
fn stats<D, Q, C>(name: &'static str, map: &QueryState<D, Q, C>) -> QueryStats
where
D: Copy + Clone + Eq + Hash,
Q: Clone,
C: QueryCache,
{
let mut stats = QueryStats {
name,
#[cfg(debug_assertions)]
Expand Down Expand Up @@ -127,7 +128,8 @@ macro_rules! print_stats {

$($(
queries.push(stats::<
TyCtxt<'_>,
crate::dep_graph::DepKind,
<TyCtxt<'_> as QueryContext>::Query,
<queries::$name<'_> as QueryAccessors<TyCtxt<'_>>>::Cache,
>(
stringify!($name),
Expand Down
33 changes: 20 additions & 13 deletions compiler/rustc_query_system/src/query/caches.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
use crate::dep_graph::DepNodeIndex;
use crate::query::plumbing::{QueryLookup, QueryState};
use crate::query::QueryContext;

use rustc_arena::TypedArena;
use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::sharded::Sharded;
use rustc_data_structures::sync::WorkerLocal;
use std::default::Default;
use std::fmt::Debug;
use std::hash::Hash;
use std::marker::PhantomData;

Expand All @@ -24,24 +24,24 @@ pub trait QueryStorage: Default {
}

pub trait QueryCache: QueryStorage {
type Key: Hash;
type Key: Hash + Eq + Clone + Debug;
type Sharded: Default;

/// Checks if the query is already computed and in the cache.
/// It returns the shard index and a lock guard to the shard,
/// which will be used if the query is not in the cache and we need
/// to compute it.
fn lookup<CTX: QueryContext, R, OnHit, OnMiss>(
fn lookup<D, Q, R, OnHit, OnMiss>(
&self,
state: &QueryState<CTX, Self>,
state: &QueryState<D, Q, Self>,
key: Self::Key,
// `on_hit` can be called while holding a lock to the query state shard.
on_hit: OnHit,
on_miss: OnMiss,
) -> R
where
OnHit: FnOnce(&Self::Stored, DepNodeIndex) -> R,
OnMiss: FnOnce(Self::Key, QueryLookup<'_, CTX, Self::Key, Self::Sharded>) -> R;
OnMiss: FnOnce(Self::Key, QueryLookup<'_, D, Q, Self::Key, Self::Sharded>) -> R;

fn complete(
&self,
Expand Down Expand Up @@ -86,21 +86,25 @@ impl<K: Eq + Hash, V: Clone> QueryStorage for DefaultCache<K, V> {
}
}

impl<K: Eq + Hash, V: Clone> QueryCache for DefaultCache<K, V> {
impl<K, V> QueryCache for DefaultCache<K, V>
where
K: Eq + Hash + Clone + Debug,
V: Clone,
{
type Key = K;
type Sharded = FxHashMap<K, (V, DepNodeIndex)>;

#[inline(always)]
fn lookup<CTX: QueryContext, R, OnHit, OnMiss>(
fn lookup<D, Q, R, OnHit, OnMiss>(
&self,
state: &QueryState<CTX, Self>,
state: &QueryState<D, Q, Self>,
key: K,
on_hit: OnHit,
on_miss: OnMiss,
) -> R
where
OnHit: FnOnce(&V, DepNodeIndex) -> R,
OnMiss: FnOnce(K, QueryLookup<'_, CTX, K, Self::Sharded>) -> R,
OnMiss: FnOnce(K, QueryLookup<'_, D, Q, K, Self::Sharded>) -> R,
{
let mut lookup = state.get_lookup(&key);
let lock = &mut *lookup.lock;
Expand Down Expand Up @@ -164,21 +168,24 @@ impl<'tcx, K: Eq + Hash, V: 'tcx> QueryStorage for ArenaCache<'tcx, K, V> {
}
}

impl<'tcx, K: Eq + Hash, V: 'tcx> QueryCache for ArenaCache<'tcx, K, V> {
impl<'tcx, K, V: 'tcx> QueryCache for ArenaCache<'tcx, K, V>
where
K: Eq + Hash + Clone + Debug,
{
type Key = K;
type Sharded = FxHashMap<K, &'tcx (V, DepNodeIndex)>;

#[inline(always)]
fn lookup<CTX: QueryContext, R, OnHit, OnMiss>(
fn lookup<D, Q, R, OnHit, OnMiss>(
&self,
state: &QueryState<CTX, Self>,
state: &QueryState<D, Q, Self>,
key: K,
on_hit: OnHit,
on_miss: OnMiss,
) -> R
where
OnHit: FnOnce(&&'tcx V, DepNodeIndex) -> R,
OnMiss: FnOnce(K, QueryLookup<'_, CTX, K, Self::Sharded>) -> R,
OnMiss: FnOnce(K, QueryLookup<'_, D, Q, K, Self::Sharded>) -> R,
{
let mut lookup = state.get_lookup(&key);
let lock = &mut *lookup.lock;
Expand Down
8 changes: 3 additions & 5 deletions compiler/rustc_query_system/src/query/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,7 @@ use std::borrow::Cow;
use std::fmt::Debug;
use std::hash::Hash;

// The parameter `CTX` is required in librustc_middle:
// implementations may need to access the `'tcx` lifetime in `CTX = TyCtxt<'tcx>`.
pub trait QueryConfig<CTX> {
pub trait QueryConfig {
const NAME: &'static str;
const CATEGORY: ProfileCategory;

Expand Down Expand Up @@ -70,15 +68,15 @@ impl<CTX: QueryContext, K, V> QueryVtable<CTX, K, V> {
}
}

pub trait QueryAccessors<CTX: QueryContext>: QueryConfig<CTX> {
pub trait QueryAccessors<CTX: QueryContext>: QueryConfig {
const ANON: bool;
const EVAL_ALWAYS: bool;
const DEP_KIND: CTX::DepKind;

type Cache: QueryCache<Key = Self::Key, Stored = Self::Stored, Value = Self::Value>;

// Don't use this method to access query results, instead use the methods on TyCtxt
fn query_state<'a>(tcx: CTX) -> &'a QueryState<CTX, Self::Cache>;
fn query_state<'a>(tcx: CTX) -> &'a QueryState<CTX::DepKind, CTX::Query, Self::Cache>;

fn to_dep_node(tcx: CTX, key: &Self::Key) -> DepNode<CTX::DepKind>
where
Expand Down
Loading

0 comments on commit 500ddc5

Please sign in to comment.