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

Add benchmark-block command #11091

Merged
merged 6 commits into from
Mar 25, 2022
Merged

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Mar 22, 2022

Deja vu! The code that I originally wanted to use for #10977 is now put to better use.

This command helps in gauging the precision or our benchmarking.
It compares the time that it takes to execute a block to its consumed weight. Ideally they are the same.

Example with a CPU intensive hand-crafted block where our benchmarking worked well:

Block 2 with     2 tx used 102.39% of its weight ( 1,281,193,264 of 1,251,309,762  ns) - OVER WEIGHT!

Or some empty blocks where we over-estimate:

Block 1 with     1 tx used  65.03% of its weight (     4,464,209 of      6,864,645 ns)    
Block 2 with     1 tx used  65.15% of its weight (     4,472,344 of      6,864,645 ns)    
Block 3 with     1 tx used  64.27% of its weight (     4,492,493 of      6,989,645 ns)

There are rust docs on the BlockCmd type with an example on how to invoke it.
In the future this could be used to setup some live monitoring where each day we re-execute the blocks of the past 24h to see which ones are over weight (bad).

Follow ups moved here: paritytech/polkadot-sdk#391

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez added A0-please_review Pull request needs code review. B5-clientnoteworthy 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. labels Mar 22, 2022
@ggwpez ggwpez self-assigned this Mar 22, 2022
@ggwpez ggwpez marked this pull request as ready for review March 22, 2022 16:16
@ggwpez ggwpez requested a review from shawntabrizi March 22, 2022 16:28
Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

this is way better than i hoped!

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, but a minor existential question I have is that why is this not part of the same check-block command that we have that is capable of re-importing old blocks? if you run with -lruntime=debug, you already get the consumed weight that was recorded onchain, and you can time and compare it with the check-block command again.

I can see some benefit in having a custom command be easier to deal with and extend, but was the alternative considered?

@ggwpez
Copy link
Member Author

ggwpez commented Mar 23, 2022

Very good question! I initially had a look at that code, but did not think about extending it.

Analyzing again; check-block uses the import queue, and that has a fast path, just returning with Block already in chain.
Visible with -lsync=trace:

substrate --dev -d /tmp/dev
substrate check-block 1 --dev -d /tmp/dev --pruning archive -lsync=trace

# prints
Scheduling 1 blocks for import    
Starting import of 1 blocks  (1)    
Header 0x7e5d…ca03 has 3 logs    
Block already in chain 1: 0x7e5d3a...
Block imported successfully Some(1) (0x7e5d…ca03) 

So it does not actually re-execute the block, but returns quickly since the block is already known-good.

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
ggwpez added 2 commits March 24, 2022 11:31
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Copy link
Member Author

ggwpez commented Mar 24, 2022

bot rebase

@paritytech-processbot
Copy link

Rebasing

@ggwpez
Copy link
Member Author

ggwpez commented Mar 25, 2022

bot rebase

@paritytech-processbot
Copy link

Rebasing

@ggwpez
Copy link
Member Author

ggwpez commented Mar 25, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 08fd14d into master Mar 25, 2022
@paritytech-processbot paritytech-processbot bot deleted the oty-find-overestimate branch March 25, 2022 11:27
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
* Add benchmark-block command

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Apply suggestions from code review

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

* Beauty fixes

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Beauty fixes

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: parity-processbot <>
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
* Add benchmark-block command

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Apply suggestions from code review

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

* Beauty fixes

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Beauty fixes

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: parity-processbot <>
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
* Add benchmark-block command

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Apply suggestions from code review

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

* Beauty fixes

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Beauty fixes

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: parity-processbot <>
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
* Add benchmark-block command

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Apply suggestions from code review

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

* Beauty fixes

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Beauty fixes

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: parity-processbot <>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Add benchmark-block command

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Apply suggestions from code review

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

* Beauty fixes

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Beauty fixes

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: parity-processbot <>
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. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
Development

Successfully merging this pull request may close these issues.

4 participants