Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Avoid some clone when creating keys to use in StorageMap methods #1876

Closed

Description

@gui1117

When we create a StorageMap with a tuple for example (T::AccountId, u64) here in substrate kitties then to use it we often need to clone AccountId (it doesn't implements Copy) like here

In this example it is not an issue as AccountId is meant to be an integer such as u64 and clone it is probably same cost as referencing it.

But in other situation it could be an issue. I don't know if such situations exist though (long key that shouldn't be cloned, (caching there encoding, I don't know)).

A solution I have in mind for that is using a trait SameEncodingAs as such:

pub trait SameEncodeAs<T>: codec::Encode {}

impl<'a, 'b, A: codec::Encode, B: codec::Encode> SameEncodeAs<(A, B)> for (&'a A, &'b B) {}
impl<T: codec::Encode> SameEncodeAs<T> for T {}

a partial implement is then (based on ebe4c97):

diff --git a/srml/example/src/lib.rs b/srml/example/src/lib.rs
index 4f4e0b96..98b93d81 100644
--- a/srml/example/src/lib.rs
+++ b/srml/example/src/lib.rs
@@ -20,7 +20,7 @@
 // Ensure we're `no_std` when compiling for Wasm.
 #![cfg_attr(not(feature = "std"), no_std)]
 
-use srml_support::{StorageValue, dispatch::Result, decl_module, decl_storage, decl_event};
+use srml_support::{StorageValue, dispatch::Result, decl_module, decl_storage, decl_event, storage::generator::StorageMap, storage::RuntimeStorage};
 use system::ensure_signed;
 
 /// Our module's configuration trait. All our types and consts go in here. If the
@@ -63,8 +63,10 @@ decl_storage! {
 		// `fn getter_name(key: KeyType) -> ValueType` for map items.
 		Dummy get(dummy) config(): Option<T::Balance>;
 
+		TestDouble: map (T::AccountId, T::AccountId) => u32;
+
 		// A map that has enumerable entries.
-		Bar get(bar) config(): linked_map T::AccountId => T::Balance;
+		// Bar get(bar) config(): linked_map T::AccountId => T::Balance;
 
 
 		// this one uses the default, we'll demonstrate the usage of 'mutate' API.
@@ -162,6 +164,8 @@ decl_module! {
 			// This is a public call, so we ensure that the origin is some signed account.
 			let _sender = ensure_signed(origin)?;
 
+			<TestDouble<T>>::insert(&(&_sender, &_sender), &3, &RuntimeStorage);
+
 			// Read the value of dummy from storage.
 			// let dummy = Self::dummy();
 			// Will also work using the `::get` on the storage item type itself:
diff --git a/srml/support/procedural/src/storage/impls.rs b/srml/support/procedural/src/storage/impls.rs
index e008c81f..5920f0ed 100644
--- a/srml/support/procedural/src/storage/impls.rs
+++ b/srml/support/procedural/src/storage/impls.rs
@@ -145,7 +145,7 @@ impl<'a> Impls<'a> {
 				}
 
 				/// Get the storage key used to fetch a value corresponding to a specific key.
-				fn key_for(x: &#kty) -> #scrate::rstd::vec::Vec<u8> {
+				fn key_for<KSame: #scrate::storage::generator::SameEncodeAs<#kty>>(x: &KSame) -> Vec<u8> {
 					let mut key = #prefix.as_bytes().to_vec();
 					#scrate::codec::Encode::encode_to(x, &mut key);
 					key
diff --git a/srml/support/src/storage/generator.rs b/srml/support/src/storage/generator.rs
index e9c38e87..a49098a3 100644
--- a/srml/support/src/storage/generator.rs
+++ b/srml/support/src/storage/generator.rs
@@ -181,6 +181,11 @@ pub trait StorageList<T: codec::Codec> {
 	fn clear<S: Storage>(storage: &S);
 }
 
+pub trait SameEncodeAs<T>: codec::Encode {}
+
+impl<'a, 'b, A: codec::Encode, B: codec::Encode> SameEncodeAs<(A, B)> for (&'a A, &'b B) {}
+impl<T: codec::Encode> SameEncodeAs<T> for T {}
+
 /// A strongly-typed map in storage.
 pub trait StorageMap<K: codec::Codec, V: codec::Codec> {
 	/// The type that get/take returns.
@@ -190,7 +195,7 @@ pub trait StorageMap<K: codec::Codec, V: codec::Codec> {
 	fn prefix() -> &'static [u8];
 
 	/// Get the storage key used to fetch a value corresponding to a specific key.
-	fn key_for(x: &K) -> Vec<u8>;
+	fn key_for<KSame: SameEncodeAs<K>>(x: &KSame) -> Vec<u8>;
 
 	/// true if the value is defined in storage.
 	fn exists<S: Storage>(key: &K, storage: &S) -> bool {
@@ -204,7 +209,7 @@ pub trait StorageMap<K: codec::Codec, V: codec::Codec> {
 	fn take<S: Storage>(key: &K, storage: &S) -> Self::Query;
 
 	/// Store a value to be associated with the given key from the map.
-	fn insert<S: Storage>(key: &K, val: &V, storage: &S) {
+	fn insert<S: Storage, KSame: SameEncodeAs<K>>(key: &KSame, val: &V, storage: &S) {
 		storage.put(&Self::key_for(key)[..], val);
 	}

Activity

  1. added this to the As-and-when milestone on Feb 26, 2019
  2. gui1117 commented on Feb 26, 2019

    @gui1117
    ContributorAuthor

    note: that could also be useful for tuple values

  3. bkchr commented on Feb 26, 2019

    @bkchr
    Member

    So you just propose to take the keys by reference?

    FWIW Copy requires Clone: https://doc.rust-lang.org/std/marker/trait.Copy.html
    Copy just tells the compiler that Clone is not expensive and you can move by copy.

  4. gui1117 commented on Feb 26, 2019

    @gui1117
    ContributorAuthor

    We already take key and value by reference, the thing is that &(T::AccountId, u64) does take ownership of T::AccoundId I propose to be able to pass whatever type implements SameEncodingAs<(T::AccountId, u64)> which would be implemented in a generic way for (&T::AccountId, u64)

    EDIT: Yes in a way I propose to be able to take inner type of a tuple (or anything if user defined the trait for his own structs) by reference

  5. burdges commented on Feb 26, 2019

    @burdges
  6. rphmeier commented on Feb 27, 2019

    @rphmeier
    Contributor
    impl<'a, T: Encode + 'a> SameEncodeAs<T> for &'a T {}
    impl<A, B, X, Y> SameEncodeAs<(X, Y)> for (A, B) where A: SameEncodeAs<X>, B: SameEncodeAs<Y> {}
    ...

    might work better because then you can mix and match.

  7. added
    I9-optimisationAn enhancement to provide better overall performance in terms of time-to-completion for a task.
    on Feb 27, 2019
  8. bkchr commented on Oct 1, 2019

    @bkchr
    Member
    impl<'a, T: Encode + 'a> SameEncodeAs<T> for &'a T {}
    impl<A, B, X, Y> SameEncodeAs<(X, Y)> for (A, B) where A: SameEncodeAs<X>, B: SameEncodeAs<Y> {}
    ...

    might work better because then you can mix and match.

    Quite interesting that I came up with a very similar api some months later, without remembering your proposal :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    I9-optimisationAn enhancement to provide better overall performance in terms of time-to-completion for a task.

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions