Don't clone values when calculating storage root (#6108)

* Don't clone values when calculating storage root

Instead of cloning all the keys and values of the overlay when
calculating the storage root, we pass all the values by reference. This
should probably bring some performance improvements when calculating the
storage root.

* no cow version (#6113)

Co-authored-by: cheme <emericchevalier.pro@gmail.com>

substrate/primitives/state-machine/src/backend.rs

@@ -20,7 +20,6 @@
 use hash_db::Hasher;
 use codec::{Decode, Encode};
 use sp_core::{traits::RuntimeCode, storage::{ChildInfo, well_known_keys}};
-
 use crate::{
 	trie_backend::TrieBackend,
 	trie_backend_essence::TrieBackendStorage,
@@ -119,22 +118,19 @@ pub trait Backend<H: Hasher>: std::fmt::Debug {
 	/// Calculate the storage root, with given delta over what is already stored in
 	/// the backend, and produce a "transaction" that can be used to commit.
 	/// Does not include child storage updates.
-	fn storage_root<I>(&self, delta: I) -> (H::Out, Self::Transaction)
-	where
-		I: IntoIterator<Item=(StorageKey, Option<StorageValue>)>,
-		H::Out: Ord;
+	fn storage_root<'a>(
+		&self,
+		delta: impl Iterator<Item=(&'a [u8], Option<&'a [u8]>)>,
+	) -> (H::Out, Self::Transaction) where H::Out: Ord;
 
 	/// Calculate the child storage root, with given delta over what is already stored in
 	/// the backend, and produce a "transaction" that can be used to commit. The second argument
 	/// is true if child storage root equals default storage root.
-	fn child_storage_root<I>(
+	fn child_storage_root<'a>(
 		&self,
 		child_info: &ChildInfo,
-		delta: I,
-	) -> (H::Out, bool, Self::Transaction)
-	where
-		I: IntoIterator<Item=(StorageKey, Option<StorageValue>)>,
-		H::Out: Ord;
+		delta: impl Iterator<Item=(&'a [u8], Option<&'a [u8]>)>,
+	) -> (H::Out, bool, Self::Transaction) where H::Out: Ord;
 
 	/// Get all key/value pairs into a Vec.
 	fn pairs(&self) -> Vec<(StorageKey, StorageValue)>;
@@ -165,17 +161,14 @@ pub trait Backend<H: Hasher>: std::fmt::Debug {
 	/// Calculate the storage root, with given delta over what is already stored
 	/// in the backend, and produce a "transaction" that can be used to commit.
 	/// Does include child storage updates.
-	fn full_storage_root<I1, I2i, I2>(
+	fn full_storage_root<'a>(
 		&self,
-		delta: I1,
-		child_deltas: I2)
-	-> (H::Out, Self::Transaction)
-	where
-		I1: IntoIterator<Item=(StorageKey, Option<StorageValue>)>,
-		I2i: IntoIterator<Item=(StorageKey, Option<StorageValue>)>,
-		I2: IntoIterator<Item=(ChildInfo, I2i)>,
-		H::Out: Ord + Encode,
-	{
+		delta: impl Iterator<Item=(&'a [u8], Option<&'a [u8]>)>,
+		child_deltas: impl Iterator<Item = (
+			&'a ChildInfo,
+			impl Iterator<Item=(&'a [u8], Option<&'a [u8]>)>,
+		)>,
+	) -> (H::Out, Self::Transaction) where H::Out: Ord + Encode {
 		let mut txs: Self::Transaction = Default::default();
 		let mut child_roots: Vec<_> = Default::default();
 		// child first
@@ -190,8 +183,13 @@ pub trait Backend<H: Hasher>: std::fmt::Debug {
 				child_roots.push((prefixed_storage_key.into_inner(), Some(child_root.encode())));
 			}
 		}
-		let (root, parent_txs) = self.storage_root(
-			delta.into_iter().chain(child_roots.into_iter())
+		let (root, parent_txs) = self.storage_root(delta
+			.map(|(k, v)| (&k[..], v.as_ref().map(|v| &v[..])))
+			.chain(
+				child_roots
+					.iter()
+					.map(|(k, v)| (&k[..], v.as_ref().map(|v| &v[..])))
+			)
 		);
 		txs.consolidate(parent_txs);
 		(root, txs)
@@ -214,7 +212,7 @@ pub trait Backend<H: Hasher>: std::fmt::Debug {
 	}
 
 	/// Commit given transaction to storage.
-	fn commit(&self, _storage_root: H::Out, _transaction: Self::Transaction) -> Result<(), Self::Error> {
+	fn commit(&self, _: H::Out, _: Self::Transaction) -> Result<(), Self::Error> {
 		unimplemented!()
 	}
 }
@@ -269,23 +267,18 @@ impl<'a, T: Backend<H>, H: Hasher> Backend<H> for &'a T {
 		(*self).for_child_keys_with_prefix(child_info, prefix, f)
 	}
 
-	fn storage_root<I>(&self, delta: I) -> (H::Out, Self::Transaction)
-	where
-		I: IntoIterator<Item=(StorageKey, Option<StorageValue>)>,
-		H::Out: Ord,
-	{
+	fn storage_root<'b>(
+		&self,
+		delta: impl Iterator<Item=(&'b [u8], Option<&'b [u8]>)>,
+	) -> (H::Out, Self::Transaction) where H::Out: Ord {
 		(*self).storage_root(delta)
 	}
 
-	fn child_storage_root<I>(
+	fn child_storage_root<'b>(
 		&self,
 		child_info: &ChildInfo,
-		delta: I,
-	) -> (H::Out, bool, Self::Transaction)
-	where
-		I: IntoIterator<Item=(StorageKey, Option<StorageValue>)>,
-		H::Out: Ord,
-	{
+		delta: impl Iterator<Item=(&'b [u8], Option<&'b [u8]>)>,
+	) -> (H::Out, bool, Self::Transaction) where H::Out: Ord {
 		(*self).child_storage_root(child_info, delta)
 	}
 

substrate/primitives/state-machine/src/basic.rs

@@ -295,7 +295,7 @@ impl Externalities for BasicExternalities {
 		child_info: &ChildInfo,
 	) -> Vec<u8> {
 		if let Some(child) = self.inner.children_default.get(child_info.storage_key()) {
-			let delta = child.data.clone().into_iter().map(|(k, v)| (k, Some(v)));
+			let delta = child.data.iter().map(|(k, v)| (k.as_ref(), Some(v.as_ref())));
 			crate::in_memory_backend::new_in_mem::<Blake2Hasher>()
 				.child_storage_root(&child.child_info, delta).0
 		} else {

substrate/primitives/state-machine/src/ext.rs

@@ -481,7 +481,7 @@ where
 			if let Some(child_info) = self.overlay.default_child_info(storage_key) {
 				let (root, is_empty, _) = {
 					let delta = self.overlay.changes(Some(child_info))
-						.map(|(k, v)| (k.clone(), v.value().cloned()));
+						.map(|(k, v)| (k.as_ref(), v.value().map(AsRef::as_ref)));
 
 					self.backend.child_storage_root(child_info, delta)
 				};

substrate/primitives/state-machine/src/overlayed_changes.rs

@@ -26,13 +26,10 @@ use crate::{
 	stats::StateMachineStats,
 };
 
-#[cfg(test)]
-use std::iter::FromIterator;
-use std::collections::{HashMap, BTreeMap, BTreeSet};
+use std::{mem, ops, collections::{HashMap, BTreeMap, BTreeSet}};
 use codec::{Decode, Encode};
 use sp_core::storage::{well_known_keys::EXTRINSIC_INDEX, ChildInfo, ChildType};
 use sp_core::offchain::storage::OffchainOverlayedChanges;
-use std::{mem, ops};
 
 use hash_db::Hasher;
 
@@ -178,7 +175,7 @@ impl<Transaction: Default, H: Hasher, N: BlockNumber> Default for StorageChanges
 }
 
 #[cfg(test)]
-impl FromIterator<(StorageKey, OverlayedValue)> for OverlayedChangeSet {
+impl std::iter::FromIterator<(StorageKey, OverlayedValue)> for OverlayedChangeSet {
 	fn from_iter<T: IntoIterator<Item = (StorageKey, OverlayedValue)>>(iter: T) -> Self {
 		Self {
 			top: iter.into_iter().collect(),
@@ -646,22 +643,29 @@ impl OverlayedChanges {
 				.chain(self.committed.children_default.keys());
 		let child_delta_iter = child_storage_keys.map(|storage_key|
 			(
-				self.default_child_info(storage_key).cloned()
+				self.default_child_info(storage_key)
 					.expect("child info initialized in either committed or prospective"),
 				self.committed.children_default.get(storage_key)
 					.into_iter()
-					.flat_map(|(map, _)| map.iter().map(|(k, v)| (k.clone(), v.value.clone())))
+					.flat_map(|(map, _)|
+						map.iter().map(|(k, v)| (&k[..], v.value().map(|v| &v[..])))
+					)
 					.chain(
 						self.prospective.children_default.get(storage_key)
 							.into_iter()
-							.flat_map(|(map, _)| map.iter().map(|(k, v)| (k.clone(), v.value.clone())))
+							.flat_map(|(map, _)|
+								map.iter().map(|(k, v)| (&k[..], v.value().map(|v| &v[..])))
+							)
 					),
 			)
 		);
 
 		// compute and memoize
-		let delta = self.committed.top.iter().map(|(k, v)| (k.clone(), v.value.clone()))
-			.chain(self.prospective.top.iter().map(|(k, v)| (k.clone(), v.value.clone())));
+		let delta = self.committed
+			.top
+			.iter()
+			.map(|(k, v)| (&k[..], v.value().map(|v| &v[..])))
+			.chain(self.prospective.top.iter().map(|(k, v)| (&k[..], v.value().map(|v| &v[..]))));
 
 		let (root, transaction) = backend.full_storage_root(delta, child_delta_iter);
 

substrate/primitives/state-machine/src/proving_backend.rs

@@ -17,7 +17,7 @@
 
 //! Proving state machine backend.
 
-use std::sync::Arc;
+use std::{sync::Arc, collections::HashMap};
 use parking_lot::RwLock;
 use codec::{Decode, Codec};
 use log::debug;
@@ -26,13 +26,10 @@ use sp_trie::{
 	MemoryDB, empty_child_trie_root, read_trie_value_with, read_child_trie_value_with,
 	record_all_keys, StorageProof,
 };
-pub use sp_trie::Recorder;
-pub use sp_trie::trie_types::{Layout, TrieError};
+pub use sp_trie::{Recorder, trie_types::{Layout, TrieError}};
 use crate::trie_backend::TrieBackend;
 use crate::trie_backend_essence::{Ephemeral, TrieBackendEssence, TrieBackendStorage};
-use crate::{Error, ExecutionError, Backend};
-use std::collections::HashMap;
-use crate::DBValue;
+use crate::{Error, ExecutionError, Backend, DBValue};
 use sp_core::storage::ChildInfo;
 
 /// Patricia trie-based backend specialized in get value proofs.
@@ -260,21 +257,18 @@ impl<'a, S, H> Backend<H> for ProvingBackend<'a, S, H>
 		self.0.child_keys(child_info, prefix)
 	}
 
-	fn storage_root<I>(&self, delta: I) -> (H::Out, Self::Transaction)
-		where I: IntoIterator<Item=(Vec<u8>, Option<Vec<u8>>)>
-	{
+	fn storage_root<'b>(
+		&self,
+		delta: impl Iterator<Item=(&'b [u8], Option<&'b [u8]>)>,
+	) -> (H::Out, Self::Transaction) where H::Out: Ord {
 		self.0.storage_root(delta)
 	}
 
-	fn child_storage_root<I>(
+	fn child_storage_root<'b>(
 		&self,
 		child_info: &ChildInfo,
-		delta: I,
-	) -> (H::Out, bool, Self::Transaction)
-	where
-		I: IntoIterator<Item=(Vec<u8>, Option<Vec<u8>>)>,
-		H::Out: Ord
-	{
+		delta: impl Iterator<Item=(&'b [u8], Option<&'b [u8]>)>,
+	) -> (H::Out, bool, Self::Transaction) where H::Out: Ord {
 		self.0.child_storage_root(child_info, delta)
 	}
 
@@ -393,9 +387,9 @@ mod tests {
 		let in_memory = InMemoryBackend::<BlakeTwo256>::default();
 		let mut in_memory = in_memory.update(contents);
 		let child_storage_keys = vec![child_info_1.to_owned(), child_info_2.to_owned()];
-		let in_memory_root = in_memory.full_storage_root::<_, Vec<_>, _>(
-			::std::iter::empty(),
-			child_storage_keys.into_iter().map(|k|(k.to_owned(), Vec::new()))
+		let in_memory_root = in_memory.full_storage_root(
+			std::iter::empty(),
+			child_storage_keys.iter().map(|k|(k, std::iter::empty()))
 		).0;
 		(0..64).for_each(|i| assert_eq!(
 			in_memory.storage(&[i]).unwrap().unwrap(),

substrate/primitives/state-machine/src/trie_backend.rs

@@ -167,9 +167,10 @@ impl<S: TrieBackendStorage<H>, H: Hasher> Backend<H> for TrieBackend<S, H> where
 		collect_all().map_err(|e| debug!(target: "trie", "Error extracting trie keys: {}", e)).unwrap_or_default()
 	}
 
-	fn storage_root<I>(&self, delta: I) -> (H::Out, S::Overlay)
-		where I: IntoIterator<Item=(StorageKey, Option<StorageValue>)>
-	{
+	fn storage_root<'a>(
+		&self,
+		delta: impl Iterator<Item=(&'a [u8], Option<&'a [u8]>)>,
+	) -> (H::Out, Self::Transaction) where H::Out: Ord {
 		let mut write_overlay = S::Overlay::default();
 		let mut root = *self.essence.root();
 
@@ -179,8 +180,7 @@ impl<S: TrieBackendStorage<H>, H: Hasher> Backend<H> for TrieBackend<S, H> where
 				&mut write_overlay,
 			);
 
-			let delta: Vec<_> = delta.into_iter().collect();
-			match delta_trie_root::<Layout<H>, _, _, _, _>(&mut eph, root, delta) {
+			match delta_trie_root::<Layout<H>, _, _, _, _, _>(&mut eph, root, delta) {
 				Ok(ret) => root = ret,
 				Err(e) => warn!(target: "trie", "Failed to write to trie: {}", e),
 			}
@@ -189,15 +189,11 @@ impl<S: TrieBackendStorage<H>, H: Hasher> Backend<H> for TrieBackend<S, H> where
 		(root, write_overlay)
 	}
 
-	fn child_storage_root<I>(
+	fn child_storage_root<'a>(
 		&self,
 		child_info: &ChildInfo,
-		delta: I,
-	) -> (H::Out, bool, Self::Transaction)
-	where
-		I: IntoIterator<Item=(StorageKey, Option<StorageValue>)>,
-		H::Out: Ord,
-	{
+		delta: impl Iterator<Item=(&'a [u8], Option<&'a [u8]>)>,
+	) -> (H::Out, bool, Self::Transaction) where H::Out: Ord {
 		let default_root = match child_info.child_type() {
 			ChildType::ParentKeyId => empty_child_trie_root::<Layout<H>>()
 		};
@@ -219,11 +215,11 @@ impl<S: TrieBackendStorage<H>, H: Hasher> Backend<H> for TrieBackend<S, H> where
 				&mut write_overlay,
 			);
 
-			match child_delta_trie_root::<Layout<H>, _, _, _, _, _>(
+			match child_delta_trie_root::<Layout<H>, _, _, _, _, _, _>(
 				child_info.keyspace(),
 				&mut eph,
 				root,
-				delta
+				delta,
 			) {
 				Ok(ret) => root = ret,
 				Err(e) => warn!(target: "trie", "Failed to write to trie: {}", e),
@@ -252,7 +248,7 @@ impl<S: TrieBackendStorage<H>, H: Hasher> Backend<H> for TrieBackend<S, H> where
 
 #[cfg(test)]
 pub mod tests {
-	use std::collections::HashSet;
+	use std::{collections::HashSet, iter};
 	use sp_core::H256;
 	use codec::Encode;
 	use sp_trie::{TrieMut, PrefixedMemoryDB, trie_types::TrieDBMut, KeySpacedDBMut};
@@ -328,19 +324,21 @@ pub mod tests {
 
 	#[test]
 	fn storage_root_is_non_default() {
-		assert!(test_trie().storage_root(::std::iter::empty()).0 != H256::repeat_byte(0));
+		assert!(test_trie().storage_root(iter::empty()).0 != H256::repeat_byte(0));
 	}
 
 	#[test]
 	fn storage_root_transaction_is_empty() {
-		assert!(test_trie().storage_root(::std::iter::empty()).1.drain().is_empty());
+		assert!(test_trie().storage_root(iter::empty()).1.drain().is_empty());
 	}
 
 	#[test]
 	fn storage_root_transaction_is_non_empty() {
-		let (new_root, mut tx) = test_trie().storage_root(vec![(b"new-key".to_vec(), Some(b"new-value".to_vec()))]);
+		let (new_root, mut tx) = test_trie().storage_root(
+			iter::once((&b"new-key"[..], Some(&b"new-value"[..]))),
+		);
 		assert!(!tx.drain().is_empty());
-		assert!(new_root != test_trie().storage_root(::std::iter::empty()).0);
+		assert!(new_root != test_trie().storage_root(iter::empty()).0);
 	}
 
 	#[test]