Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Benchmarking RMRK-core #219

Merged
merged 5 commits into from
Nov 6, 2022

Conversation

Maar-io
Copy link
Contributor

@Maar-io Maar-io commented Sep 17, 2022

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
  • unittest updated and passing
  • benchmark tests work on runtime and on MockRuntime
  • 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 Maar-io force-pushed the feature/pallet-benchmarks branch from aac5225 to 8fb369e Compare September 23, 2022 09:47
@Maar-io Maar-io marked this pull request as ready for review September 23, 2022 12:53
@Maar-io Maar-io requested a review from HashWarlock September 26, 2022 07:43
@Maar-io Maar-io force-pushed the feature/pallet-benchmarks branch from f7e026e to 200cd5d Compare September 30, 2022 11:52
@HashWarlock
Copy link
Contributor

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

@Maar-io Maar-io closed this Oct 22, 2022
@Maar-io Maar-io force-pushed the feature/pallet-benchmarks branch from 5a8b33b to 7abb495 Compare October 22, 2022 08:50
@Maar-io Maar-io reopened this Oct 22, 2022
@Maar-io Maar-io marked this pull request as draft October 24, 2022 08:23
@Maar-io Maar-io marked this pull request as ready for review October 24, 2022 15:31
@Maar-io Maar-io force-pushed the feature/pallet-benchmarks branch from 0185f7c to 14563ad Compare October 29, 2022 08:32
rpc/src/lib.rs Outdated Show resolved Hide resolved
rpc/src/lib.rs Outdated Show resolved Hide resolved
runtime/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@HashWarlock HashWarlock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@HashWarlock
Copy link
Contributor

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

@Maar-io Maar-io force-pushed the feature/pallet-benchmarks branch from 87bfba7 to 9c68ab9 Compare November 5, 2022 14:18
Mar.io and others added 2 commits November 5, 2022 16:16
rebase typo

merge conflict resolved

use T::CollectionId instead of u32

create collection with the given id

updates to traits

update market and equip

update benchmark tests

generated and used benchmaked weights

fixing tests

cleanup before review

more cleanup
removing commented-out code
@Maar-io Maar-io force-pushed the feature/pallet-benchmarks branch from 9c68ab9 to 9411414 Compare November 5, 2022 15:19
Copy link
Contributor

@ilionic ilionic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you.
Was able to re-generate weights and approach is more clear now.
Will proceed with the merge and create separate PR for extra calls.

@ilionic ilionic merged commit 46ab58f into rmrk-team:main Nov 6, 2022
@ilionic ilionic deleted the feature/pallet-benchmarks branch November 6, 2022 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants