From dd8d6ef190133bcd376d0267abd0a1191322f149 Mon Sep 17 00:00:00 2001 From: Mac L Date: Wed, 25 Mar 2026 03:02:15 +1100 Subject: [PATCH 1/2] Use HashCell in place of RwLock --- Cargo.toml | 1 - src/hash_cell.rs | 303 +++++++++++++++++++++++++++++++++++++++++++ src/leaf.rs | 14 +- src/lib.rs | 1 + src/packed_leaf.rs | 35 +++-- src/repeat.rs | 29 ++--- src/tests/size_of.rs | 11 +- src/tree.rs | 166 +++++++++--------------- src/utils.rs | 6 +- 9 files changed, 404 insertions(+), 162 deletions(-) create mode 100644 src/hash_cell.rs diff --git a/Cargo.toml b/Cargo.toml index d6247a3..dd2275f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,6 @@ ethereum_hashing = "0.8" ethereum_ssz = "0.10" ethereum_ssz_derive = "0.10" itertools = "0.13.0" -parking_lot = "0.12.1" rayon = "1.5.1" serde = { version = "1.0.0", features = ["derive"] } tree_hash = "0.12" diff --git a/src/hash_cell.rs b/src/hash_cell.rs new file mode 100644 index 0000000..dfd73b4 --- /dev/null +++ b/src/hash_cell.rs @@ -0,0 +1,303 @@ +//! Lock-free write-once hash cache for tree hash values. +//! +//! `HashCell` caches a single `Hash256` using atomics instead of a lock. Reads are +//! non-blocking (`Acquire` load on a bool + four `Relaxed` loads), and `set()` +//! skips the write if the cell is already initialized. +//! +//! ## Memory ordering +//! +//! The first writer does a `Release` store on `ready`, which synchronizes-with the +//! `Acquire` load in `get()`. Subsequent `set()` calls observe `true` and return +//! early without writing. +//! +//! ## Safety invariant +//! +//! All callers must write the same value for a given cell. This is guaranteed by +//! tree hash. All threads compute the same hash for the same tree node. + +use std::fmt; +use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; +use tree_hash::Hash256; + +/// Lock-free write-once hash cache. +pub struct HashCell { + /// Whether the cell has been initialized with a hash value. + ready: AtomicBool, + /// The cached Hash256 hash value, stored as 4 × AtomicU64 for lock-free + /// unconditional writes without data races. + value: [AtomicU64; 4], +} + +impl HashCell { + /// Create a new empty (uninitialized) hash cell. + pub const fn new() -> Self { + HashCell { + ready: AtomicBool::new(false), + value: [ + AtomicU64::new(0), + AtomicU64::new(0), + AtomicU64::new(0), + AtomicU64::new(0), + ], + } + } + + /// Read the cached hash value, if initialized. + /// + /// Returns `None` if the cell has not been initialized. + /// Returns `Some(hash)` if at least one writer has completed `set()`. + #[inline] + pub fn get(&self) -> Option { + if !self.ready.load(Ordering::Acquire) { + return None; + } + // The Acquire load above synchronizes-with the Release store in `set()`, + // guaranteeing that all Relaxed stores to `value` by the first writer are + // visible. Redundant writers store the same bytes, so concurrent reads + // always produce the correct hash. + Some(self.load_value()) + } + + /// Write the hash value if the cell is uninitialized. + /// + /// If another thread has already initialized this cell, the write is skipped. + #[inline] + pub fn set(&self, hash: Hash256) { + if self.ready.load(Ordering::Acquire) { + #[cfg(debug_assertions)] + debug_assert_eq!( + self.load_value(), + hash, + "HashCell written with different value" + ); + return; + } + self.store_value(hash); + self.ready.store(true, Ordering::Release); + } + + /// Reset the cell to the uninitialized state. + /// This avoids constructing a new HashCell and the stale value will never + /// be read. + #[inline] + pub fn clear(&mut self) { + *self.ready.get_mut() = false; + } + + /// Load the cached Hash256 from the four AtomicU64 parts. + #[inline(always)] + fn load_value(&self) -> Hash256 { + let mut bytes = [0u8; 32]; + for i in 0..4 { + let val = self.value[i].load(Ordering::Relaxed); + bytes[i * 8..][..8].copy_from_slice(&val.to_le_bytes()); + } + Hash256::new(bytes) + } + + /// Store Hash256 into the four AtomicU64 parts. + #[inline(always)] + fn store_value(&self, hash: Hash256) { + let bytes = hash.0; + for i in 0..4 { + let mut buf = [0u8; 8]; + buf.copy_from_slice(&bytes[i * 8..][..8]); + self.value[i].store(u64::from_le_bytes(buf), Ordering::Relaxed); + } + } +} + +impl Default for HashCell { + fn default() -> Self { + Self::new() + } +} + +impl Clone for HashCell { + fn clone(&self) -> Self { + match self.get() { + Some(h) => Self::from(h), + None => Self::new(), + } + } +} + +impl From for HashCell { + fn from(hash: Hash256) -> Self { + let cell = Self::new(); + cell.set(hash); + cell + } +} + +impl From> for HashCell { + fn from(hash: Option) -> Self { + match hash { + Some(h) => Self::from(h), + None => Self::new(), + } + } +} + +impl fmt::Debug for HashCell { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self.get() { + Some(h) => f.debug_tuple("HashCell").field(&h).finish(), + None => write!(f, "HashCell()"), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn new_cell_is_empty() { + let cell = HashCell::new(); + assert!(cell.get().is_none()); + } + + #[test] + fn set_then_get() { + let cell = HashCell::new(); + let hash = Hash256::from([0xAB; 32]); + cell.set(hash); + assert_eq!(cell.get(), Some(hash)); + } + + #[test] + fn from_value() { + let hash = Hash256::from([0xCD; 32]); + let cell = HashCell::from(hash); + assert_eq!(cell.get(), Some(hash)); + } + + #[test] + fn zero_hash_is_cached_correctly() { + let cell = HashCell::new(); + cell.set(Hash256::ZERO); + assert_eq!(cell.get(), Some(Hash256::ZERO)); + } + + #[test] + fn redundant_set_preserves_value() { + let cell = HashCell::new(); + let hash = Hash256::from([0x42; 32]); + cell.set(hash); + // Redundant set with the same value. + cell.set(hash); + assert_eq!(cell.get(), Some(hash)); + } + + #[test] + fn clone_preserves_value() { + let cell = HashCell::from(Hash256::from([0x11; 32])); + let cloned = cell.clone(); + assert_eq!(cloned.get(), cell.get()); + } + + #[test] + fn clone_empty_is_empty() { + let cell = HashCell::new(); + let cloned = cell.clone(); + assert!(cloned.get().is_none()); + } + + #[test] + fn size_and_alignment() { + assert_eq!(size_of::(), 40); + assert_eq!(align_of::(), 8); + } + + #[test] + fn concurrent_set_same_value() { + use std::sync::Arc; + let cell = Arc::new(HashCell::new()); + let hash = Hash256::from([0xFF; 32]); + + let handles: Vec<_> = (0..8) + .map(|_| { + let cell = cell.clone(); + std::thread::spawn(move || { + cell.set(hash); + }) + }) + .collect(); + + for h in handles { + h.join().expect("thread panicked"); + } + + assert_eq!(cell.get(), Some(hash)); + } + + #[test] + fn concurrent_get_and_set() { + use std::sync::Arc; + let cell = Arc::new(HashCell::new()); + let hash = Hash256::from([0xEE; 32]); + + let handles: Vec<_> = (0..8) + .map(|i| { + let cell = cell.clone(); + if i % 2 == 0 { + std::thread::spawn(move || { + cell.set(hash); + }) + } else { + std::thread::spawn(move || { + // Reader may see None or Some(hash), never anything else. + if let Some(v) = cell.get() { + assert_eq!(v, hash); + } + }) + } + }) + .collect(); + + for h in handles { + h.join().expect("thread panicked"); + } + + assert_eq!(cell.get(), Some(hash)); + } + + #[test] + fn from_option_some() { + let hash = Hash256::from([0xBB; 32]); + let cell = HashCell::from(Some(hash)); + assert_eq!(cell.get(), Some(hash)); + } + + #[test] + fn from_option_none() { + let cell = HashCell::from(None); + assert!(cell.get().is_none()); + } + + #[test] + fn clear_resets_initialized_cell() { + let mut cell = HashCell::from(Hash256::from([0xAA; 32])); + assert!(cell.get().is_some()); + cell.clear(); + assert!(cell.get().is_none()); + } + + #[test] + fn clear_on_empty_is_noop() { + let mut cell = HashCell::new(); + assert!(cell.get().is_none()); + cell.clear(); + assert!(cell.get().is_none()); + } + + #[test] + fn set_after_clear() { + let mut cell = HashCell::from(Hash256::from([0xAA; 32])); + cell.clear(); + let new_hash = Hash256::from([0xBB; 32]); + cell.set(new_hash); + assert_eq!(cell.get(), Some(new_hash)); + } +} diff --git a/src/leaf.rs b/src/leaf.rs index b1daec7..4ffedea 100644 --- a/src/leaf.rs +++ b/src/leaf.rs @@ -1,6 +1,6 @@ use crate::Arc; +use crate::hash_cell::HashCell; use educe::Educe; -use parking_lot::RwLock; use tree_hash::Hash256; #[derive(Debug, Educe)] @@ -8,8 +8,8 @@ use tree_hash::Hash256; #[educe(PartialEq, Hash)] pub struct Leaf { #[educe(PartialEq(ignore), Hash(ignore))] - #[cfg_attr(feature = "arbitrary", arbitrary(with = crate::utils::arb_rwlock))] - pub hash: RwLock, + #[cfg_attr(feature = "arbitrary", arbitrary(with = crate::utils::arb_hashcell))] + pub hash: HashCell, #[cfg_attr(feature = "arbitrary", arbitrary(with = crate::utils::arb_arc))] pub value: Arc, } @@ -20,7 +20,7 @@ where { fn clone(&self) -> Self { Self { - hash: RwLock::new(*self.hash.read()), + hash: self.hash.clone(), value: self.value.clone(), } } @@ -28,12 +28,12 @@ where impl Leaf { pub fn new(value: T) -> Self { - Self::with_hash(value, Hash256::ZERO) + Self::with_hash(value, None) } - pub fn with_hash(value: T, hash: Hash256) -> Self { + pub fn with_hash(value: T, hash: Option) -> Self { Self { - hash: RwLock::new(hash), + hash: hash.into(), value: Arc::new(value), } } diff --git a/src/lib.rs b/src/lib.rs index c09d0fd..ee81e36 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -4,6 +4,7 @@ pub mod builder; pub mod cow; pub mod error; +pub(crate) mod hash_cell; pub mod interface; pub mod interface_iter; pub mod iter; diff --git a/src/packed_leaf.rs b/src/packed_leaf.rs index ad34d46..3f4678b 100644 --- a/src/packed_leaf.rs +++ b/src/packed_leaf.rs @@ -1,6 +1,6 @@ +use crate::hash_cell::HashCell; use crate::{Error, UpdateMap}; use educe::Educe; -use parking_lot::RwLock; use std::ops::ControlFlow; use tree_hash::{BYTES_PER_CHUNK, Hash256, TreeHash}; @@ -9,8 +9,8 @@ use tree_hash::{BYTES_PER_CHUNK, Hash256, TreeHash}; #[educe(PartialEq, Hash)] pub struct PackedLeaf { #[educe(PartialEq(ignore), Hash(ignore))] - #[cfg_attr(feature = "arbitrary", arbitrary(with = crate::utils::arb_rwlock))] - pub hash: RwLock, + #[cfg_attr(feature = "arbitrary", arbitrary(with = crate::utils::arb_hashcell))] + pub hash: HashCell, pub values: Vec, } @@ -20,7 +20,7 @@ where { fn clone(&self) -> Self { Self { - hash: RwLock::new(*self.hash.read()), + hash: self.hash.clone(), values: self.values.clone(), } } @@ -28,14 +28,11 @@ where impl PackedLeaf { pub fn tree_hash(&self) -> Hash256 { - let read_lock = self.hash.read(); - let mut hash = *read_lock; - drop(read_lock); - - if !hash.is_zero() { - return hash; + if let Some(cached) = self.hash.get() { + return cached; } + let mut hash = Hash256::ZERO; let hash_bytes = hash.as_mut_slice(); let value_len = BYTES_PER_CHUNK / T::tree_hash_packing_factor(); @@ -44,13 +41,13 @@ impl PackedLeaf { .copy_from_slice(&value.tree_hash_packed_encoding()); } - *self.hash.write() = hash; + self.hash.set(hash); hash } pub fn empty() -> Self { PackedLeaf { - hash: RwLock::new(Hash256::ZERO), + hash: HashCell::new(), values: Vec::with_capacity(T::tree_hash_packing_factor()), } } @@ -60,7 +57,7 @@ impl PackedLeaf { values.push(value); PackedLeaf { - hash: RwLock::new(Hash256::ZERO), + hash: HashCell::new(), values, } } @@ -68,14 +65,14 @@ impl PackedLeaf { pub fn repeat(value: T, n: usize) -> Self { assert!(n <= T::tree_hash_packing_factor()); PackedLeaf { - hash: RwLock::new(Hash256::ZERO), + hash: HashCell::new(), values: vec![value; n], } } pub fn insert_at_index(&self, index: usize, value: T) -> Result { let mut updated = PackedLeaf { - hash: RwLock::new(Hash256::ZERO), + hash: HashCell::new(), values: self.values.clone(), }; let sub_index = index % T::tree_hash_packing_factor(); @@ -86,11 +83,11 @@ impl PackedLeaf { pub fn update>( &self, prefix: usize, - hash: Hash256, + hash: Option, updates: &U, ) -> Result { let mut updated = PackedLeaf { - hash: RwLock::new(hash), + hash: hash.into(), values: self.values.clone(), }; @@ -104,8 +101,8 @@ impl PackedLeaf { } pub fn insert_mut(&mut self, sub_index: usize, value: T) -> Result<(), Error> { - // Ensure hash is 0. - *self.hash.get_mut() = Hash256::ZERO; + // Ensure hash is cleared. + self.hash.clear(); if sub_index == self.values.len() { self.values.push(value); diff --git a/src/repeat.rs b/src/repeat.rs index ebaaa8f..5d5fe57 100644 --- a/src/repeat.rs +++ b/src/repeat.rs @@ -1,7 +1,7 @@ use crate::utils::{Length, opt_packing_factor}; use crate::{Arc, Error, Leaf, List, PackedLeaf, Tree, UpdateMap, Value}; use smallvec::{SmallVec, smallvec}; -use tree_hash::Hash256; + use typenum::Unsigned; /// Efficiently construct a list from `n` copies of `elem`. @@ -44,32 +44,26 @@ where for depth in 0..tree_depth { let new_layer = match &layer[..] { [(repeat_leaf, 1)] => { - smallvec![( - Tree::node(repeat_leaf.clone(), Tree::zero(depth), Hash256::ZERO), - 1, - )] + smallvec![(Tree::node(repeat_leaf.clone(), Tree::zero(depth), None), 1,)] } [(repeat_leaf, repeat_count)] if repeat_count.is_multiple_of(2) => { smallvec![( - Tree::node(repeat_leaf.clone(), repeat_leaf.clone(), Hash256::ZERO), + Tree::node(repeat_leaf.clone(), repeat_leaf.clone(), None), repeat_count / 2, )] } [(repeat_leaf, repeat_count)] => { smallvec![ ( - Tree::node(repeat_leaf.clone(), repeat_leaf.clone(), Hash256::ZERO), + Tree::node(repeat_leaf.clone(), repeat_leaf.clone(), None), repeat_count / 2, ), - ( - Tree::node(repeat_leaf.clone(), Tree::zero(depth), Hash256::ZERO), - 1, - ), + (Tree::node(repeat_leaf.clone(), Tree::zero(depth), None), 1,), ] } [(repeat_leaf, 1), (lonely_leaf, 1)] => { smallvec![( - Tree::node(repeat_leaf.clone(), lonely_leaf.clone(), Hash256::ZERO), + Tree::node(repeat_leaf.clone(), lonely_leaf.clone(), None), 1, )] } @@ -77,22 +71,19 @@ where if repeat_count.is_multiple_of(2) { smallvec![ ( - Tree::node(repeat_leaf.clone(), repeat_leaf.clone(), Hash256::ZERO), + Tree::node(repeat_leaf.clone(), repeat_leaf.clone(), None), repeat_count / 2, ), - ( - Tree::node(lonely_leaf.clone(), Tree::zero(depth), Hash256::ZERO), - 1, - ), + (Tree::node(lonely_leaf.clone(), Tree::zero(depth), None), 1,), ] } else { smallvec![ ( - Tree::node(repeat_leaf.clone(), repeat_leaf.clone(), Hash256::ZERO), + Tree::node(repeat_leaf.clone(), repeat_leaf.clone(), None), repeat_count / 2, ), ( - Tree::node(repeat_leaf.clone(), lonely_leaf.clone(), Hash256::ZERO), + Tree::node(repeat_leaf.clone(), lonely_leaf.clone(), None), 1, ), ] diff --git a/src/tests/size_of.rs b/src/tests/size_of.rs index 31b53f7..84f3903 100644 --- a/src/tests/size_of.rs +++ b/src/tests/size_of.rs @@ -1,5 +1,5 @@ +use crate::hash_cell::HashCell; use crate::{Arc, Leaf, PackedLeaf, Tree}; -use parking_lot::RwLock; use std::mem::size_of; use tree_hash::Hash256; @@ -10,8 +10,8 @@ fn size_of_hash256() { assert_eq!(size_of::>(), 48); assert_eq!(size_of::>(), 64); - let rw_lock_size = size_of::>(); - assert_eq!(rw_lock_size, 40); + let hash_cell_size = size_of::(); + assert_eq!(hash_cell_size, 40); let arc_size = size_of::>>(); assert_eq!(arc_size, 8); @@ -27,12 +27,9 @@ fn size_of_u8() { assert_eq!(size_of::>(), 64); assert_eq!( size_of::>(), - size_of::>() + size_of::>() + size_of::() + size_of::>() ); - let rw_lock_size = size_of::>(); - assert_eq!(rw_lock_size, 16); - let arc_size = size_of::>>(); assert_eq!(arc_size, 8); diff --git a/src/tree.rs b/src/tree.rs index b40eae9..d0f5046 100644 --- a/src/tree.rs +++ b/src/tree.rs @@ -1,8 +1,8 @@ +use crate::hash_cell::HashCell; use crate::utils::{Length, opt_hash, opt_packing_depth, opt_packing_factor}; use crate::{Arc, Error, Leaf, PackedLeaf, UpdateMap, Value}; use educe::Educe; use ethereum_hashing::{ZERO_HASHES, hash32_concat}; -use parking_lot::RwLock; use std::collections::BTreeMap; use std::collections::HashMap; use std::ops::ControlFlow; @@ -16,8 +16,8 @@ pub enum Tree { PackedLeaf(PackedLeaf), Node { #[educe(PartialEq(ignore), Hash(ignore))] - #[cfg_attr(feature = "arbitrary", arbitrary(with = crate::utils::arb_rwlock))] - hash: RwLock, + #[cfg_attr(feature = "arbitrary", arbitrary(with = crate::utils::arb_hashcell))] + hash: HashCell, #[cfg_attr(feature = "arbitrary", arbitrary(with = crate::utils::arb_arc))] left: Arc, #[cfg_attr(feature = "arbitrary", arbitrary(with = crate::utils::arb_arc))] @@ -30,7 +30,7 @@ impl Clone for Tree { fn clone(&self) -> Self { match self { Self::Node { hash, left, right } => Self::Node { - hash: RwLock::new(*hash.read()), + hash: hash.clone(), left: left.clone(), right: right.clone(), }, @@ -46,9 +46,9 @@ impl Tree { Self::zero(depth) } - pub fn node(left: Arc, right: Arc, hash: Hash256) -> Arc { + pub fn node(left: Arc, right: Arc, hash: Option) -> Arc { Arc::new(Self::Node { - hash: RwLock::new(hash), + hash: hash.into(), left, right, }) @@ -62,13 +62,13 @@ impl Tree { Arc::new(Self::Leaf(Leaf::new(value))) } - pub fn leaf_with_hash(value: T, hash: Hash256) -> Arc { + pub fn leaf_with_hash(value: T, hash: Option) -> Arc { Arc::new(Self::Leaf(Leaf::with_hash(value, hash))) } pub fn node_unboxed(left: Arc, right: Arc) -> Self { Self::Node { - hash: RwLock::new(Hash256::ZERO), + hash: HashCell::new(), left, right, } @@ -125,14 +125,14 @@ impl Tree { Ok(Self::node( left.with_updated_leaf(index, new_value, new_depth)?, right.clone(), - Hash256::ZERO, + None, )) } else { // Index lies on the right, recurse right Ok(Self::node( left.clone(), right.with_updated_leaf(index, new_value, new_depth)?, - Hash256::ZERO, + None, )) } } @@ -147,7 +147,7 @@ impl Tree { // Split zero node into a node with left and right, and recurse into // the appropriate subtree let new_zero = Self::zero(depth - 1); - Self::node(new_zero.clone(), new_zero, Hash256::ZERO) + Self::node(new_zero.clone(), new_zero, None) .with_updated_leaf(index, new_value, depth) } } @@ -162,7 +162,7 @@ impl Tree { depth: usize, hashes: Option<&BTreeMap<(usize, usize), Hash256>>, ) -> Result, Error> { - let hash = opt_hash(hashes, depth, prefix).unwrap_or_default(); + let hash = opt_hash(hashes, depth, prefix); match self { Self::Leaf(_) if depth == 0 => { @@ -294,29 +294,31 @@ impl Tree { (Self::Zero(z1), Self::Zero(z2)) if z1 == z2 => Ok(RebaseAction::EqualReplace(base)), ( Self::Node { - hash: orig_hash_lock, + hash: orig_hash_cell, left: l1, right: r1, }, Self::Node { - hash: base_hash_lock, + hash: base_hash_cell, left: l2, right: r2, }, ) if full_depth > 0 => { use RebaseAction::*; - let orig_hash = *orig_hash_lock.read(); - let base_hash = *base_hash_lock.read(); + let orig_hash = orig_hash_cell.get(); + let base_hash = base_hash_cell.get(); // If hashes *and* lengths are equal then we can short-cut the recursion // and immediately replace `orig` by the `base` node. If `lengths` are `None` // then we know they are already equal (e.g. we're in a vector). - if !orig_hash.is_zero() - && orig_hash == base_hash - && lengths.is_none_or(|(orig_length, base_length)| orig_length == base_length) - { - return Ok(EqualReplace(base)); + if let (Some(oh), Some(bh)) = (orig_hash, base_hash) { + if oh == bh + && lengths + .is_none_or(|(orig_length, base_length)| orig_length == base_length) + { + return Ok(EqualReplace(base)); + } } let new_full_depth = full_depth - 1; @@ -345,55 +347,29 @@ impl Tree { Ok(NotEqualNoop) } (EqualNoop, EqualNoop) => Ok(EqualNoop), - (NotEqualNoop | EqualNoop, NotEqualReplace(new_right)) => { - Ok(NotEqualReplace(Arc::new(Self::Node { - hash: RwLock::new(orig_hash), - left: l1.clone(), - right: new_right, - }))) - } - (NotEqualNoop | EqualNoop, EqualReplace(new_right)) => { - Ok(NotEqualReplace(Arc::new(Self::Node { - hash: RwLock::new(orig_hash), - left: l1.clone(), - right: new_right.clone(), - }))) - } + (NotEqualNoop | EqualNoop, NotEqualReplace(new_right)) => Ok(NotEqualReplace( + Self::node(l1.clone(), new_right, orig_hash), + )), + (NotEqualNoop | EqualNoop, EqualReplace(new_right)) => Ok(NotEqualReplace( + Self::node(l1.clone(), new_right.clone(), orig_hash), + )), (NotEqualReplace(new_left), NotEqualNoop | EqualNoop) => { - Ok(NotEqualReplace(Arc::new(Self::Node { - hash: RwLock::new(orig_hash), - left: new_left, - right: r1.clone(), - }))) + Ok(NotEqualReplace(Self::node(new_left, r1.clone(), orig_hash))) } (NotEqualReplace(new_left), NotEqualReplace(new_right)) => { - Ok(NotEqualReplace(Arc::new(Self::Node { - hash: RwLock::new(orig_hash), - left: new_left, - right: new_right, - }))) - } - (NotEqualReplace(new_left), EqualReplace(new_right)) => { - Ok(NotEqualReplace(Arc::new(Self::Node { - hash: RwLock::new(orig_hash), - left: new_left, - right: new_right.clone(), - }))) - } - (EqualReplace(new_left), NotEqualNoop) => { - Ok(NotEqualReplace(Arc::new(Self::Node { - hash: RwLock::new(orig_hash), - left: new_left.clone(), - right: r1.clone(), - }))) - } - (EqualReplace(new_left), NotEqualReplace(new_right)) => { - Ok(NotEqualReplace(Arc::new(Self::Node { - hash: RwLock::new(orig_hash), - left: new_left.clone(), - right: new_right, - }))) + Ok(NotEqualReplace(Self::node(new_left, new_right, orig_hash))) } + (NotEqualReplace(new_left), EqualReplace(new_right)) => Ok(NotEqualReplace( + Self::node(new_left, new_right.clone(), orig_hash), + )), + (EqualReplace(new_left), NotEqualNoop) => Ok(NotEqualReplace(Self::node( + new_left.clone(), + r1.clone(), + orig_hash, + ))), + (EqualReplace(new_left), NotEqualReplace(new_right)) => Ok(NotEqualReplace( + Self::node(new_left.clone(), new_right, orig_hash), + )), (EqualReplace(_), EqualReplace(_)) | (EqualReplace(_), EqualNoop) => { Ok(EqualReplace(base)) } @@ -438,12 +414,8 @@ impl Tree { match &**orig { Self::Leaf(_) | Self::PackedLeaf(_) | Self::Zero(_) => Ok(IntraRebaseAction::Noop), Self::Node { hash, left, right } if current_depth > 0 => { - let hash = *hash.read(); - // Tree must be fully hashed prior to intra-rebase. - if hash.is_zero() { - return Err(Error::IntraRebaseZeroHash); - } + let hash = hash.get().ok_or(Error::IntraRebaseZeroHash)?; if let Some(known_subtree) = known_subtrees.get(&(current_depth, hash)) { // Node is already known from elsewhere in the tree. We can replace it without @@ -457,15 +429,15 @@ impl Tree { let action = match (left_action, right_action) { (IntraRebaseAction::Noop, IntraRebaseAction::Noop) => IntraRebaseAction::Noop, (IntraRebaseAction::Noop, IntraRebaseAction::Replace(new_right)) => { - IntraRebaseAction::Replace(Self::node(left.clone(), new_right, hash)) + IntraRebaseAction::Replace(Self::node(left.clone(), new_right, Some(hash))) } (IntraRebaseAction::Replace(new_left), IntraRebaseAction::Noop) => { - IntraRebaseAction::Replace(Self::node(new_left, right.clone(), hash)) + IntraRebaseAction::Replace(Self::node(new_left, right.clone(), Some(hash))) } ( IntraRebaseAction::Replace(new_left), IntraRebaseAction::Replace(new_right), - ) => IntraRebaseAction::Replace(Self::node(new_left, new_right, hash)), + ) => IntraRebaseAction::Replace(Self::node(new_left, new_right, Some(hash))), }; // Add the new version of this node to the known subtrees. @@ -495,44 +467,26 @@ impl Tree { pub fn tree_hash(&self) -> Hash256 { match self { Self::Leaf(Leaf { hash, value }) => { - // FIXME(sproul): upgradeable RwLock? - let read_lock = hash.read(); - let existing_hash = *read_lock; - drop(read_lock); - - // NOTE: We re-compute the hash whenever it is non-zero. Computed hashes may - // legitimately be zero, but this only occurs at the leaf level when the value is - // entirely zeroes (e.g. [0u64, 0, 0, 0]). In order to avoid storing an - // `Option` we choose to re-compute the hash in this case. In practice - // this is unlikely to provide any performance penalty except at very small list - // lengths (<= 32), because a node higher in the tree will cache a non-zero hash - // preventing its children from being visited more than once. - if !existing_hash.is_zero() { - existing_hash - } else { - let tree_hash = value.tree_hash_root(); - *hash.write() = tree_hash; - tree_hash + if let Some(cached) = hash.get() { + return cached; } + let computed = value.tree_hash_root(); + hash.set(computed); + computed } Self::PackedLeaf(leaf) => leaf.tree_hash(), Self::Zero(depth) => Hash256::from(ZERO_HASHES[*depth]), Self::Node { hash, left, right } => { - let read_lock = hash.read(); - let existing_hash = *read_lock; - drop(read_lock); - - if !existing_hash.is_zero() { - existing_hash - } else { - // Parallelism goes brrrr. - let (left_hash, right_hash) = - rayon::join(|| left.tree_hash(), || right.tree_hash()); - let tree_hash = - Hash256::from(hash32_concat(left_hash.as_slice(), right_hash.as_slice())); - *hash.write() = tree_hash; - tree_hash + if let Some(cached) = hash.get() { + return cached; } + // Parallelism goes brrrr. + let (left_hash, right_hash) = + rayon::join(|| left.tree_hash(), || right.tree_hash()); + let computed = + Hash256::from(hash32_concat(left_hash.as_slice(), right_hash.as_slice())); + hash.set(computed); + computed } } } diff --git a/src/utils.rs b/src/utils.rs index 5bcb7ac..2b0aa31 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -106,10 +106,10 @@ pub fn arb_arc<'a, T: Arbitrary<'a>>( } #[cfg(feature = "arbitrary")] -pub fn arb_rwlock<'a, T: Arbitrary<'a>>( +pub fn arb_hashcell<'a>( u: &mut arbitrary::Unstructured<'a>, -) -> arbitrary::Result> { - T::arbitrary(u).map(parking_lot::RwLock::new) +) -> arbitrary::Result { + Hash256::arbitrary(u).map(crate::hash_cell::HashCell::from) } #[cfg(test)] From d81a9c3f426f7ac689dcb28a6ffa7317d8974118 Mon Sep 17 00:00:00 2001 From: Mac L Date: Wed, 25 Mar 2026 03:17:37 +1100 Subject: [PATCH 2/2] clippy --- src/tree.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/tree.rs b/src/tree.rs index d0f5046..e82e8b2 100644 --- a/src/tree.rs +++ b/src/tree.rs @@ -312,13 +312,11 @@ impl Tree { // If hashes *and* lengths are equal then we can short-cut the recursion // and immediately replace `orig` by the `base` node. If `lengths` are `None` // then we know they are already equal (e.g. we're in a vector). - if let (Some(oh), Some(bh)) = (orig_hash, base_hash) { - if oh == bh - && lengths - .is_none_or(|(orig_length, base_length)| orig_length == base_length) - { - return Ok(EqualReplace(base)); - } + if let (Some(oh), Some(bh)) = (orig_hash, base_hash) + && oh == bh + && lengths.is_none_or(|(orig_length, base_length)| orig_length == base_length) + { + return Ok(EqualReplace(base)); } let new_full_depth = full_depth - 1;