rmrk-substrate icon indicating copy to clipboard operation
rmrk-substrate copied to clipboard

Benchmarking RMRK-core

Open Maar-io opened this issue 3 years ago • 1 comments

This PR introduces benchmark test for pallet-rmrk-core

Note!!! Each parachain need to re-run benchmark tests on their referent HW

The major change in the most of the files is that following construct could not be used in the benchmark! macro

where
	T: pallet_uniques::Config<CollectionId = CollectionId, ItemId = NftId>,
  • The pallet-uniques associated type CollectionId does not support From<u32> and it is therefore used as T::CollectionId replacing the use of locally defined CollectionId.
  • The pallet-uniques associated type ItemId does not support From<u32> and it is therefore used as T::ItemId replacing the use of locally defined NftId.
  • removed default() from ExtBuilder::default() in order to accommodate benchmark unit tests.

Another big change is the removal of CollectionIndex storage item and allowing caller to give the collection_id as an argument. It is not likely that rmrk pallets will be sole user of the uniques ids. Second reason for removing CollectionIndex is due to change of collection_id type from u32 to Unique's associated type T::CollectionId, After this change try_mutate will not work over CollectionIndex

  • removed storage item CollectionIndex
  • removed rpc call last_collection_idx() see above
  • [x] unittest updated and passing
  • [x] benchmark tests work on runtime and on MockRuntime
  • [x] runtime lib updated with Weights

Next steps

  • benchmark tests update to calculate iteration steps in case of nesting.
  • benchmark test for rmrk-markets
  • benchmark test for rmrk-equip

Maar-io avatar Sep 17 '22 15:09 Maar-io

The Weights are now a struct with a u64 field named ref_time. I think some of the build errors can be fixed by using the Weights::from_ref_time(u64). Here is the file https://github.com/paritytech/substrate/blob/c6ebc1e73f85deba349db0ea785ad53addb6f69f/frame/support/src/weights/weight_v2.rs#L45

HashWarlock avatar Sep 30 '22 17:09 HashWarlock

Need to remove the commented out code. I'll run this in my environment. Do the integration tests pass, as well?

Another thought is on downstream implementations. These chains will need to be notified so they do the appropriate storage migrations for the changes to Storage.

Other than these items, it LGTM. Nice work Maario

HashWarlock avatar Oct 29 '22 21:10 HashWarlock