diff --git a/plt/plt-block-state/benches/hashed_cacheable_reference.rs b/plt/plt-block-state/benches/hashed_cacheable_reference.rs index 609055b637..80ab933936 100644 --- a/plt/plt-block-state/benches/hashed_cacheable_reference.rs +++ b/plt/plt-block-state/benches/hashed_cacheable_reference.rs @@ -25,5 +25,5 @@ fn a_warmup() { #[divan::bench(threads = THREADS)] fn bench_with_value(bencher: Bencher) { let hcr = divan::black_box(HashedCacheableRef::new(StoreSerialized(0))); - bencher.bench(|| hcr.with_value(&UnreachableBlobStore, |v| Ok(*v)).unwrap()); + bencher.bench(|| hcr.value(&UnreachableBlobStore).unwrap()); } diff --git a/plt/plt-block-state/src/block_state/blob_reference/hashed_cacheable_reference.rs b/plt/plt-block-state/src/block_state/blob_reference/hashed_cacheable_reference.rs index 779f48b0fd..bcbf170837 100644 --- a/plt/plt-block-state/src/block_state/blob_reference/hashed_cacheable_reference.rs +++ b/plt/plt-block-state/src/block_state/blob_reference/hashed_cacheable_reference.rs @@ -7,12 +7,12 @@ use crate::block_state::blob_store::{ }; use crate::block_state::cacheable::Cacheable; use crate::block_state::hash::Hashable; -use crate::block_state::utils::{LockRef, OwnedOrBorrowed}; +use crate::block_state::utils::OwnedOrBorrowed; use crate::block_state_interface::BlockStateResult; use concordium_base::common::{Buffer, Get, Put}; use concordium_base::hashes::Hash; use std::io::Read; -use std::mem; +use std::sync::{Arc, OnceLock}; /// Representation of an immutable, cachable and lazily hashed value of type `V`. /// The represented value is immutable in the sense that the value itself does not change, @@ -38,11 +38,9 @@ use std::mem; /// via interior mutability. #[derive(Debug)] pub struct HashedCacheableRef { - /// The representation is wrapped in a [`LockRef`] to allow cheap cloning and - /// interior mutability. The interior mutability must not be used to change which value - /// is actually represented, only to update internal structure, such that where the value - /// is represented. - inner: LockRef>, + /// The representation is wrapped in a [`Arc`] to allow cheap cloning and + /// interior mutability of the shared inner value. + inner: Arc>, } impl Default for HashedCacheableRef { @@ -55,56 +53,32 @@ impl HashedCacheableRef { /// Create a new value represented in memory. pub fn new(value: V) -> Self { let inner = HashedBufferedRefInner { - hash: None, - repr: HashedCacheableRefRepr::Memory { value }, + hash: OnceLock::new(), + repr: HashedCacheableRefRepr::Memory { + value, + blob_location_lock: OnceLock::new(), + }, }; - Self::from_inner(inner) - } - - fn from_inner(inner: HashedBufferedRefInner) -> Self { Self { - inner: LockRef::new(inner), + inner: Arc::new(inner), } } - /// Access the referenced value via the given `read` closure. The value of type `T` returned by `read` - /// is the value returned by `with_value`. If the value is already in memory, the value - /// is passed to the closure as borrowed. If it is not in memory, it is loaded from the - /// blob store, and passed owned to the closure as owned. - /// - /// Loading from the blob store will not make the value cached in the reference. - /// - /// # Errors - /// - /// Returns [`BlockStateError`] if returned by `read` or if - /// decoding data from the blob store fails. - pub fn with_value( - &self, - loader: &impl BlobStoreLoad, - read: impl FnOnce(OwnedOrBorrowed) -> BlockStateResult, - ) -> BlockStateResult - where - V: Loadable, - { - let inner = self.inner.read(); - inner.repr.get_or_load_value(loader).and_then(read) - } - - /// Load the referenced value and return it. If the value is already in memory, it is cloned - /// and returned. If it is not in memory, it is loaded from the blob store, and returned. + /// Access the referenced value. If the value is already in memory, the value + /// is returned as borrowed. If it is not in memory, it is loaded from the + /// blob store, and returned as owned. /// /// Loading from the blob store will not make the value cached in the reference. /// /// # Errors /// /// Returns [`BlockStateError`] if decoding data from the blob store fails. - pub fn clone_or_load_value(&self, loader: &impl BlobStoreLoad) -> BlockStateResult + pub fn value(&self, loader: &impl BlobStoreLoad) -> BlockStateResult> where - V: Loadable + Clone, + V: Loadable, { - let inner = self.inner.read(); - Ok(inner.repr.get_or_load_value(loader)?.into_owned()) + self.inner.repr.get_or_load_value(loader) } } @@ -118,15 +92,38 @@ impl Clone for HashedCacheableRef { } } -/// The [`HashedCacheableRef`] behind the [`LockRef`]. +/// The [`HashedCacheableRef`] behind the `Arc`. #[derive(Debug)] struct HashedBufferedRefInner { - /// Lazily calculated hash. - hash: Option, - /// The potentially buffered value. + /// Lazily calculated hash. If set, it is the hash of the referenced value. + hash: OnceLock, + /// Representation of the value. repr: HashedCacheableRefRepr, } +/// The possible representations of the referenced value. +/// +/// Notice that the two variants decide where the value was represented, when the reference +/// was created (in blob store or in memory). But both of the variants `Store` and `Memory` +/// can represent a value that is in memory and stored at the same time. +#[derive(Debug)] +enum HashedCacheableRefRepr { + /// The value is in the blob store, and is maybe cached. + Store { + /// Location of the value in the blob store + blob_location: BlobStoreLocation, + /// In-memory value. Is set if the value is currently cached in memory. + value_lock: OnceLock, + }, + /// The value is in memory, and is maybe also stored (the same as cached) + Memory { + /// In-memory value. + value: V, + /// Location of the value in the blob store. Is set if the value is also stored in the blob store. + blob_location_lock: OnceLock, + }, +} + impl HashedCacheableRefRepr { /// Load the referenced value and return it. If the value is already in memory, a reference /// to it is simply returned. If it is not in memory, it is loaded from the blob store, @@ -141,38 +138,49 @@ impl HashedCacheableRefRepr { V: Loadable, { Ok(match self { - HashedCacheableRefRepr::Store { reference } => { - let value: V = blob_store::load_from_store(loader, *reference)?; - OwnedOrBorrowed::Owned(value) - } - HashedCacheableRefRepr::Memory { value } => OwnedOrBorrowed::Borrowed(value), - HashedCacheableRefRepr::Cache { value, .. } => OwnedOrBorrowed::Borrowed(value), + HashedCacheableRefRepr::Store { + blob_location, + value_lock, + } => match value_lock.get() { + None => { + let value: V = blob_store::load_from_store(loader, *blob_location)?; + OwnedOrBorrowed::Owned(value) + } + Some(value) => OwnedOrBorrowed::Borrowed(value), + }, + HashedCacheableRefRepr::Memory { value, .. } => OwnedOrBorrowed::Borrowed(value), }) } /// Cache the referenced value and return it. If the value is already in memory, a reference /// to it is simply returned. If it is not in memory, it is loaded from the blob store and /// cached in [`HashedCacheableRefRepr::Cache`] first. - fn get_or_cache_value(&mut self, loader: &impl BlobStoreLoad) -> BlockStateResult<&V> + fn get_or_cache_value(&self, loader: &impl BlobStoreLoad) -> BlockStateResult<&V> where V: Loadable, { Ok(match self { - HashedCacheableRefRepr::Store { reference } => { - let value: V = blob_store::load_from_store(loader, *reference)?; - *self = HashedCacheableRefRepr::Cache { - reference: *reference, - value, - }; - // We just wrote the Cache variant, so we can safely assert this variant, - // in order to borrow the value just written. - let HashedCacheableRefRepr::Cache { value, .. } = self else { - unreachable!("not HashedBufferedRefRepr::Cache though it was just written") - }; - value + HashedCacheableRefRepr::Store { + blob_location, + value_lock, + } => { + // When OnceLock::get_mut_or_try_init is stable, we can use that instead of the + // get/set pattern used here. get_mut_or_try_init imposes a critical region such + // that two threads will not load the value for caching. Right now we just log + // if it happens. + match value_lock.get() { + None => { + let value: V = blob_store::load_from_store(loader, *blob_location)?; + value_lock + .set(value) + .inspect_err(|_| eprintln!("HashedCacheableRef: Value loaded by two threads at the same time")) + .ok(); + value_lock.get().expect("HashedCacheableRefRepr::get_or_cache_value: value not present though just set in lock") + } + Some(value) => value, + } } - HashedCacheableRefRepr::Memory { value } => value, - HashedCacheableRefRepr::Cache { value, .. } => value, + HashedCacheableRefRepr::Memory { value, .. } => value, }) } @@ -180,88 +188,82 @@ impl HashedCacheableRefRepr { /// the blob store, the [`BlobStoreLocation`] for it is simply returned. If it is not stored, /// it is stored into the blob store, and the resulting [`BlobStoreLocation`] is saved in /// [`HashedCacheableRefRepr::Cache`] and returned. - fn get_reference_or_store(&mut self, storer: &mut impl BlobStoreStore) -> BlobStoreLocation + fn get_reference_or_store(&self, storer: &mut impl BlobStoreStore) -> BlobStoreLocation where V: Storable, { match self { - HashedCacheableRefRepr::Store { reference } => *reference, - HashedCacheableRefRepr::Memory { value } => { - let reference = blob_store::store_to_store(storer, &*value); - - // We need the Memory representation owned, in order to move the value out of - // it and into the Cache representation, without cloning it. - // In order to achieve this, we swap the intermediate representation Store to self. - let mut repr_tmp = HashedCacheableRefRepr::Store { reference }; - mem::swap(&mut repr_tmp, self); - let HashedCacheableRefRepr::Memory { value } = repr_tmp else { - unreachable!("not HashedBufferedRefRepr::Memory though it was just matched") - }; - *self = HashedCacheableRefRepr::Cache { value, reference }; - - reference - } - HashedCacheableRefRepr::Cache { reference, .. } => *reference, + HashedCacheableRefRepr::Store { blob_location, .. } => *blob_location, + HashedCacheableRefRepr::Memory { + value, + blob_location_lock, + } => match blob_location_lock.get() { + Some(blob_location) => *blob_location, + None => { + *blob_location_lock.get_or_init(|| blob_store::store_to_store(storer, value)) + } + }, } } } -/// The possible representations of the referenced value. -#[derive(Debug)] -enum HashedCacheableRefRepr { - /// The value is in the blob store. - Store { reference: BlobStoreLocation }, - /// The value is in memory and not written to blob store. - Memory { value: V }, - /// The value is in the blob store, and also cached in memory. - Cache { - reference: BlobStoreLocation, - value: V, - }, -} - impl Loadable for HashedCacheableRef { fn load_from_buffer( mut buffer: impl Read, _loader: &impl BlobStoreLoad, ) -> BlockStateResult { - let reference = buffer.get().map_parse_err_to_block_state_err()?; + let blob_location: BlobStoreLocation = buffer.get().map_parse_err_to_block_state_err()?; let inner = HashedBufferedRefInner { - hash: None, - repr: HashedCacheableRefRepr::Store { reference }, + hash: OnceLock::new(), + repr: HashedCacheableRefRepr::Store { + blob_location, + value_lock: OnceLock::new(), + }, }; - Ok(Self::from_inner(inner)) + Ok(Self { + inner: Arc::new(inner), + }) } } impl Storable for HashedCacheableRef { fn store_to_buffer(&self, mut buffer: impl Buffer, storer: &mut impl BlobStoreStore) { - let mut inner = self.inner.write(); - let reference = inner.repr.get_reference_or_store(storer); + let reference = self.inner.repr.get_reference_or_store(storer); buffer.put(reference); } } impl Cacheable for HashedCacheableRef { fn cache_reference_values(&self, loader: &impl BlobStoreLoad) -> BlockStateResult<()> { - let mut inner = self.inner.write(); - let value = inner.repr.get_or_cache_value(loader)?; + let value = self.inner.repr.get_or_cache_value(loader)?; value.cache_reference_values(loader) } } impl Hashable for HashedCacheableRef { fn hash(&self, loader: &impl BlobStoreLoad) -> BlockStateResult { - let mut inner = self.inner.write(); - Ok(if let Some(hash) = inner.hash { - hash - } else { - let value = inner.repr.get_or_load_value(loader)?; - let hash = value.hash(loader)?; - inner.hash = Some(hash); - hash - }) + // When OnceLock::get_mut_or_try_init is stable, we can use that instead of the + // get/set pattern used here. get_mut_or_try_init imposes a critical region such + // that two threads will not hash the value. Right now we just log + // if it happens. + match self.inner.hash.get() { + Some(hash) => Ok(*hash), + None => { + let value = self.inner.repr.get_or_load_value(loader)?; + let hash = value.hash(loader)?; + self.inner + .hash + .set(hash) + .inspect_err(|_| { + eprintln!( + "HashedCacheableRef: Hash calculated by two threads at the same time" + ) + }) + .ok(); + Ok(hash) + } + } } } @@ -272,9 +274,37 @@ mod tests { use crate::block_state::blob_store::StoreSerialized; use crate::block_state::blob_store::test_stub::{BlobStoreStub, UnreachableBlobStore}; use assert_matches::assert_matches; + use std::fmt::Debug; type TestRef = HashedCacheableRef>; + fn assert_in_memory_repr(hcr: &HashedCacheableRef) -> &V { + assert_matches!(&hcr.inner.repr, HashedCacheableRefRepr::Memory { blob_location_lock, value } => { + assert!(blob_location_lock.get().is_none(), "in blob store"); + value + }) + } + + fn assert_stored_repr(hcr: &HashedCacheableRef) -> BlobStoreLocation { + assert_matches!(&hcr.inner.repr, HashedCacheableRefRepr::Store { blob_location, value_lock } => { + assert!(value_lock.get().is_none(), "in memory"); + *blob_location + }) + } + + fn assert_cached_repr(hcr: &HashedCacheableRef) -> (&V, BlobStoreLocation) { + match &hcr.inner.repr { + HashedCacheableRefRepr::Store { + value_lock, + blob_location, + } => (value_lock.get().expect("not in memory"), *blob_location), + HashedCacheableRefRepr::Memory { + value, + blob_location_lock, + } => (value, *blob_location_lock.get().expect("not in blob store")), + } + } + /// Test full lifecycle of a value: /// /// * create as new in memory @@ -290,31 +320,24 @@ mod tests { // Create new value an assert representation is memory let val1 = TestRef::new(StoreSerialized(1u64)); - assert_matches!(&val1.inner.read().repr, HashedCacheableRefRepr::Memory{value} => { - assert_eq!(*value, StoreSerialized(1)); - }); + assert_eq!(*assert_in_memory_repr(&val1), StoreSerialized(1)); // Store value to blob store and assert representation is now cache. let blob_ref = blob_store::store_to_store(&mut store, &val1); - let val_ref = assert_matches!(&val1.inner.read().repr, HashedCacheableRefRepr::Cache {reference,value} => { - assert_eq!(*value, StoreSerialized(1)); - *reference - }); + let (val_tmp, val_ref) = assert_cached_repr(&val1); + assert_eq!(*val_tmp, StoreSerialized(1)); drop(val1); // Load value from blob store and assert representation is store. let val2: TestRef = blob_store::load_from_store(&store, blob_ref).unwrap(); - assert_matches!(&val2.inner.read().repr, HashedCacheableRefRepr::Store {reference} => { - assert_eq!(*reference, val_ref); - }); + assert_eq!(assert_stored_repr(&val2), val_ref); // Cache the value and assert representation is now cache val2.cache_reference_values(&store).expect("cache"); - assert_matches!(&val2.inner.read().repr, HashedCacheableRefRepr::Cache {reference,value} => { - assert_eq!(*value, StoreSerialized(1)); - assert_eq!(*reference, val_ref); - }); + let (val_tmp, val_ref_tmp) = assert_cached_repr(&val2); + assert_eq!(*val_tmp, StoreSerialized(1)); + assert_eq!(val_ref_tmp, val_ref); } /// Test storing cached value. @@ -325,16 +348,13 @@ mod tests { // Store value to make it cached blob_store::store_to_store(&mut store, &val1); - let val_ref = assert_matches!(&val1.inner.read().repr, HashedCacheableRefRepr::Cache {reference,..} => { - *reference - }); + let (_, val_ref) = assert_cached_repr(&val1); // Store value again and assert this does not change the reference to the value. blob_store::store_to_store(&mut store, &val1); - assert_matches!(&val1.inner.read().repr, HashedCacheableRefRepr::Cache {reference,value} => { - assert_eq!(*value, StoreSerialized(1)); - assert_eq!(*reference, val_ref); - }); + let (val_tmp, val_ref_tmp) = assert_cached_repr(&val1); + assert_eq!(*val_tmp, StoreSerialized(1)); + assert_eq!(val_ref_tmp, val_ref); } /// Test storing stored value. @@ -347,15 +367,11 @@ mod tests { // Load value to make it stored let val2: TestRef = blob_store::load_from_store(&store, blob_ref).unwrap(); - let val_ref = assert_matches!(&val2.inner.read().repr, HashedCacheableRefRepr::Store {reference} => { - *reference - }); + let val_ref = assert_stored_repr(&val2); // Store value and assert this does not change the reference to the value. blob_store::store_to_store(&mut store, &val2); - assert_matches!(&val2.inner.read().repr, HashedCacheableRefRepr::Store {reference} => { - assert_eq!(*reference, val_ref); - }); + assert_eq!(assert_stored_repr(&val2), val_ref); } /// Test caching cached value. @@ -366,19 +382,16 @@ mod tests { // Store value to make it cached blob_store::store_to_store(&mut store, &val1); - let val_ref = assert_matches!(&val1.inner.read().repr, HashedCacheableRefRepr::Cache {reference,value} => { - assert_eq!(*value, StoreSerialized(1)); - *reference - }); + let (val_tmp, val_ref) = assert_cached_repr(&val1); + assert_eq!(*val_tmp, StoreSerialized(1)); // Cache value and assert it does not change representation // Assert that we don't need to read from the blob store by using UnreachableBlobStore val1.cache_reference_values(&UnreachableBlobStore) .expect("cache"); - assert_matches!(&val1.inner.read().repr, HashedCacheableRefRepr::Cache {reference,value} => { - assert_eq!(*value, StoreSerialized(1)); - assert_eq!(*reference, val_ref); - }); + let (val_tmp, val_ref_tmp) = assert_cached_repr(&val1); + assert_eq!(*val_tmp, StoreSerialized(1)); + assert_eq!(val_ref_tmp, val_ref); } /// Test caching in-memory value. @@ -390,119 +403,43 @@ mod tests { // Assert that we don't need to read from the blob store by using UnreachableBlobStore val1.cache_reference_values(&UnreachableBlobStore) .expect("cache"); - assert_matches!(&val1.inner.read().repr, HashedCacheableRefRepr::Memory {value} => { - assert_eq!(*value, StoreSerialized(1)); - }); + assert_eq!(*assert_in_memory_repr(&val1), StoreSerialized(1)); } - /// Test [`HashedCacheableRef::clone_or_load_value`] + /// Test [`HashedCacheableRef::value`] #[test] - fn test_clone_or_load_value() { + fn value() { let mut store = BlobStoreStub::default(); // Test in-memory value. Assert in-memory representation does not change. // Assert that we don't need to read from the blob store by using UnreachableBlobStore let val1 = TestRef::new(StoreSerialized(1u64)); assert_eq!( - val1.clone_or_load_value(&UnreachableBlobStore).unwrap(), + *val1.value(&UnreachableBlobStore).unwrap(), StoreSerialized(1u64) ); - assert_matches!( - &val1.inner.read().repr, - HashedCacheableRefRepr::Memory { .. } - ); + assert_in_memory_repr(&val1); // Store value to make it cached let blob_ref = blob_store::store_to_store(&mut store, &val1); - assert_matches!( - &val1.inner.read().repr, - HashedCacheableRefRepr::Cache { .. } - ); + assert_cached_repr(&val1); // Test cached value. Assert cached representation does not change. // Assert that we don't need to read from the blob store by using UnreachableBlobStore assert_eq!( - val1.clone_or_load_value(&UnreachableBlobStore).unwrap(), + *val1.value(&UnreachableBlobStore).unwrap(), StoreSerialized(1u64) ); - assert_matches!( - &val1.inner.read().repr, - HashedCacheableRefRepr::Cache { .. } - ); + assert_cached_repr(&val1); // Load value to make it stored. drop(val1); let val2: TestRef = blob_store::load_from_store(&store, blob_ref).unwrap(); - assert_matches!( - &val2.inner.read().repr, - HashedCacheableRefRepr::Store { .. } - ); + assert_stored_repr(&val2); // Test stored value. Assert stored representation does not change. - assert_eq!( - val2.clone_or_load_value(&store).unwrap(), - StoreSerialized(1u64) - ); - assert_matches!( - &val2.inner.read().repr, - HashedCacheableRefRepr::Store { .. } - ); - } - - /// Test [`HashedCacheableRef::with_value`] - #[test] - fn test_with_value() { - let mut store = BlobStoreStub::default(); - - // Test in-memory value. Assert in-memory representation does not change. - // Assert that we don't need to read from the blob store by using UnreachableBlobStore - let val1 = TestRef::new(StoreSerialized(1u64)); - assert_eq!( - val1.with_value(&UnreachableBlobStore, |val| Ok(*val)) - .unwrap(), - StoreSerialized(1u64) - ); - assert_matches!( - &val1.inner.read().repr, - HashedCacheableRefRepr::Memory { .. } - ); - - // Store value to make it cached - let blob_ref = blob_store::store_to_store(&mut store, &val1); - assert_matches!( - &val1.inner.read().repr, - HashedCacheableRefRepr::Cache { .. } - ); - - // Test cached value. Assert cached representation does not change. - // Assert that we don't need to read from the blob store by using UnreachableBlobStore - assert_eq!( - val1.with_value(&UnreachableBlobStore, |val| Ok(*val)) - .unwrap(), - StoreSerialized(1u64) - ); - assert_matches!( - &val1.inner.read().repr, - HashedCacheableRefRepr::Cache { .. } - ); - - // Load value to make it stored. - drop(val1); - let val2: TestRef = blob_store::load_from_store(&store, blob_ref).unwrap(); - assert_matches!( - &val2.inner.read().repr, - HashedCacheableRefRepr::Store { .. } - ); - - // Test stored value. Assert stored representation does not change. - assert_eq!( - val2.with_value(&store, |val| Ok(*val)).unwrap(), - StoreSerialized(1u64) - ); - assert_matches!( - &val2.inner.read().repr, - HashedCacheableRefRepr::Store { .. } - ); + assert_eq!(*val2.value(&store).unwrap(), StoreSerialized(1u64)); + assert_stored_repr(&val2); } /// Test hash in-memory value. @@ -513,13 +450,13 @@ mod tests { // Test hash in-memory value. // Assert that we don't need to read from the blob store by using UnreachableBlobStore let val1 = TestRef::new(StoreSerialized(1u64)); - assert_eq!(val1.inner.read().hash, None); + assert_eq!(val1.inner.hash.get(), None); assert_eq!( val1.hash(&UnreachableBlobStore).unwrap(), StoreSerialized(1u64).hash(&store).unwrap() ); assert_eq!( - val1.inner.read().hash, + val1.inner.hash.get().copied(), Some(StoreSerialized(1u64).hash(&store).unwrap()) ); } @@ -532,20 +469,17 @@ mod tests { // Store value to make it cached let val1 = TestRef::new(StoreSerialized(1u64)); blob_store::store_to_store(&mut store, &val1); - assert_matches!( - &val1.inner.read().repr, - HashedCacheableRefRepr::Cache { .. } - ); + assert_cached_repr(&val1); // Test hash cached value. // Assert that we don't need to read from the blob store by using UnreachableBlobStore - assert_eq!(val1.inner.read().hash, None); + assert_eq!(val1.inner.hash.get(), None); assert_eq!( val1.hash(&UnreachableBlobStore).unwrap(), StoreSerialized(1u64).hash(&store).unwrap() ); assert_eq!( - val1.inner.read().hash, + val1.inner.hash.get().copied(), Some(StoreSerialized(1u64).hash(&store).unwrap()) ); } @@ -560,25 +494,19 @@ mod tests { let blob_ref = blob_store::store_to_store(&mut store, &val1); drop(val1); let val2: TestRef = blob_store::load_from_store(&store, blob_ref).unwrap(); - assert_matches!( - &val2.inner.read().repr, - HashedCacheableRefRepr::Store { .. } - ); + assert_stored_repr(&val2); // Test hash stored value. Assert stored representation does not change. - assert_eq!(val2.inner.read().hash, None); + assert_eq!(val2.inner.hash.get(), None); assert_eq!( val2.hash(&store).unwrap(), StoreSerialized(1u64).hash(&store).unwrap() ); assert_eq!( - val2.inner.read().hash, + val2.inner.hash.get().copied(), Some(StoreSerialized(1u64).hash(&store).unwrap()) ); - assert_matches!( - &val2.inner.read().repr, - HashedCacheableRefRepr::Store { .. } - ); + assert_stored_repr(&val2); // Test hash again. This time we don't need to read from blob store, since we cache the hash. assert_eq!( @@ -597,30 +525,21 @@ mod tests { // Store value to blob store and assert representation is now cache. let blob_ref = blob_store::store_to_store(&mut store, &val1); - let (val_blob_ref, val_nested1) = assert_matches!(&val1.inner.read().repr, HashedCacheableRefRepr::Cache {reference,value} => { - (*reference, value.clone()) - }); - let val_nested_blob_ref = assert_matches!(&val_nested1.inner.read().repr, HashedCacheableRefRepr::Cache {reference,..} => { - *reference - }); + let (val_nested1, val_blob_ref) = assert_cached_repr(&val1); + let (_, val_nested_blob_ref) = assert_cached_repr(val_nested1); drop(val1); // Load value from blob store and assert representation is store. let val2: NestedTestRef = blob_store::load_from_store(&store, blob_ref).unwrap(); - assert_matches!(&val2.inner.read().repr, HashedCacheableRefRepr::Store {reference} => { - assert_eq!(*reference, val_blob_ref); - }); + assert_eq!(assert_stored_repr(&val2), val_blob_ref); // Cache the value and assert representation is now cache val2.cache_reference_values(&store).expect("cache"); - let val_nested2 = assert_matches!(&val2.inner.read().repr, HashedCacheableRefRepr::Cache {reference,value} => { - assert_eq!(*reference, val_blob_ref); - value.clone() - }); - assert_matches!(&val_nested2.inner.read().repr, HashedCacheableRefRepr::Cache {reference,value} => { - assert_eq!(*value, StoreSerialized(1)); - assert_eq!(*reference, val_nested_blob_ref); - }); + let (val_nested2, val_ref_tmp) = assert_cached_repr(&val2); + assert_eq!(val_ref_tmp, val_blob_ref); + let (val_tmp, val_ref_tmp) = assert_cached_repr(val_nested2); + assert_eq!(*val_tmp, StoreSerialized(1)); + assert_eq!(val_ref_tmp, val_nested_blob_ref); } } diff --git a/plt/plt-block-state/src/block_state/lfmb_tree.rs b/plt/plt-block-state/src/block_state/lfmb_tree.rs index e69fb43cbf..26c61e4577 100644 --- a/plt/plt-block-state/src/block_state/lfmb_tree.rs +++ b/plt/plt-block-state/src/block_state/lfmb_tree.rs @@ -441,7 +441,7 @@ impl Subtree { key ))); } - val_ref.with_value(loader, read) + read(val_ref.value(loader)?) } Subtree::Node(height, left_ref, right_ref) => { // The height'th bit in key decides if we should follow left `0` @@ -449,11 +449,11 @@ impl Subtree { // This allows us to check the invariant that the key must be identical to 0 // when we reach the leaf for the key. if is_nth_bit_set(*height, key) { - right_ref.with_value(loader, |right| { - right.lookup_value(loader, flip_nth_bit(*height, key), read) - }) + right_ref + .value(loader)? + .lookup_value(loader, flip_nth_bit(*height, key), read) } else { - left_ref.with_value(loader, |left| left.lookup_value(loader, key, read)) + left_ref.value(loader)?.lookup_value(loader, key, read) } } } @@ -544,14 +544,12 @@ impl Subtree { ) } else { // There is still room in the right branch of the tree, so insert the new value in the right branch. - let new_right = right_ref.with_value(loader, |right| { - right.insert_value( - loader, - Some(right_ref), - node_size - left_branch_size, - new_value, - ) - })?; + let new_right = right_ref.value(loader)?.insert_value( + loader, + Some(right_ref), + node_size - left_branch_size, + new_value, + )?; Subtree::Node( *height, left_ref.clone(), @@ -588,7 +586,7 @@ impl Subtree { key ))); } - let new_value = val_ref.with_value(loader, update)?; + let new_value = update(val_ref.value(loader)?)?; Subtree::Leaf(HashedCacheableRef::new(new_value)) } Subtree::Node(height, left_ref, right_ref) => { @@ -597,9 +595,11 @@ impl Subtree { // This allows us to check the invariant that the key must be identical to 0 // when we reach the leaf for the key. if is_nth_bit_set(*height, key) { - let new_right = right_ref.with_value(loader, |right| { - right.update_value(loader, flip_nth_bit(*height, key), update) - })?; + let new_right = right_ref.value(loader)?.update_value( + loader, + flip_nth_bit(*height, key), + update, + )?; Subtree::Node( *height, @@ -607,8 +607,7 @@ impl Subtree { HashedCacheableRef::new(new_right), ) } else { - let new_left = left_ref - .with_value(loader, |left| left.update_value(loader, key, update))?; + let new_left = left_ref.value(loader)?.update_value(loader, key, update)?; Subtree::Node( *height, @@ -681,7 +680,11 @@ where } let key = self.next_key; self.next_key.0 += 1; - Some(next_value.with_value(self.loader, |value| (self.read)(key, value))) + Some( + next_value + .value(self.loader) + .and_then(|val| (self.read)(key, val)), + ) } else if let Some(next_node_ref) = self.node_stack.pop() { if self.next_key.0 == self.tree_size { return Some(Err(BlockStateFailure::Invariant( @@ -727,13 +730,13 @@ fn next_value_push_right_branches( where F: FnMut(SubtreeKey, OwnedOrBorrowed<'_, V>) -> BlockStateResult, { - node_ref.with_value(loader, |node| match &*node { - Subtree::Leaf(value) => value.with_value(loader, |value| read(key, value)), + match &*node_ref.value(loader)? { + Subtree::Leaf(value) => read(key, value.value(loader)?), Subtree::Node(_, left_ref, right_ref) => { node_stack.push(right_ref.clone()); next_value_push_right_branches(key, loader, left_ref, node_stack, read) } - }) + } } impl Loadable for LfmbTree { @@ -1226,8 +1229,8 @@ mod tests { ) { match (subtree1, subtree2) { (Subtree::Leaf(val_ref1), Subtree::Leaf(val_ref2)) => { - let val1 = val_ref1.clone_or_load_value(loader).unwrap(); - let val2 = val_ref2.clone_or_load_value(loader).unwrap(); + let val1 = &*val_ref1.value(loader).unwrap(); + let val2 = &*val_ref2.value(loader).unwrap(); assert_eq!(val1, val2, "{}: leaf value", context); } ( @@ -1235,12 +1238,12 @@ mod tests { Subtree::Node(height2, left_ref2, right_ref2), ) => { assert_eq!(height1, height2); - let left1 = left_ref1.clone_or_load_value(loader).unwrap(); - let right1 = right_ref1.clone_or_load_value(loader).unwrap(); - let left2 = left_ref2.clone_or_load_value(loader).unwrap(); - let right2 = right_ref2.clone_or_load_value(loader).unwrap(); - assert_subtrees_eq(loader, &left1, &left2, context.clone()); - assert_subtrees_eq(loader, &right1, &right2, context.clone()); + let left1 = &*left_ref1.value(loader).unwrap(); + let right1 = &*right_ref1.value(loader).unwrap(); + let left2 = &*left_ref2.value(loader).unwrap(); + let right2 = &*right_ref2.value(loader).unwrap(); + assert_subtrees_eq(loader, left1, left2, context.clone()); + assert_subtrees_eq(loader, right1, right2, context.clone()); } (_, _) => { panic!("subtrees not equal: {:?}, {:?}", subtree1, subtree2); diff --git a/plt/plt-block-state/src/block_state/state/protocol_level_tokens.rs b/plt/plt-block-state/src/block_state/state/protocol_level_tokens.rs index 35537f1d0a..605c41fa5d 100644 --- a/plt/plt-block-state/src/block_state/state/protocol_level_tokens.rs +++ b/plt/plt-block-state/src/block_state/state/protocol_level_tokens.rs @@ -44,11 +44,9 @@ impl ProtocolLevelTokens { loader: &impl BlobStoreLoad, ) -> impl ExactSizeIterator> { self.tokens.values(loader, |_token_index, token| { - token.configuration.with_value(loader, |conf| { - Ok(match conf { - OwnedOrBorrowed::Owned(v) => v.0.token_id, - OwnedOrBorrowed::Borrowed(r) => r.0.token_id.clone(), - }) + Ok(match token.configuration.value(loader)? { + OwnedOrBorrowed::Owned(v) => v.0.token_id, + OwnedOrBorrowed::Borrowed(r) => r.0.token_id.clone(), }) }) } @@ -66,9 +64,7 @@ impl ProtocolLevelTokens { ) -> BlockStateResult { self.tokens .lookup_value(loader, token_index, |token| { - token - .key_value_state - .with_value(loader, |key_value_state| Ok(key_value_state.thaw())) + Ok(token.key_value_state.value(loader)?.thaw()) }) .ok_or_else(|| { BlockStateFailure::Invariant(format!("token not found by index: {:?}", token_index)) @@ -82,9 +78,7 @@ impl ProtocolLevelTokens { ) -> BlockStateResult { self.tokens .lookup_value(loader, token_index, |token| { - token - .configuration - .with_value(loader, |conf| Ok(conf.into_owned().0)) + Ok(token.configuration.value(loader)?.into_owned().0) }) .ok_or_else(|| { BlockStateFailure::Invariant(format!("token not found by index: {:?}", token_index)) @@ -200,9 +194,8 @@ impl Loadable for ProtocolLevelTokens { // rather wait until it is cached in memory before constructing the map. let token_id_map = tokens .values(loader, |token_index, plt| { - plt.configuration.with_value(loader, |conf| { - Ok((normalize_token_id(&conf.0.token_id), token_index)) - }) + let conf = plt.configuration.value(loader)?; + Ok((normalize_token_id(&conf.0.token_id), token_index)) }) .collect::>>()?; diff --git a/plt/plt-block-state/src/block_state/utils.rs b/plt/plt-block-state/src/block_state/utils.rs index bc5e25b1b0..8022989d6e 100644 --- a/plt/plt-block-state/src/block_state/utils.rs +++ b/plt/plt-block-state/src/block_state/utils.rs @@ -1,7 +1,6 @@ //! Block state utility types and functions. use std::ops::Deref; -use std::sync::{Arc, RwLock, RwLockReadGuard, RwLockWriteGuard}; /// Value of type `T` that is either owned or borrowed. /// @@ -33,45 +32,3 @@ impl Deref for OwnedOrBorrowed<'_, T> { } } } - -/// A link to a shared occurrence of a value `V`. -/// This is used in this module to construct trees, allowing for sharing of -/// values in trees and subtrees in case of the persistent tree. -/// -/// This [LockRef] achieves the following properties -/// - it is cheap to clone -/// - it allows for inner mutability -/// - it is safe to use in a concurrent context -#[derive(Debug)] -pub struct LockRef { - lock_ref: Arc>, -} - -/// Implement [`Clone`] explicitly, such that clonability does not depend on -/// if `V` is clonable. -impl Clone for LockRef { - fn clone(&self) -> Self { - Self { - lock_ref: self.lock_ref.clone(), - } - } -} - -impl LockRef { - /// Create new [`LockRef`] with given value. - pub fn new(value: V) -> Self { - Self { - lock_ref: Arc::new(RwLock::new(value)), - } - } - - /// Get read access guard for the linked value. - pub fn read(&self) -> RwLockReadGuard<'_, V> { - self.lock_ref.read().expect("LockRef poisoned") - } - - /// Get write access guard for the linked value. - pub fn write(&self) -> RwLockWriteGuard<'_, V> { - self.lock_ref.write().expect("LockRef poisoned") - } -}