From d4bcc9b76e1a8dc9b76204efb0cd4b1d656de98c Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Tue, 25 Jun 2024 16:25:37 +0200 Subject: [PATCH] Remove `Hash` cache in `Node` (#872) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hashing is much less important to apollo-compiler as it was when we were using Salsa. Many of the structs stored in `Node<_>` don’t even implement `Hash` since they contain `IndexMap` which doesn’t. We lose: potential CPU time if repeatedly hashing nodes. `ast::Definition` implements `PartialEq` and `Hash` by traversing the whole subtree, which can be expensive. We gain: 8 fewer bytes in the heap allocation of every `Node`, and slightly less code complexity. --- crates/apollo-compiler/CHANGELOG.md | 23 ++++++++++ crates/apollo-compiler/src/node.rs | 70 ++++------------------------- 2 files changed, 31 insertions(+), 62 deletions(-) diff --git a/crates/apollo-compiler/CHANGELOG.md b/crates/apollo-compiler/CHANGELOG.md index 6cb8e0006..d5ea73984 100644 --- a/crates/apollo-compiler/CHANGELOG.md +++ b/crates/apollo-compiler/CHANGELOG.md @@ -19,13 +19,36 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm # [x.x.x] (unreleased) - 2024-mm-dd +## BREAKING + +- **Feature REMOVED: `Hash` cache in `Node` - [SimonSapin] in [pull/872].** + `Node` is a reference-counted smart pointer that provides thread-safe shared ownership + for at `T` value together with an optional source location. + In previous beta version of apollo-compiler 1.0 it contained a cache in its `Hash` implementation: + the hash of the `T` value would be computed once and stored, + then `Hash for Node` would hash that hash code. + That functionality is now removed, `Hash for Node` simply forwards to `Hash for T`. + This reduces each `Node` heap allocation by 8 bytes, and reduces code complexity. + + Now that apollo-compiler does not use [Salsa] anymore, + `Hash` is much less central than it used to be. + Many types stored in `Node<_>` don’t implement `Hash` at all + (because they contain an `IndexMap` which doesn’t either). + + Programs that relied on this cache will still compile. + We still consider this change breaking + as they’ll need to build their own cache to maintain performance characteristics. + ## Fixes - **Fix validation error message for missing subselections - [goto-bus-stop] in [pull/865]** It now reports the correct coordinate for the missing subselection. [goto-bus-stop]: https://github.com/goto-bus-stop +[SimonSapin]: https://github.com/SimonSapin [pull/865]: https://github.com/apollographql/apollo-rs/pull/865 +[pull/872]: https://github.com/apollographql/apollo-rs/pull/872 +[Salsa]: https://crates.io/crates/salsa # [1.0.0-beta.17](https://crates.io/crates/apollo-compiler/1.0.0-beta.17) - 2024-06-05 diff --git a/crates/apollo-compiler/src/node.rs b/crates/apollo-compiler/src/node.rs index 8a31a0387..9340de7cd 100644 --- a/crates/apollo-compiler/src/node.rs +++ b/crates/apollo-compiler/src/node.rs @@ -4,16 +4,11 @@ use crate::schema::ComponentOrigin; use crate::SourceMap; use apollo_parser::SyntaxNode; use rowan::TextRange; -use std::collections::hash_map::RandomState; use std::fmt; -use std::hash::BuildHasher; use std::hash::Hash; use std::hash::Hasher; use std::num::NonZeroI64; use std::sync::atomic; -use std::sync::atomic::AtomicU64; -use std::sync::atomic::Ordering; -use std::sync::OnceLock; /// A thread-safe reference-counted smart pointer for GraphQL nodes. /// @@ -21,27 +16,17 @@ use std::sync::OnceLock; /// /// * In addition to `T`, contains an optional [`NodeLocation`]. /// This location notably allows diagnostics to point to relevant parts of parsed input files. -/// * [`std::hash::Hash`] is implemented by caching the result of hashing `T`. /// * Weak references are not supported. -/// -/// For the cache to be correct, **`T` is expected to have a stable hash** -/// a long as no `&mut T` exclusive reference to it is given out. -/// Generally this excludes interior mutability. -/// -/// `Node` cannot implement [`Borrow`][std::borrow::Borrow] because `Node as Hash` -/// produces a result (the hash of the cached hash) different from `T as Hash`. #[derive(serde::Deserialize)] #[serde(from = "T")] pub struct Node(triomphe::Arc>); +#[derive(Clone)] struct NodeInner { location: Option, - hash_cache: AtomicU64, node: T, } -const HASH_NOT_COMPUTED_YET: u64 = 0; - /// The source location of a parsed node: file ID and range within that file. #[derive(Clone, Copy, Hash, PartialEq, Eq)] pub struct NodeLocation { @@ -72,11 +57,7 @@ impl Node { } pub(crate) fn new_opt_location(node: T, location: Option) -> Self { - Self(triomphe::Arc::new(NodeInner { - location, - node, - hash_cache: AtomicU64::new(HASH_NOT_COMPUTED_YET), - })) + Self(triomphe::Arc::new(NodeInner { location, node })) } pub fn location(&self) -> Option { @@ -131,8 +112,6 @@ impl Node { T: Clone, { let inner = triomphe::Arc::make_mut(&mut self.0); - // Clear the cache as mutation through the returned `&mut T` may invalidate it - *inner.hash_cache.get_mut() = HASH_NOT_COMPUTED_YET; // TODO: should the `inner.location` be set to `None` here? // After a node is mutated it is kind of not from that source location anymore &mut inner.node @@ -184,43 +163,20 @@ impl Eq for Node {} impl PartialEq for Node { fn eq(&self, other: &Self) -> bool { self.ptr_eq(other) // fast path - || self.0.node == other.0.node // location and hash_cache not included + || self.0.node == other.0.node // location not included } } impl Hash for Node { fn hash(&self, state: &mut H) { - let hash = self.0.hash_cache.load(Ordering::Relaxed); - if hash != HASH_NOT_COMPUTED_YET { - // cache hit - hash - } else { - hash_slow_path(&self.0) - } - .hash(state) + self.0.node.hash(state) } } -// It is possible for multiple threads to race and take this path for the same `NodeInner`. -// This is ok as they should compute the same result. -// We save on the extra space that `OnceLock` would occupy, -// at the cost of extra computation in the unlikely case of this race. -#[cold] -#[inline(never)] -fn hash_slow_path(inner: &NodeInner) -> u64 { - /// We share a single `BuildHasher` process-wide, - /// not only for the race described above but also - /// so that multiple `HarcInner`’s with the same contents have the same hash. - static SHARED_RANDOM: OnceLock = OnceLock::new(); - let mut hash = SHARED_RANDOM - .get_or_init(RandomState::new) - .hash_one(&inner.node); - // Don’t use the marker value for an actual hash - if hash == HASH_NOT_COMPUTED_YET { - hash += 1 - } - inner.hash_cache.store(hash, Ordering::Relaxed); - hash +impl std::borrow::Borrow for Node { + fn borrow(&self) -> &T { + self + } } impl AsRef for Node { @@ -235,16 +191,6 @@ impl From for Node { } } -impl Clone for NodeInner { - fn clone(&self) -> Self { - Self { - location: self.location, - hash_cache: AtomicU64::new(self.hash_cache.load(Ordering::Relaxed)), - node: self.node.clone(), - } - } -} - impl NodeLocation { pub(crate) fn new(file_id: FileId, node: &'_ SyntaxNode) -> Self { Self {