From 283c2bbb79849f86be7aae3f8662b663b4026fb2 Mon Sep 17 00:00:00 2001 From: Anton Puhach Date: Fri, 30 Jun 2023 09:00:00 +0200 Subject: [PATCH] refactor: add FlatStateValue constructor for on-disk values (#9269) This logic is used in multiple places, so it makes sense to abstract it behind a method in `FlatStateValue`. `on_disk` name is chosen over `new` since this logic is only applicable for values to be persisted on disk. Later we will also add inlining for values as part of Flat Storage Delta and those will have lower inlining threshold, so we will have another constructor. --- core/primitives/src/state.rs | 16 ++++++++++ core/store/src/flat/delta.rs | 9 +----- core/store/src/flat/inlining_migration.rs | 37 +++++++++++++++++------ core/store/src/flat/mod.rs | 2 +- core/store/src/flat/types.rs | 8 ----- core/store/src/trie/state_parts.rs | 10 ++---- 6 files changed, 47 insertions(+), 35 deletions(-) diff --git a/core/primitives/src/state.rs b/core/primitives/src/state.rs index 1c7949fcbba..c7b784559f3 100644 --- a/core/primitives/src/state.rs +++ b/core/primitives/src/state.rs @@ -69,6 +69,22 @@ pub enum FlatStateValue { } impl FlatStateValue { + /// Defines value size threshold for flat state inlining. + /// It means that values having size greater than the threshold will be stored + /// in FlatState as `FlatStateValue::Ref`, otherwise the whole value will be + /// stored as `FlatStateValue::Inlined`. + /// See the following comment for reasoning behind the threshold value: + /// https://github.com/near/nearcore/issues/8243#issuecomment-1523049994 + pub const INLINE_DISK_VALUE_THRESHOLD: usize = 4000; + + pub fn on_disk(value: &[u8]) -> Self { + if value.len() <= Self::INLINE_DISK_VALUE_THRESHOLD { + Self::inlined(value) + } else { + Self::value_ref(value) + } + } + pub fn value_ref(value: &[u8]) -> Self { Self::Ref(ValueRef::new(value)) } diff --git a/core/store/src/flat/delta.rs b/core/store/src/flat/delta.rs index 4965805162d..17d70906757 100644 --- a/core/store/src/flat/delta.rs +++ b/core/store/src/flat/delta.rs @@ -7,7 +7,6 @@ use near_primitives::types::RawStateChangesWithTrieKey; use std::collections::HashMap; use std::sync::Arc; -use super::types::INLINE_DISK_VALUE_THRESHOLD; use super::{store_helper, BlockInfo}; use crate::{CryptoHash, StoreUpdate}; @@ -94,13 +93,7 @@ impl FlatStateChanges { .last() .expect("Committed entry should have at least one change") .data; - let flat_state_value = last_change.as_ref().map(|value| { - if value.len() <= INLINE_DISK_VALUE_THRESHOLD { - FlatStateValue::inlined(value) - } else { - FlatStateValue::value_ref(value) - } - }); + let flat_state_value = last_change.as_ref().map(|value| FlatStateValue::on_disk(value)); delta.insert(key, flat_state_value); } Self(delta) diff --git a/core/store/src/flat/inlining_migration.rs b/core/store/src/flat/inlining_migration.rs index d16a15ab6d4..4886d5e2026 100644 --- a/core/store/src/flat/inlining_migration.rs +++ b/core/store/src/flat/inlining_migration.rs @@ -22,7 +22,7 @@ use crate::{DBCol, Store, TrieDBStorage, TrieStorage}; use super::store_helper::{ decode_flat_state_db_key, get_flat_state_values_inlining_migration_status, }; -use super::types::{FlatStateValuesInliningMigrationStatus, INLINE_DISK_VALUE_THRESHOLD}; +use super::types::FlatStateValuesInliningMigrationStatus; use super::FlatStorageManager; pub struct FlatStateValuesInliningMigrationHandle { @@ -159,7 +159,7 @@ impl StateValueReader { } } -/// Inlines all FlatState values having length below `INLINE_DISK_VALUE_THRESHOLD`. +/// Inlines all FlatState values having length below `FlatStateValue::INLINE_DISK_VALUE_THRESHOLD`. /// Migration is safe to be executed in parallel with block processing, which /// is achieved by temporary preventing FlatState updates with /// `FlatStorageManager::set_flat_state_updates_mode`. @@ -216,7 +216,7 @@ pub fn inline_flat_state_values( }; PROCESSED_TOTAL_VALUES_SIZE.inc_by(value_size); if let FlatStateValue::Ref(value_ref) = fs_value { - if value_ref.length as usize <= INLINE_DISK_VALUE_THRESHOLD { + if value_ref.length as usize <= FlatStateValue::INLINE_DISK_VALUE_THRESHOLD { if min_key.is_none() { min_key = Some(key.to_vec()); } @@ -311,7 +311,6 @@ fn log_skipped(reason: &str, err: impl std::error::Error) { mod tests { use super::inline_flat_state_values; use crate::flat::store_helper::encode_flat_state_db_key; - use crate::flat::types::INLINE_DISK_VALUE_THRESHOLD; use crate::flat::{FlatStateValuesInliningMigrationHandle, FlatStorageManager}; use crate::{DBCol, NodeStorage, Store, TrieCachingStorage}; use borsh::{BorshDeserialize, BorshSerialize}; @@ -326,8 +325,14 @@ mod tests { fn full_migration() { let store = NodeStorage::test_opener().1.open().unwrap().get_hot_store(); let shard_uid = ShardLayout::v0_single_shard().get_shard_uids()[0]; - let values = - [vec![0], vec![1], vec![2; INLINE_DISK_VALUE_THRESHOLD + 1], vec![3], vec![4], vec![5]]; + let values = [ + vec![0], + vec![1], + vec![2; FlatStateValue::INLINE_DISK_VALUE_THRESHOLD + 1], + vec![3], + vec![4], + vec![5], + ]; populate_flat_store(&store, shard_uid, &values); let flat_storage_manager = @@ -363,8 +368,14 @@ mod tests { init_test_logger(); let store = NodeStorage::test_opener().1.open().unwrap().get_hot_store(); let shard_uid = ShardLayout::v0_single_shard().get_shard_uids()[0]; - let values = - [vec![0], vec![1], vec![2; INLINE_DISK_VALUE_THRESHOLD + 1], vec![3], vec![4], vec![5]]; + let values = [ + vec![0], + vec![1], + vec![2; FlatStateValue::INLINE_DISK_VALUE_THRESHOLD + 1], + vec![3], + vec![4], + vec![5], + ]; populate_flat_store(&store, shard_uid, &values); let flat_storage_manager = @@ -398,8 +409,14 @@ mod tests { init_test_logger(); let store = NodeStorage::test_opener().1.open().unwrap().get_hot_store(); let shard_uid = ShardLayout::v0_single_shard().get_shard_uids()[0]; - let values = - [vec![0], vec![1], vec![2; INLINE_DISK_VALUE_THRESHOLD + 1], vec![3], vec![4], vec![5]]; + let values = [ + vec![0], + vec![1], + vec![2; FlatStateValue::INLINE_DISK_VALUE_THRESHOLD + 1], + vec![3], + vec![4], + vec![5], + ]; populate_flat_store(&store, shard_uid, &values); let flat_storage_manager = diff --git a/core/store/src/flat/mod.rs b/core/store/src/flat/mod.rs index 72719bc0721..5e30aa71efe 100644 --- a/core/store/src/flat/mod.rs +++ b/core/store/src/flat/mod.rs @@ -42,7 +42,7 @@ pub use metrics::FlatStorageCreationMetrics; pub use storage::FlatStorage; pub use types::{ BlockInfo, FetchingStateStatus, FlatStateIterator, FlatStorageCreationStatus, FlatStorageError, - FlatStorageReadyStatus, FlatStorageStatus, INLINE_DISK_VALUE_THRESHOLD, + FlatStorageReadyStatus, FlatStorageStatus, }; pub(crate) const POISONED_LOCK_ERR: &str = "The lock was poisoned."; diff --git a/core/store/src/flat/types.rs b/core/store/src/flat/types.rs index a10449ab599..d8e31c2ea8d 100644 --- a/core/store/src/flat/types.rs +++ b/core/store/src/flat/types.rs @@ -4,14 +4,6 @@ use near_primitives::hash::CryptoHash; use near_primitives::state::FlatStateValue; use near_primitives::types::BlockHeight; -/// Defines value size threshold for flat state inlining. -/// It means that values having size greater than the threshold will be stored -/// in FlatState as `FlatStateValue::Ref`, otherwise the whole value will be -/// stored as `FlatStateValue::Inlined`. -/// See the following comment for reasoning behind the threshold value: -/// https://github.com/near/nearcore/issues/8243#issuecomment-1523049994 -pub const INLINE_DISK_VALUE_THRESHOLD: usize = 4000; - #[derive(BorshSerialize, BorshDeserialize, Debug, Copy, Clone, PartialEq, Eq)] pub struct BlockInfo { pub hash: CryptoHash, diff --git a/core/store/src/trie/state_parts.rs b/core/store/src/trie/state_parts.rs index c28d74b845b..f72155b8c14 100644 --- a/core/store/src/trie/state_parts.rs +++ b/core/store/src/trie/state_parts.rs @@ -16,7 +16,7 @@ //! Moreover, we include all left siblings for each path, because they are //! necessary to prove its position in the list of prefix sums. -use crate::flat::{FlatStateChanges, FlatStateIterator, INLINE_DISK_VALUE_THRESHOLD}; +use crate::flat::{FlatStateChanges, FlatStateIterator}; use crate::trie::iterator::TrieTraversalItem; use crate::trie::nibble_slice::NibbleSlice; use crate::trie::trie_storage::TrieMemoryPartialStorage; @@ -463,13 +463,7 @@ impl Trie { let value = trie.storage.retrieve_raw_bytes(&hash)?; map.entry(hash).or_insert_with(|| (value.to_vec(), 0)).1 += 1; if let Some(trie_key) = key { - // TODO: Refactor to hide this condition in an abstraction. - // For example, it could be `FlatStateValue::new(&value)`. - let flat_state_value = if value.len() <= INLINE_DISK_VALUE_THRESHOLD { - FlatStateValue::inlined(&value) - } else { - FlatStateValue::value_ref(&value) - }; + let flat_state_value = FlatStateValue::on_disk(&value); flat_state_delta.insert(trie_key.clone(), Some(flat_state_value)); if is_contract_code_key(&trie_key) { contract_codes.push(ContractCode::new(value.to_vec(), None));