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

Add benchmark extrinsic command #11456

Merged
merged 27 commits into from
Jul 19, 2022
Merged

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented May 18, 2022

Makes it possible to benchmark the execution speed of an extrinsic. It does so by filling a block with these extrinsics and measures its execution time.
The results is then divided by the number of extrinsics and printed to console.

Example invocation:

$ substrate benchmark extrinsic --pallet system --extrinsic remark --dev

Creating empty BABE epoch changes on what appears to be first startup.    
Building block, this takes some time...    
Extrinsics per block: 12000    
Running 10 warmups...    
Executing block 100 times    
Executing a SYSTEM::REMARK extrinsic takes[ns]:
Total: 6897382
Min: 68685, Max: 69797
Average: 68973, Median: 68899, Stddev: 188.99
Percentiles 99th, 95th, 75th: 69293, 69230, 69122

Changes:

  • Add a benchmark extrinsic <pallet> <extrinsic> command
  • Implement the command for System::Remark and Balances::TransferKeepAlive
  • Modify benchmark overhead to re-use the logic
  • Update node+template to integrate the command

WIP:

  • Document that it is non-endowing, non-reaping and currently always the same sender and receiver account
  • Print all available pallets and extrinsics in the help menu
  • Do some more lenient input transformation (eg. lower-case, trim - and _)

Polkadot companion: paritytech/polkadot#5620
Cumulus companion: paritytech/cumulus#1350

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Running this 1000 times with a full block takes ~33 minutes 🙈.
Reducing it to ~3 minutes per default.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@github-actions github-actions bot added the A0-please_review Pull request needs code review. label May 18, 2022
@ggwpez ggwpez added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels May 18, 2022
@ggwpez ggwpez added B5-clientnoteworthy C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jun 1, 2022
@ggwpez ggwpez added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jun 1, 2022
Copy link
Contributor

@gilescope gilescope left a comment

Choose a reason for hiding this comment

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

Nice addition. Looking forward to trying it out.

bin/node-template/node/src/benchmarking.rs Show resolved Hide resolved
bin/node-template/runtime/src/lib.rs Show resolved Hide resolved
utils/frame/benchmarking-cli/src/extrinsic/bench.rs Outdated Show resolved Hide resolved
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
The .to_lowercase() on the builder is actually not needes
since its already documented to only return lower case.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
let record = self.measure_block(&block, num_ext, bench_type)?;
pub fn bench(&self, ext_builder: Option<&dyn ExtrinsicBuilder>) -> Result<Stats> {
let (block, num_ext) = self.build_block(ext_builder)?;
let record = self.measure_block(&block, num_ext)?;
Copy link
Member

Choose a reason for hiding this comment

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

You don't remove the block execution time of an empty block. Meaning you don't really measure the extrinsic execution time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes this is a simplification in the code. An empty block executes in about 5.3ms, which is only 0.265% of a 2s block and therefore negligible.
I could still change it to measure an empty block first, do you think its worth it?

Copy link
Member

Choose a reason for hiding this comment

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

How do you know for every runtime what it takes to execute an empty block? :P

Yes, please do this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated it to first measure an empty block and then subtract the average of that from the full block to account for the inherent timings and execution overhead. @bkchr

@ggwpez ggwpez requested a review from bkchr July 15, 2022 07:28
This should already be the case since --dev is passed but
somehow not...

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
Copy link
Member Author

ggwpez commented Jul 19, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit ced4169 into master Jul 19, 2022
@paritytech-processbot paritytech-processbot bot deleted the oty-bench-extrinsic branch July 19, 2022 06:10
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
* Benchmark extrinsic

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Reduce warmup and repeat

Running this 1000 times with a full block takes ~33 minutes 🙈.
Reducing it to ~3 minutes per default.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Make ExistentialDeposit public

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Add 'bechmark extrinsic' command

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* fmt

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Add --list and cleanup

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Unrelated Clippy

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Clippy

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fix tests and doc

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Move implementations up + fmt

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Dont use parameter_types macro

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Cache to_lowercase() call

The .to_lowercase() on the builder is actually not needes
since its already documented to only return lower case.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Spelling

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Use correct nightly for fmt...

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Rename ED

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fix compile

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Subtract block base weight

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fixes

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Use tmp folder for test

This should already be the case since --dev is passed but
somehow not...

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fix test

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Benchmark extrinsic

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Reduce warmup and repeat

Running this 1000 times with a full block takes ~33 minutes 🙈.
Reducing it to ~3 minutes per default.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Make ExistentialDeposit public

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Add 'bechmark extrinsic' command

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* fmt

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Add --list and cleanup

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Unrelated Clippy

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Clippy

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fix tests and doc

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Move implementations up + fmt

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Dont use parameter_types macro

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Cache to_lowercase() call

The .to_lowercase() on the builder is actually not needes
since its already documented to only return lower case.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Spelling

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Use correct nightly for fmt...

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Rename ED

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fix compile

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Subtract block base weight

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fixes

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Use tmp folder for test

This should already be the case since --dev is passed but
somehow not...

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fix test

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
Development

Successfully merging this pull request may close these issues.

4 participants