Skip to content

Cache local DefId-keyed queries without hashing #119977

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
Jan 17, 2024
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
3 changes: 2 additions & 1 deletion compiler/rustc_middle/src/query/keys.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use crate::ty::{self, Ty, TyCtxt};
use crate::ty::{GenericArg, GenericArgsRef};
use rustc_hir::def_id::{CrateNum, DefId, LocalDefId, LocalModDefId, ModDefId, LOCAL_CRATE};
use rustc_hir::hir_id::{HirId, OwnerId};
use rustc_query_system::query::DefIdCacheSelector;
use rustc_query_system::query::{DefaultCacheSelector, SingleCacheSelector, VecCacheSelector};
use rustc_span::symbol::{Ident, Symbol};
use rustc_span::{Span, DUMMY_SP};
Expand Down Expand Up @@ -152,7 +153,7 @@ impl Key for LocalDefId {
}

impl Key for DefId {
type CacheSelector = DefaultCacheSelector<Self>;
type CacheSelector = DefIdCacheSelector;
Copy link
Contributor

Choose a reason for hiding this comment

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

ModDefId below could use DefIdCacheSelector too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer to leave that to follow up (want separate perf run on it - ModDefId sounds like it'll be much more commonly quite sparse as a % of indexes, so it may not be a win to move it like this).


fn default_span(&self, tcx: TyCtxt<'_>) -> Span {
tcx.def_span(*self)
Expand Down
79 changes: 78 additions & 1 deletion compiler/rustc_query_system/src/query/caches.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@ use crate::dep_graph::DepNodeIndex;

use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::sharded::{self, Sharded};
use rustc_data_structures::sync::OnceLock;
use rustc_data_structures::sync::{Lock, OnceLock};
use rustc_hir::def_id::LOCAL_CRATE;
use rustc_index::{Idx, IndexVec};
use rustc_span::def_id::DefId;
use rustc_span::def_id::DefIndex;
use std::fmt::Debug;
use std::hash::Hash;
use std::marker::PhantomData;
Expand Down Expand Up @@ -148,6 +151,8 @@ where

#[inline(always)]
fn lookup(&self, key: &K) -> Option<(V, DepNodeIndex)> {
// FIXME: lock_shard_by_hash will use high bits which are usually zero in the index() passed
// here. This makes sharding essentially useless, always selecting the zero'th shard.
let lock = self.cache.lock_shard_by_hash(key.index() as u64);
if let Some(Some(value)) = lock.get(*key) { Some(*value) } else { None }
}
Expand All @@ -168,3 +173,75 @@ where
}
}
}

pub struct DefIdCacheSelector;

impl<'tcx, V: 'tcx> CacheSelector<'tcx, V> for DefIdCacheSelector {
type Cache = DefIdCache<V>
where
V: Copy;
}

pub struct DefIdCache<V> {
/// Stores the local DefIds in a dense map. Local queries are much more often dense, so this is
/// a win over hashing query keys at marginal memory cost (~5% at most) compared to FxHashMap.
///
/// The second element of the tuple is the set of keys actually present in the IndexVec, used
/// for faster iteration in `iter()`.
// FIXME: This may want to be sharded, like VecCache. However *how* to shard an IndexVec isn't
// super clear; VecCache is effectively not sharded today (see FIXME there). For now just omit
// that complexity here.
local: Lock<(IndexVec<DefIndex, Option<(V, DepNodeIndex)>>, Vec<DefIndex>)>,
foreign: DefaultCache<DefId, V>,
}

impl<V> Default for DefIdCache<V> {
fn default() -> Self {
DefIdCache { local: Default::default(), foreign: Default::default() }
}
}

impl<V> QueryCache for DefIdCache<V>
where
V: Copy,
{
type Key = DefId;
type Value = V;

#[inline(always)]
fn lookup(&self, key: &DefId) -> Option<(V, DepNodeIndex)> {
if key.krate == LOCAL_CRATE {
let cache = self.local.lock();
cache.0.get(key.index).and_then(|v| *v)
} else {
self.foreign.lookup(key)
}
}

#[inline]
fn complete(&self, key: DefId, value: V, index: DepNodeIndex) {
if key.krate == LOCAL_CRATE {
let mut cache = self.local.lock();
let (cache, present) = &mut *cache;
let slot = cache.ensure_contains_elem(key.index, Default::default);
if slot.is_none() {
// FIXME: Only store the present set when running in incremental mode. `iter` is not
// used outside of saving caches to disk and self-profile.
present.push(key.index);
}
*slot = Some((value, index));
} else {
self.foreign.complete(key, value, index)
}
}

fn iter(&self, f: &mut dyn FnMut(&Self::Key, &Self::Value, DepNodeIndex)) {
let guard = self.local.lock();
let (cache, present) = &*guard;
for &idx in present.iter() {
let value = cache[idx].unwrap();
f(&DefId { krate: LOCAL_CRATE, index: idx }, &value.0, value.1);
}
self.foreign.iter(f);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should guard be dropped before iterating on foreign?

Copy link
Member Author

@Mark-Simulacrum Mark-Simulacrum Jan 15, 2024

Choose a reason for hiding this comment

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

Shouldn't matter -- the callbacks are usually either math or serializing stuff to disk. We'd previously hold the lock for the whole map (modulo sharding) while calling the iteration function, whether we hold two locks or not I think can't really impact anything?

}
}
3 changes: 2 additions & 1 deletion compiler/rustc_query_system/src/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ pub use self::job::{

mod caches;
pub use self::caches::{
CacheSelector, DefaultCacheSelector, QueryCache, SingleCacheSelector, VecCacheSelector,
CacheSelector, DefIdCacheSelector, DefaultCacheSelector, QueryCache, SingleCacheSelector,
VecCacheSelector,
};

mod config;
Expand Down