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

Benchmarking pallet-example #8301

Merged
merged 22 commits into from
Mar 29, 2021
Merged

Benchmarking pallet-example #8301

merged 22 commits into from
Mar 29, 2021

Conversation

jimmychu0807
Copy link
Contributor

  1. Split tests and benchmarking out in pallet-example
  2. Added weights.rs in pallet-example, so it can be referenced in documentation / substrate devhub.

Before you submitting, please check that:

  • You added a brief description of the PR, e.g.:
    • What does it do?
    • What important points reviewers should know?
    • Is there something left for follow-up PRs?
  • You labeled the PR appropriately if you have permissions to do so:
    • A* for PR status (one required)
    • B* for changelog (one required)
    • C* for release notes (exactly one required)
    • D* for various implications/requirements
    • Github's project assignment
  • You mentioned a related issue if this PR related to it, e.g. Fixes #228 or Related #1337.
  • You asked any particular reviewers to review. If you aren't sure, start with GH suggestions.
  • Your PR adheres to the style guide
    • In particular, mind the maximal line length of 100 (120 in exceptional circumstances).
    • There is no commented code checked in unless necessary.
    • Any panickers have a proof or removed.
  • You bumped the runtime version if there are breaking changes in the runtime.
  • You updated any rustdocs which may have changed
  • Has the PR altered the external API or interfaces used by Polkadot? Do you have the corresponding Polkadot PR ready?

Refer to the contributing guide for details.

After you've read this notice feel free to remove it.
Thank you!

✄ -----------------------------------------------------------------------------

@jimmychu0807
Trying to benchmark pallet-example
34853d2 
Signed-off-by: Jimmy Chu <jimmychu0807@gmail.com>
@jimmychu0807
added the weights info in function
19ff8c4 
Signed-off-by: Jimmy Chu <jimmychu0807@gmail.com>
@jimmychu0807
updated naming
6e5902a 
Signed-off-by: Jimmy Chu <jimmychu0807@gmail.com>
@jimmychu0807
updated with benchmarking and weight
c4c0376 
Signed-off-by: Jimmy Chu <jimmychu0807@gmail.com>
@jimmychu0807
Merge branch 'master' into jc/example-benchmarking
86a3333 
Signed-off-by: Jimmy Chu <jimmychu0807@gmail.com>

# Conflicts:
#	frame/example/src/lib.rs
@jimmychu0807
Updated test for pallet-example
2a3ea00 
Signed-off-by: Jimmy Chu <jimmychu0807@gmail.com>
@jimmychu0807
adding weights.rs benchmarked in release mode
3d68f86 
Signed-off-by: Jimmy Chu <jimmychu0807@gmail.com>
@jimmychu0807
Removed pallet-example from node-runtime
c436b14 
Signed-off-by: Jimmy Chu <jimmychu0807@gmail.com>
@jimmychu0807
adjusted
3ab9102 
Signed-off-by: Jimmy Chu <jimmychu0807@gmail.com>
@jimmychu0807 jimmychu0807 added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Mar 9, 2021
@jimmychu0807 jimmychu0807 requested a review from kianenigma as a code owner March 9, 2021 10:57
@jimmychu0807
modification based on peer comments
8d2cdf9 
Signed-off-by: Jimmy Chu <jimmychu0807@gmail.com>
@jimmychu0807
updated so far
abd4507 
Signed-off-by: Jimmy Chu <jimmychu0807@gmail.com>
@jimmychu0807
Merge branch 'master' into jc/example-benchmarking
ef98f05 
Signed-off-by: Jimmy Chu <jimmychu0807@gmail.com>

# Conflicts:
#	bin/node/runtime/src/lib.rs
#	frame/example/src/lib.rs
@jimmychu0807
updated
7614873 
Signed-off-by: Jimmy Chu <jimmychu0807@gmail.com>
@jimmychu0807
updated
911fdcc 
Signed-off-by: Jimmy Chu <jimmychu0807@gmail.com>
gui1117
gui1117 reviewed Mar 23, 2021
View reviewed changes
frame/example/src/benchmarking.rs Show resolved Hide resolved
frame/example/src/lib.rs
@@ -496,7 +506,9 @@ pub mod pallet {
// calls to be executed - we don't need to care why. Because it's privileged, we can
// assume it's a one-off operation and substantial processing/storage/memory can be used
// without worrying about gameability or attack scenarios.
#[pallet::weight(WeightForSetDummy::<T>(<BalanceOf<T>>::from(100u32)))]
Copy link
Contributor

Choose a reason for hiding this comment

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

the documentation for WeightForSetDummy is outdated now.

Maybe we can just remove it as we prefer to just show benchmarking way, not sure. cc @kianenigma

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am thinking it is a good way to showcasing how to set the weight: 1) thru benchmarking, 2) manually setting it via WeightForSetDummy, and it shows setting an extrinsic call as operational call.

I think it will be great showing runtime developers alternatives that they can override the measured benchmarking and tweak the weight function as they want. Will there be some scenarios that this is necessary for dev?

Copy link
Contributor

Choose a reason for hiding this comment

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

ideally what people need to understand is that pallet::weight take into argument any type which implements:

  • WeighData<(Arg1, Arg2, ..)>
  • ClassifyDispatch<(Arg1, Arg2, ..)>
  • PaysFee<(Arg1, Arg2, ..)>

where Arg1, Arg2, ..., are the type of the argument of the call.

Make we can keep WeightForSetDummy for this call.

I think if we keep WeightForSetDummy in the code and never use it, people can't understand how to use it.

Copy link
Contributor

@kianenigma kianenigma Mar 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

oh boy I totally have forgotten about these manual weights.

I think they can stay, only if we mention that this is an advanced feature if you want some custom functionality. Most often, you want to use WeightInfo stuff only.

If too much for now, @jimmychu0807 you can remove it in this PR, make a new Issue in the devhub for custom weight calculators and ping me there, I can help with that.

Copy link
Contributor Author

@jimmychu0807 jimmychu0807 Mar 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@kianenigma We kind of have that in devhub here, though we should probably move it to the Transaction Weight document.

And the example implementation of WeightForSetDummy demonstrates what you could customize.

Added additional comment in code for readers to consider running benchmarking toolchain instead of manually setting weight unless they know what they are doing.

frame/example/src/weights.rs Show resolved Hide resolved
@jimmychu0807
updated code
a291e13 
Signed-off-by: Jimmy Chu <jimmychu0807@gmail.com>
@jimmychu0807
Added explanation on impl_benchmark_test_suite! macro
612cde9 
Signed-off-by: Jimmy Chu <jimmychu0807@gmail.com>
@jimmychu0807
Remove pallet-example from runtime
f4ff4f7 
Signed-off-by: Jimmy Chu <jimmychu0807@gmail.com>
@jimmychu0807
Merge remote-tracking branch 'gh-parity/master' into jc/example-bench…
8c66979 
…marking
@jimmychu0807
rm pallet-example
a103aca 
Signed-off-by: Jimmy Chu <jimmychu0807@gmail.com>
jimmychu0807
jimmychu0807 commented Mar 25, 2021
View reviewed changes
frame/example/src/lib.rs
@@ -292,7 +310,9 @@ impl<T: pallet_balances::Config> WeighData<(&BalanceOf<T>,)> for WeightForSetDum
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

re: #8301 (comment)

In here, we start implementing the WeightData, ClassifyDispatch, and PaysFee. In each implementations, we do use the paramter (&BalanceOf<T>). Is that what you mean to "use it" in your comment?

@jimmychu0807
Copy link
Contributor Author

@kianenigma @shawntabrizi @thiolliere I did a clean up on the pallet code, comment, and benchmarking code in pallet-example. See if this PR is okay to merge.

btw, how do I fix the CI error of error: the lock file /builds/parity/substrate/Cargo.lock needs to be updated but --locked was passed to prevent this?

kianenigma
kianenigma reviewed Mar 29, 2021
View reviewed changes
frame/example/src/lib.rs
/// The overarching event type.
type Event: From<Event<Self>> + IsType<<Self as frame_system::Config>::Event>;
type WeightInfo: WeightInfo;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
type WeightInfo: WeightInfo;
/// Somethign representing the weights of this pallet.
type WeightInfo: WeightInfo;

kianenigma
kianenigma approved these changes Mar 29, 2021
View reviewed changes
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

LGTM

@kianenigma
Update frame/example/src/benchmarking.rs
f28e430 
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
@jimmychu0807 jimmychu0807 added the D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. label Mar 29, 2021
@jimmychu0807 jimmychu0807 changed the title Trying to benchmark pallet-example Benchmarking pallet-example Mar 29, 2021
@jimmychu0807 jimmychu0807 merged commit 2125b50 into master Mar 29, 2021
@jimmychu0807 jimmychu0807 deleted the jc/try-benchmarking branch March 29, 2021 13:33
@jimmychu0807
Copy link
Contributor Author

thanks @kianenigma @thiolliere @shawntabrizi for the review!

ordian added a commit that referenced this pull request Mar 31, 2021
@ordian
Merge branch 'master' into ao-update-wasmtime
ded49c0 
* master: (84 commits)
  Duplicate logging to stdout (#8495)
  Fix sync restart (#8497)
  client: fix justifications migration (#8489)
  helper macro to create storage types on the fly (#8456)
  Make `BlockImport` and `Verifier` async (#8472)
  Get rid of `test-helpers` feature in sc-consensus-babe (#8486)
  Enhancement on Substrate Node Template (#8473)
  Add Social Network (#8065)
  Prepare UI tests for Rust 1.51 & new CI image (#8474)
  Benchmarking pallet-example (#8301)
  Use pathbuf for remote externalities (#8480)
  Bring back the on_finalize weight of staking. (#8463)
  Implement `fungible::*` for Balances (#8454)
  make types within `generate_solution_type` macro explicit (#8447)
  [pallet-staking] Refund unused weight for `payout_stakers` (#8458)
  Use `async_trait` in sc-consensus-slots (#8461)
  Repot frame_support::traits; introduce some new currency stuff (#8435)
  Fix &mut self -> &self in add_known_address (#8468)
  Add NetworkService::add_known_address (#8467)
  Fix companion check (#8464)
  ...
hirschenberger pushed a commit to hirschenberger/substrate that referenced this pull request Apr 14, 2021
@kianenigma @hirschenberger
Benchmarking pallet-example (paritytech#8301)
ba11a5f 
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@shawntabrizi shawntabrizi shawntabrizi left review comments

@gui1117 gui1117 gui1117 approved these changes

@kianenigma kianenigma kianenigma approved these changes

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jimmychu0807 @shawntabrizi @kianenigma @gui1117 @paritytech-ci