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

Add special tag to exclude runtime storage items from benchmarking #12205

Merged
merged 72 commits into from
Sep 16, 2022
Merged
Show file tree
Hide file tree
Changes from 68 commits
Commits
Show all changes
72 commits
Select commit Hold shift + click to select a range
f08d3ae
initial setup
sam0x17 Aug 30, 2022
c381aef
add WhitelistedStorageKeys trait
sam0x17 Aug 30, 2022
1275c42
add (A, B) tuple implementation for whitelisted_storage_keys()
sam0x17 Aug 31, 2022
84beb0f
fix formatting
sam0x17 Aug 31, 2022
3699c19
implement WhitelistedStorageKeys for all tuple combinations
sam0x17 Aug 31, 2022
3056275
impl_for_tuples up to 128 for WhitelistedStorageKeys
sam0x17 Aug 31, 2022
adc7585
refactor to #[benchmarking(cached)]
sam0x17 Aug 31, 2022
f645511
tweak error message and mark BlockNumber as cached
sam0x17 Aug 31, 2022
1b8af15
add benchmarking(cached) to the other default types
sam0x17 Sep 1, 2022
2a1ae01
add docs for benchmarking(cached)
sam0x17 Sep 2, 2022
586bafb
properly parse storage type declaration
sam0x17 Sep 6, 2022
cfcd073
make storage_alias structs public so we can use them in this macro
sam0x17 Sep 6, 2022
8534891
use BTreeMap since TrackedStorageKey missing Ord outside of std
sam0x17 Sep 6, 2022
e2550b4
make WhitelistedStorageKeys accessible
sam0x17 Sep 6, 2022
157fb29
basic detection of benchmarking(cached) :boom:
sam0x17 Sep 6, 2022
5318167
proper parsing of #[benchmarking(cached)] from pallet parse macro
sam0x17 Sep 7, 2022
8cde8fe
store presence of #[benchmarking(cached)] macro on StorageDef
sam0x17 Sep 7, 2022
4cccd8a
compiling blank impl for WhitelistedStorageKeys
sam0x17 Sep 8, 2022
f21315c
move impl to expand_pallet_struct
sam0x17 Sep 8, 2022
faa63a7
use frame_support::sp_std::vec::Vec properly
sam0x17 Sep 8, 2022
73b01f1
successfully compiling with storage info loaded into a variable :boom:
sam0x17 Sep 9, 2022
fc6d239
plausible implementation for whitelisted_storage_keys()
sam0x17 Sep 9, 2022
f920bb9
use Pallet::whitelisted_storage_keys() instead of hard-coded list
sam0x17 Sep 9, 2022
0040a45
AllPallets::whitelisted_storage_keys() properly working :boom:
sam0x17 Sep 9, 2022
4a0fe7f
collect storage names
sam0x17 Sep 9, 2022
8ab6ab3
whitelisted_storage_keys() impl working :boom:
sam0x17 Sep 9, 2022
8ca026e
clean up
sam0x17 Sep 9, 2022
58a6229
fix compiler error
sam0x17 Sep 10, 2022
854a102
just one compiler error
sam0x17 Sep 10, 2022
771202c
fix doc compiler error
sam0x17 Sep 12, 2022
fe856ac
use better import path
sam0x17 Sep 12, 2022
0936ca4
fix comment
sam0x17 Sep 12, 2022
a72a280
whoops
sam0x17 Sep 12, 2022
c4ecb9a
whoops again
sam0x17 Sep 12, 2022
c6fd281
fix macro import issue
sam0x17 Sep 12, 2022
f360944
cargo fmt
sam0x17 Sep 12, 2022
92d5f5e
mark example as ignore
sam0x17 Sep 12, 2022
da51def
use keyword tokens instead of string parsing
sam0x17 Sep 13, 2022
e00420f
fix keyword-based parsing of benchmarking(cached)
sam0x17 Sep 13, 2022
a63acdf
preliminary spec for check_whitelist()
sam0x17 Sep 13, 2022
1a0ba3f
add additional test for benchmarking whitelist
sam0x17 Sep 13, 2022
d0ee2c6
add TODO note
sam0x17 Sep 13, 2022
2d1d637
remove irrelevant line from example
sam0x17 Sep 13, 2022
9ce39d2
use filter_map instead of filter and map
sam0x17 Sep 13, 2022
33828ea
simplify syntax
sam0x17 Sep 13, 2022
49b13ac
clean up
sam0x17 Sep 13, 2022
a2e8185
fix test
sam0x17 Sep 13, 2022
557a0c2
fix tests
sam0x17 Sep 13, 2022
d20bfe9
use keyword parsing instead of string parsing
sam0x17 Sep 14, 2022
1d31abe
use collect() instead of a for loop
sam0x17 Sep 14, 2022
dcb6312
fix compiler error
sam0x17 Sep 15, 2022
6e66eb1
clean up benchmarking(cached) marking code
sam0x17 Sep 15, 2022
dcda735
use cloned()
sam0x17 Sep 15, 2022
3f7bbc5
refactor to not use panic! and remove need for pub types in storage_a…
sam0x17 Sep 15, 2022
975f371
remove unneeded use
sam0x17 Sep 15, 2022
5ff3623
remove unneeded visibility changes
sam0x17 Sep 15, 2022
7214d0f
don't manually hard code hash for treasury account as hex
sam0x17 Sep 16, 2022
6e49069
proper Ord, PartialOrd, and Hash impls for TrackedStorageKey
sam0x17 Sep 16, 2022
4a71bd4
use BTreeSet instead of BTreeMap
sam0x17 Sep 16, 2022
7752321
fix comments
sam0x17 Sep 16, 2022
72a8ac8
cargo fmt
sam0x17 Sep 16, 2022
20e1375
switch to pallet::whitelist and re-do it basti's way :D
sam0x17 Sep 16, 2022
eb46fbb
make PartialOrd for TrackedStorageKey consistent with Ord
sam0x17 Sep 16, 2022
fd23e9e
more correct implementation of hash-related traits for TrackedStorageKey
sam0x17 Sep 16, 2022
d483a7a
fix integration test
sam0x17 Sep 16, 2022
05db4e5
update TODO
sam0x17 Sep 16, 2022
cb4ed97
remove unused keyword
sam0x17 Sep 16, 2022
4faf150
remove more unused keywords
sam0x17 Sep 16, 2022
91f33f0
use into_iter()
sam0x17 Sep 16, 2022
42127fc
Update frame/support/procedural/src/pallet/parse/mod.rs
sam0x17 Sep 16, 2022
cbf5959
add docs for whitelisted
sam0x17 Sep 16, 2022
ae1bf10
fix comment
sam0x17 Sep 16, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 39 additions & 12 deletions bin/node-template/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -514,18 +514,8 @@ impl_runtime_apis! {
impl frame_system_benchmarking::Config for Runtime {}
impl baseline::Config for Runtime {}

let whitelist: Vec<TrackedStorageKey> = vec![
// Block Number
hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef702a5c1b19ab7a04f536c519aca4983ac").to_vec().into(),
// Total Issuance
hex_literal::hex!("c2261276cc9d1f8598ea4b6a74b15c2f57c875e4cff74148e4628f264b974c80").to_vec().into(),
// Execution Phase
hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef7ff553b5a9862a516939d82b3d3d8661a").to_vec().into(),
// Event Count
hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef70a98fdbe9ce6c55837576c60c7af3850").to_vec().into(),
// System Events
hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef780d41e5e16056765bc8461851072c9d7").to_vec().into(),
];
use frame_support::traits::WhitelistedStorageKeys;
let whitelist: Vec<TrackedStorageKey> = AllPalletsWithSystem::whitelisted_storage_keys();

let mut batches = Vec::<BenchmarkBatch>::new();
let params = (&config, &whitelist);
Expand Down Expand Up @@ -556,3 +546,40 @@ impl_runtime_apis! {
}
}
}

#[cfg(test)]
mod tests {
use super::*;
use frame_support::traits::WhitelistedStorageKeys;
use sp_core::hexdisplay::HexDisplay;
use std::collections::HashSet;

#[test]
fn check_whitelist() {
let whitelist: HashSet<String> = AllPalletsWithSystem::whitelisted_storage_keys()
.iter()
.map(|e| HexDisplay::from(&e.key).to_string())
.collect();

// Block Number
assert!(
whitelist.contains("26aa394eea5630e07c48ae0c9558cef702a5c1b19ab7a04f536c519aca4983ac")
);
// Total Issuance
assert!(
whitelist.contains("c2261276cc9d1f8598ea4b6a74b15c2f57c875e4cff74148e4628f264b974c80")
);
// Execution Phase
assert!(
whitelist.contains("26aa394eea5630e07c48ae0c9558cef7ff553b5a9862a516939d82b3d3d8661a")
);
// Event Count
assert!(
whitelist.contains("26aa394eea5630e07c48ae0c9558cef70a98fdbe9ce6c55837576c60c7af3850")
);
// System Events
assert!(
whitelist.contains("26aa394eea5630e07c48ae0c9558cef780d41e5e16056765bc8461851072c9d7")
);
}
}
60 changes: 44 additions & 16 deletions bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2142,22 +2142,14 @@ impl_runtime_apis! {
impl baseline::Config for Runtime {}
impl pallet_nomination_pools_benchmarking::Config for Runtime {}

let whitelist: Vec<TrackedStorageKey> = vec![
// Block Number
hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef702a5c1b19ab7a04f536c519aca4983ac").to_vec().into(),
// Total Issuance
hex_literal::hex!("c2261276cc9d1f8598ea4b6a74b15c2f57c875e4cff74148e4628f264b974c80").to_vec().into(),
// Execution Phase
hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef7ff553b5a9862a516939d82b3d3d8661a").to_vec().into(),
// Event Count
hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef70a98fdbe9ce6c55837576c60c7af3850").to_vec().into(),
// System Events
hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef780d41e5e16056765bc8461851072c9d7").to_vec().into(),
// System BlockWeight
hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef734abf5cb34d6244378cddbf18e849d96").to_vec().into(),
// Treasury Account
hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef7b99d880ec681799c0cf30e8886371da95ecffd7b6c0f78751baa9d281e0bfa3a6d6f646c70792f74727372790000000000000000000000000000000000000000").to_vec().into(),
];
use frame_support::traits::WhitelistedStorageKeys;
let mut whitelist: Vec<TrackedStorageKey> = AllPalletsWithSystem::whitelisted_storage_keys();
sam0x17 marked this conversation as resolved.
Show resolved Hide resolved

// Treasury Account
// TODO: this is manual for now, someday we might be able to use a
// macro for this particular key
let treasury_key = frame_system::Account::<Runtime>::hashed_key_for(Treasury::account_id());
whitelist.push(treasury_key.to_vec().into());

let mut batches = Vec::<BenchmarkBatch>::new();
let params = (&config, &whitelist);
Expand All @@ -2171,8 +2163,44 @@ impl_runtime_apis! {
mod tests {
use super::*;
use frame_election_provider_support::NposSolution;
use frame_support::traits::WhitelistedStorageKeys;
use frame_system::offchain::CreateSignedTransaction;
use sp_core::hexdisplay::HexDisplay;
use sp_runtime::UpperOf;
use std::collections::HashSet;

#[test]
fn check_whitelist() {
let whitelist: HashSet<String> = AllPalletsWithSystem::whitelisted_storage_keys()
.iter()
.map(|e| HexDisplay::from(&e.key).to_string())
.collect();

// Block Number
assert!(
whitelist.contains("26aa394eea5630e07c48ae0c9558cef702a5c1b19ab7a04f536c519aca4983ac")
);
// Total Issuance
assert!(
whitelist.contains("c2261276cc9d1f8598ea4b6a74b15c2f57c875e4cff74148e4628f264b974c80")
);
// Execution Phase
assert!(
whitelist.contains("26aa394eea5630e07c48ae0c9558cef7ff553b5a9862a516939d82b3d3d8661a")
);
// Event Count
assert!(
whitelist.contains("26aa394eea5630e07c48ae0c9558cef70a98fdbe9ce6c55837576c60c7af3850")
);
// System Events
assert!(
whitelist.contains("26aa394eea5630e07c48ae0c9558cef780d41e5e16056765bc8461851072c9d7")
);
// System BlockWeight
assert!(
whitelist.contains("26aa394eea5630e07c48ae0c9558cef734abf5cb34d6244378cddbf18e849d96")
);
}

#[test]
fn validate_transaction_submitter_bounds() {
Expand Down
1 change: 1 addition & 0 deletions frame/balances/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,7 @@ pub mod pallet {
/// The total units issued in the system.
#[pallet::storage]
#[pallet::getter(fn total_issuance)]
#[pallet::whitelist_storage]
pub type TotalIssuance<T: Config<I>, I: 'static = ()> = StorageValue<_, T::Balance, ValueQuery>;

/// The Balances pallet example of storing the balance of an account.
Expand Down
19 changes: 19 additions & 0 deletions frame/support/procedural/src/pallet/expand/pallet_struct.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,24 @@ pub fn expand_pallet_struct(def: &mut Def) -> proc_macro2::TokenStream {
quote::quote! { #frame_support::traits::StorageVersion::default() }
};

let whitelisted_storage_idents: Vec<syn::Ident> = def
.storages
.iter()
.filter_map(|s| s.whitelisted.then_some(s.ident.clone()))
.collect();

let whitelisted_storage_keys_impl = quote::quote![
use #frame_support::traits::{StorageInfoTrait, TrackedStorageKey, WhitelistedStorageKeys};
impl<#type_impl_gen> WhitelistedStorageKeys for #pallet_ident<#type_use_gen> #storages_where_clauses {
fn whitelisted_storage_keys() -> #frame_support::sp_std::vec::Vec<TrackedStorageKey> {
use #frame_support::sp_std::vec;
vec![#(
TrackedStorageKey::new(#whitelisted_storage_idents::<#type_use_gen>::hashed_key().to_vec())
),*]
}
}
];

quote::quote_spanned!(def.pallet_struct.attr_span =>
#pallet_error_metadata

Expand Down Expand Up @@ -253,5 +271,6 @@ pub fn expand_pallet_struct(def: &mut Def) -> proc_macro2::TokenStream {
}

#storage_info
#whitelisted_storage_keys_impl
)
}
6 changes: 4 additions & 2 deletions frame/support/procedural/src/pallet/parse/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,10 @@ impl Def {
origin = Some(origin::OriginDef::try_from(index, item)?),
Some(PalletAttr::Inherent(_)) if inherent.is_none() =>
inherent = Some(inherent::InherentDef::try_from(index, item)?),
Some(PalletAttr::Storage(span)) =>
storages.push(storage::StorageDef::try_from(span, index, item)?),
Some(PalletAttr::Storage(span)) => {
let storage_def = storage::StorageDef::try_from(span, index, item)?;
storages.push(storage_def);
},
sam0x17 marked this conversation as resolved.
Show resolved Hide resolved
Some(PalletAttr::ValidateUnsigned(_)) if validate_unsigned.is_none() => {
let v = validate_unsigned::ValidateUnsignedDef::try_from(index, item)?;
validate_unsigned = Some(v);
Expand Down
20 changes: 17 additions & 3 deletions frame/support/procedural/src/pallet/parse/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ mod keyword {
syn::custom_keyword!(getter);
syn::custom_keyword!(storage_prefix);
syn::custom_keyword!(unbounded);
syn::custom_keyword!(whitelist_storage);
syn::custom_keyword!(OptionQuery);
syn::custom_keyword!(ResultQuery);
syn::custom_keyword!(ValueQuery);
Expand All @@ -37,16 +38,21 @@ mod keyword {
/// * `#[pallet::getter(fn dummy)]`
/// * `#[pallet::storage_prefix = "CustomName"]`
/// * `#[pallet::unbounded]`
/// * `#[pallet::whitelist_storage]
pub enum PalletStorageAttr {
Getter(syn::Ident, proc_macro2::Span),
StorageName(syn::LitStr, proc_macro2::Span),
Unbounded(proc_macro2::Span),
WhitelistStorage(proc_macro2::Span),
}

impl PalletStorageAttr {
fn attr_span(&self) -> proc_macro2::Span {
match self {
Self::Getter(_, span) | Self::StorageName(_, span) | Self::Unbounded(span) => *span,
Self::Getter(_, span) |
Self::StorageName(_, span) |
Self::Unbounded(span) |
Self::WhitelistStorage(span) => *span,
}
}
}
Expand Down Expand Up @@ -84,6 +90,9 @@ impl syn::parse::Parse for PalletStorageAttr {
content.parse::<keyword::unbounded>()?;

Ok(Self::Unbounded(attr_span))
} else if lookahead.peek(keyword::whitelist_storage) {
content.parse::<keyword::whitelist_storage>()?;
Ok(Self::WhitelistStorage(attr_span))
} else {
Err(lookahead.error())
}
Expand All @@ -94,19 +103,22 @@ struct PalletStorageAttrInfo {
getter: Option<syn::Ident>,
rename_as: Option<syn::LitStr>,
unbounded: bool,
whitelisted: bool,
}

impl PalletStorageAttrInfo {
fn from_attrs(attrs: Vec<PalletStorageAttr>) -> syn::Result<Self> {
let mut getter = None;
let mut rename_as = None;
let mut unbounded = false;
let mut whitelisted = false;
for attr in attrs {
match attr {
PalletStorageAttr::Getter(ident, ..) if getter.is_none() => getter = Some(ident),
PalletStorageAttr::StorageName(name, ..) if rename_as.is_none() =>
rename_as = Some(name),
PalletStorageAttr::Unbounded(..) if !unbounded => unbounded = true,
PalletStorageAttr::WhitelistStorage(..) if !whitelisted => whitelisted = true,
attr =>
return Err(syn::Error::new(
attr.attr_span(),
Expand All @@ -115,7 +127,7 @@ impl PalletStorageAttrInfo {
}
}

Ok(PalletStorageAttrInfo { getter, rename_as, unbounded })
Ok(PalletStorageAttrInfo { getter, rename_as, unbounded, whitelisted })
}
}

Expand Down Expand Up @@ -171,6 +183,7 @@ pub struct StorageDef {
pub named_generics: Option<StorageGenerics>,
/// If the value stored in this storage is unbounded.
pub unbounded: bool,
pub whitelisted: bool,
sam0x17 marked this conversation as resolved.
Show resolved Hide resolved
}

/// The parsed generic from the
Expand Down Expand Up @@ -672,7 +685,7 @@ impl StorageDef {
};

let attrs: Vec<PalletStorageAttr> = helper::take_item_pallet_attrs(&mut item.attrs)?;
let PalletStorageAttrInfo { getter, rename_as, unbounded } =
let PalletStorageAttrInfo { getter, rename_as, unbounded, whitelisted } =
PalletStorageAttrInfo::from_attrs(attrs)?;

let cfg_attrs = helper::get_item_cfg_attrs(&item.attrs);
Expand Down Expand Up @@ -814,6 +827,7 @@ impl StorageDef {
cfg_attrs,
named_generics,
unbounded,
whitelisted,
})
}
}
1 change: 1 addition & 0 deletions frame/support/procedural/src/storage/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ pub(crate) use instance_trait::INHERENT_INSTANCE_NAME;
use frame_support_procedural_tools::{
generate_crate_access, generate_hidden_includes, syn_ext as ext,
};

use quote::quote;

/// All information contained in input of decl_storage
Expand Down
16 changes: 16 additions & 0 deletions frame/support/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1855,6 +1855,22 @@ pub mod pallet_prelude {
/// pub(super) type MyStorage<T> = StorageValue<Value = u32>;
/// ```
///
/// The optional attribute `#[pallet::whitelist_storage]` will declare the
/// storage as whitelisted from benchmarking. Doing so will exclude reads of
/// that value's storage key from counting towards weight calculations during
/// benchmarking.
///
/// This attribute should only be attached to storages that are known to be
/// read/used in every block. This will result in a more efficient read pattern
/// for the value and a more accurate benchmarking weight.
sam0x17 marked this conversation as resolved.
Show resolved Hide resolved
///
/// ### Example
/// ```ignore
/// #[pallet::storage]
/// #[pallet::whitelist_storage]
/// pub(super) type Number<T: Config> = StorageValue<_, T::BlockNumber, ValueQuery>;
/// ```
///
/// All the `cfg` attributes are automatically copied to the items generated for the storage,
/// i.e. the getter, storage prefix, and the metadata element etc.
///
Expand Down
1 change: 1 addition & 0 deletions frame/support/src/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ pub mod schedule;
mod storage;
pub use storage::{
Instance, PartialStorageInfoTrait, StorageInfo, StorageInfoTrait, StorageInstance,
TrackedStorageKey, WhitelistedStorageKeys,
};

mod dispatch;
Expand Down
28 changes: 28 additions & 0 deletions frame/support/src/traits/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@

//! Traits for encoding data related to pallet's storage items.

use crate::sp_std::collections::btree_set::BTreeSet;
use impl_trait_for_tuples::impl_for_tuples;
pub use sp_core::storage::TrackedStorageKey;
use sp_std::prelude::*;

/// An instance of a pallet in the storage.
Expand Down Expand Up @@ -90,3 +92,29 @@ impl StorageInfoTrait for Tuple {
pub trait PartialStorageInfoTrait {
fn partial_storage_info() -> Vec<StorageInfo>;
}

/// Allows a pallet to specify storage keys to whitelist during benchmarking.
/// This means those keys will be excluded from the benchmarking performance
/// calculation.
pub trait WhitelistedStorageKeys {
/// Returns a [`Vec<TrackedStorageKey>`] indicating the storage keys that
/// should be whitelisted during benchmarking. This means that those keys
/// will be excluded from the benchmarking performance calculation.
fn whitelisted_storage_keys() -> Vec<TrackedStorageKey>;
}

#[cfg_attr(all(not(feature = "tuples-96"), not(feature = "tuples-128")), impl_for_tuples(64))]
#[cfg_attr(all(feature = "tuples-96", not(feature = "tuples-128")), impl_for_tuples(96))]
#[cfg_attr(feature = "tuples-128", impl_for_tuples(128))]
impl WhitelistedStorageKeys for Tuple {
fn whitelisted_storage_keys() -> Vec<TrackedStorageKey> {
// de-duplicate the storage keys
let mut combined_keys: BTreeSet<TrackedStorageKey> = BTreeSet::new();
for_tuples!( #(
for storage_key in Tuple::whitelisted_storage_keys() {
combined_keys.insert(storage_key);
}
)* );
combined_keys.iter().cloned().collect::<Vec<_>>()
sam0x17 marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error: expected one of: `getter`, `storage_prefix`, `unbounded`
error: expected one of: `getter`, `storage_prefix`, `unbounded`, `whitelist_storage`
--> $DIR/storage_invalid_attribute.rs:16:12
|
16 | #[pallet::generate_store(pub trait Store)]
Expand Down
Loading