From 128f2224afa6bf135a674fc14e5c825f2c76c93f Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Sat, 4 Feb 2023 15:16:59 +0000 Subject: [PATCH] Remove `OnHit` callback from query caches. This is not useful now that query results are `Copy`. --- compiler/rustc_middle/src/ty/query.rs | 62 ++++++---------- .../rustc_query_system/src/query/caches.rs | 73 +++++-------------- .../rustc_query_system/src/query/config.rs | 2 +- .../rustc_query_system/src/query/plumbing.rs | 54 +++++++------- 4 files changed, 66 insertions(+), 125 deletions(-) diff --git a/compiler/rustc_middle/src/ty/query.rs b/compiler/rustc_middle/src/ty/query.rs index 1be819ca610..7151b79c5ab 100644 --- a/compiler/rustc_middle/src/ty/query.rs +++ b/compiler/rustc_middle/src/ty/query.rs @@ -106,16 +106,6 @@ pub fn try_mark_green(self, dep_node: &dep_graph::DepNode) -> bool { } } -/// Helper for `TyCtxtEnsure` to avoid a closure. -#[inline(always)] -fn noop(_: &T) {} - -/// Helper to ensure that queries only return `Copy` types. -#[inline(always)] -fn copy(x: &T) -> T { - *x -} - macro_rules! query_helper_param_ty { (DefId) => { impl IntoQueryParam }; (LocalDefId) => { impl IntoQueryParam }; @@ -225,14 +215,10 @@ pub fn $name(self, key: query_helper_param_ty!($($K)*)) { let key = key.into_query_param(); opt_remap_env_constness!([$($modifiers)*][key]); - let cached = try_get_cached(self.tcx, &self.tcx.query_caches.$name, &key, noop); - - match cached { - Ok(()) => return, - Err(()) => (), - } - - self.tcx.queries.$name(self.tcx, DUMMY_SP, key, QueryMode::Ensure); + match try_get_cached(self.tcx, &self.tcx.query_caches.$name, &key) { + Some(_) => return, + None => self.tcx.queries.$name(self.tcx, DUMMY_SP, key, QueryMode::Ensure), + }; })* } @@ -254,14 +240,10 @@ pub fn $name(self, key: query_helper_param_ty!($($K)*)) -> query_stored::$name<' let key = key.into_query_param(); opt_remap_env_constness!([$($modifiers)*][key]); - let cached = try_get_cached(self.tcx, &self.tcx.query_caches.$name, &key, copy); - - match cached { - Ok(value) => return value, - Err(()) => (), + match try_get_cached(self.tcx, &self.tcx.query_caches.$name, &key) { + Some(value) => value, + None => self.tcx.queries.$name(self.tcx, self.span, key, QueryMode::Get).unwrap(), } - - self.tcx.queries.$name(self.tcx, self.span, key, QueryMode::Get).unwrap() })* } @@ -353,27 +335,25 @@ pub fn $name(self, value: $V) -> query_stored::$name<'tcx> { let tcx = self.tcx; let cache = &tcx.query_caches.$name; - let cached = try_get_cached(tcx, cache, &key, copy); - - match cached { - Ok(old) => { + match try_get_cached(tcx, cache, &key) { + Some(old) => { bug!( "Trying to feed an already recorded value for query {} key={key:?}:\nold value: {old:?}\nnew value: {value:?}", stringify!($name), - ); + ) + } + None => { + let dep_node = dep_graph::DepNode::construct(tcx, dep_graph::DepKind::$name, &key); + let dep_node_index = tcx.dep_graph.with_feed_task( + dep_node, + tcx, + key, + &value, + hash_result!([$($modifiers)*]), + ); + cache.complete(key, value, dep_node_index) } - Err(()) => (), } - - let dep_node = dep_graph::DepNode::construct(tcx, dep_graph::DepKind::$name, &key); - let dep_node_index = tcx.dep_graph.with_feed_task( - dep_node, - tcx, - key, - &value, - hash_result!([$($modifiers)*]), - ); - cache.complete(key, value, dep_node_index) } })* } diff --git a/compiler/rustc_query_system/src/query/caches.rs b/compiler/rustc_query_system/src/query/caches.rs index 77d0d0314fc..21c89cbc4f1 100644 --- a/compiler/rustc_query_system/src/query/caches.rs +++ b/compiler/rustc_query_system/src/query/caches.rs @@ -16,13 +16,13 @@ pub trait CacheSelector<'tcx, V> { type Cache where - V: Clone; + V: Copy; type ArenaCache; } pub trait QueryStorage { type Value: Debug; - type Stored: Clone; + type Stored: Copy; /// Store a value without putting it in the cache. /// This is meant to be used with cycle errors. @@ -36,14 +36,7 @@ pub trait QueryCache: QueryStorage + Sized { /// 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( - &self, - key: &Self::Key, - // `on_hit` can be called while holding a lock to the query state shard. - on_hit: OnHit, - ) -> Result - where - OnHit: FnOnce(&Self::Stored, DepNodeIndex) -> R; + fn lookup(&self, key: &Self::Key) -> Option<(Self::Stored, DepNodeIndex)>; fn complete(&self, key: Self::Key, value: Self::Value, index: DepNodeIndex) -> Self::Stored; @@ -55,7 +48,7 @@ fn lookup( impl<'tcx, K: Eq + Hash, V: 'tcx> CacheSelector<'tcx, V> for DefaultCacheSelector { type Cache = DefaultCache where - V: Clone; + V: Copy; type ArenaCache = ArenaCache<'tcx, K, V>; } @@ -72,7 +65,7 @@ fn default() -> Self { } } -impl QueryStorage for DefaultCache { +impl QueryStorage for DefaultCache { type Value = V; type Stored = V; @@ -86,15 +79,12 @@ fn store_nocache(&self, value: Self::Value) -> Self::Stored { impl QueryCache for DefaultCache where K: Eq + Hash + Clone + Debug, - V: Clone + Debug, + V: Copy + Debug, { type Key = K; #[inline(always)] - fn lookup(&self, key: &K, on_hit: OnHit) -> Result - where - OnHit: FnOnce(&V, DepNodeIndex) -> R, - { + fn lookup(&self, key: &K) -> Option<(V, DepNodeIndex)> { let key_hash = sharded::make_hash(key); #[cfg(parallel_compiler)] let lock = self.cache.get_shard_by_hash(key_hash).lock(); @@ -102,12 +92,7 @@ fn lookup(&self, key: &K, on_hit: OnHit) -> Result let lock = self.cache.lock(); let result = lock.raw_entry().from_key_hashed_nocheck(key_hash, key); - if let Some((_, value)) = result { - let hit_result = on_hit(&value.0, value.1); - Ok(hit_result) - } else { - Err(()) - } + if let Some((_, value)) = result { Some(*value) } else { None } } #[inline] @@ -176,10 +161,7 @@ impl<'tcx, K, V: 'tcx> QueryCache for ArenaCache<'tcx, K, V> type Key = K; #[inline(always)] - fn lookup(&self, key: &K, on_hit: OnHit) -> Result - where - OnHit: FnOnce(&&'tcx V, DepNodeIndex) -> R, - { + fn lookup(&self, key: &K) -> Option<(&'tcx V, DepNodeIndex)> { let key_hash = sharded::make_hash(key); #[cfg(parallel_compiler)] let lock = self.cache.get_shard_by_hash(key_hash).lock(); @@ -187,12 +169,7 @@ fn lookup(&self, key: &K, on_hit: OnHit) -> Result let lock = self.cache.lock(); let result = lock.raw_entry().from_key_hashed_nocheck(key_hash, key); - if let Some((_, value)) = result { - let hit_result = on_hit(&&value.0, value.1); - Ok(hit_result) - } else { - Err(()) - } + if let Some((_, value)) = result { Some((&value.0, value.1)) } else { None } } #[inline] @@ -234,7 +211,7 @@ fn iter(&self, f: &mut dyn FnMut(&Self::Key, &Self::Value, DepNodeIndex)) { impl<'tcx, K: Idx, V: 'tcx> CacheSelector<'tcx, V> for VecCacheSelector { type Cache = VecCache where - V: Clone; + V: Copy; type ArenaCache = VecArenaCache<'tcx, K, V>; } @@ -251,7 +228,7 @@ fn default() -> Self { } } -impl QueryStorage for VecCache { +impl QueryStorage for VecCache { type Value = V; type Stored = V; @@ -265,25 +242,17 @@ fn store_nocache(&self, value: Self::Value) -> Self::Stored { impl QueryCache for VecCache where K: Eq + Idx + Clone + Debug, - V: Clone + Debug, + V: Copy + Debug, { type Key = K; #[inline(always)] - fn lookup(&self, key: &K, on_hit: OnHit) -> Result - where - OnHit: FnOnce(&V, DepNodeIndex) -> R, - { + fn lookup(&self, key: &K) -> Option<(V, DepNodeIndex)> { #[cfg(parallel_compiler)] let lock = self.cache.get_shard_by_hash(key.index() as u64).lock(); #[cfg(not(parallel_compiler))] let lock = self.cache.lock(); - if let Some(Some(value)) = lock.get(*key) { - let hit_result = on_hit(&value.0, value.1); - Ok(hit_result) - } else { - Err(()) - } + if let Some(Some(value)) = lock.get(*key) { Some(*value) } else { None } } #[inline] @@ -357,20 +326,12 @@ impl<'tcx, K, V: 'tcx> QueryCache for VecArenaCache<'tcx, K, V> type Key = K; #[inline(always)] - fn lookup(&self, key: &K, on_hit: OnHit) -> Result - where - OnHit: FnOnce(&&'tcx V, DepNodeIndex) -> R, - { + fn lookup(&self, key: &K) -> Option<(&'tcx V, DepNodeIndex)> { #[cfg(parallel_compiler)] let lock = self.cache.get_shard_by_hash(key.index() as u64).lock(); #[cfg(not(parallel_compiler))] let lock = self.cache.lock(); - if let Some(Some(value)) = lock.get(*key) { - let hit_result = on_hit(&&value.0, value.1); - Ok(hit_result) - } else { - Err(()) - } + if let Some(Some(value)) = lock.get(*key) { Some((&value.0, value.1)) } else { None } } #[inline] diff --git a/compiler/rustc_query_system/src/query/config.rs b/compiler/rustc_query_system/src/query/config.rs index 8c0330e438d..a28e45a5c08 100644 --- a/compiler/rustc_query_system/src/query/config.rs +++ b/compiler/rustc_query_system/src/query/config.rs @@ -21,7 +21,7 @@ pub trait QueryConfig { type Key: DepNodeParams + Eq + Hash + Clone + Debug; type Value: Debug; - type Stored: Debug + Clone + std::borrow::Borrow; + type Stored: Debug + Copy + std::borrow::Borrow; type Cache: QueryCache; diff --git a/compiler/rustc_query_system/src/query/plumbing.rs b/compiler/rustc_query_system/src/query/plumbing.rs index b3b939eae88..bf380f6e2d3 100644 --- a/compiler/rustc_query_system/src/query/plumbing.rs +++ b/compiler/rustc_query_system/src/query/plumbing.rs @@ -130,7 +130,7 @@ fn mk_cycle( where Qcx: QueryContext + crate::query::HasDepContext, V: std::fmt::Debug + Value, - R: Clone, + R: Copy, { let error = report_cycle(qcx.dep_context().sess(), &cycle_error); let value = handle_cycle_error(*qcx.dep_context(), &cycle_error, error, handler); @@ -339,25 +339,21 @@ enum TryGetJob<'tcx, K, D> /// which will be used if the query is not in the cache and we need /// to compute it. #[inline] -pub fn try_get_cached( - tcx: Tcx, - cache: &C, - key: &C::Key, - // `on_hit` can be called while holding a lock to the query cache - on_hit: OnHit, -) -> Result +pub fn try_get_cached(tcx: Tcx, cache: &C, key: &C::Key) -> Option where C: QueryCache, Tcx: DepContext, - OnHit: FnOnce(&C::Stored) -> R, { - cache.lookup(&key, |value, index| { - if std::intrinsics::unlikely(tcx.profiler().enabled()) { - tcx.profiler().query_cache_hit(index.into()); + match cache.lookup(&key) { + Some((value, index)) => { + if std::intrinsics::unlikely(tcx.profiler().enabled()) { + tcx.profiler().query_cache_hit(index.into()); + } + tcx.dep_graph().read_index(index); + Some(value) } - tcx.dep_graph().read_index(index); - on_hit(value) - }) + None => None, + } } fn try_execute_query( @@ -379,17 +375,25 @@ fn try_execute_query( if Q::FEEDABLE { // We may have put a value inside the cache from inside the execution. // Verify that it has the same hash as what we have now, to ensure consistency. - let _ = cache.lookup(&key, |cached_result, _| { + if let Some((cached_result, _)) = cache.lookup(&key) { let hasher = Q::HASH_RESULT.expect("feedable forbids no_hash"); - let old_hash = qcx.dep_context().with_stable_hashing_context(|mut hcx| hasher(&mut hcx, cached_result.borrow())); - let new_hash = qcx.dep_context().with_stable_hashing_context(|mut hcx| hasher(&mut hcx, &result)); + let old_hash = qcx.dep_context().with_stable_hashing_context(|mut hcx| { + hasher(&mut hcx, cached_result.borrow()) + }); + let new_hash = qcx + .dep_context() + .with_stable_hashing_context(|mut hcx| hasher(&mut hcx, &result)); debug_assert_eq!( - old_hash, new_hash, + old_hash, + new_hash, "Computed query value for {:?}({:?}) is inconsistent with fed value,\ncomputed={:#?}\nfed={:#?}", - Q::DEP_KIND, key, result, cached_result, + Q::DEP_KIND, + key, + result, + cached_result, ); - }); + } } let result = job.complete(cache, result, dep_node_index); (result, Some(dep_node_index)) @@ -771,15 +775,11 @@ pub fn force_query(qcx: Qcx, key: Q::Key, dep_node: DepNode return, - Err(()) => {} + return; } let state = Q::query_state(qcx);