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

Add benchmark machine #11198

Merged
merged 28 commits into from Apr 15, 2022
Merged

Add benchmark machine #11198

merged 28 commits into from Apr 15, 2022

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Apr 11, 2022

👉 Please put feedback about this command either in here or ping me on matrix @oliver.tale-yazdi:matrix.parity.io.

First version of a benchmark machine command using the sc-sysinfo crate (:pray: @koute).
@shawntabrizi wanted to have a MVP version handy.

Changes:

  • Add benchmark machine command
  • Integrate it into the substrate node and node-template
  • Move new_rng to shared
  • Add --no-hardware-benchmarks where applicable

Output:

$ substrate benchmark machine --dev

+----------+----------------+-------+------+
| Category | Function       | Score | Unit |
+----------+----------------+-------+------+
| CPU      | BLAKE2-256     | 1029  | MB/s |
+----------+----------------+-------+------+
| CPU      | SR25519 Verify | 666.6 | KB/s |
+----------+----------------+-------+------+
| Memory   | Copy           | 14668 | MB/s |
+----------+----------------+-------+------+
| Disk     | Seq Write      | 502   | MB/s |
+----------+----------------+-------+------+
| Disk     | Rnd Write      | 227   | MB/s |
+----------+----------------+-------+------+

Next steps:

  • Add check that the build profile was production
  • Extract more info from sc-sysinfo (eg. total time, reps, input len)
  • Pass in thresholds such that every chain can have theirs own requirements
  • Print results relative to thresholds
  • Measure on ref-hardware to get polkadot thresholds

Polkadot companion: paritytech/polkadot#5326
Cumulus companion: paritytech/cumulus#1172

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez added A3-in_progress Pull request is in progress. No review needed at this stage. A7-needspolkadotpr 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 Apr 11, 2022
@github-actions github-actions bot 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 Apr 11, 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 Apr 11, 2022
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
utils/frame/benchmarking-cli/src/machine/mod.rs Outdated Show resolved Hide resolved
utils/frame/benchmarking-cli/src/machine/mod.rs Outdated Show resolved Hide resolved
utils/frame/benchmarking-cli/src/machine/mod.rs Outdated Show resolved Hide resolved
utils/frame/benchmarking-cli/src/machine/mod.rs Outdated Show resolved Hide resolved
pub fn run(&self) -> Result<()> {
info!("Running machine benchmarks...");
let tmp_dir = tempdir()?;
let info = gather_hwbench(Some(tmp_dir.path()));
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd probably be better to modify sc-sysinfo to expose the benchmark_* functions separately and call them one by one here instead of relying the gather_hwbench.

The gather_hwbench is deliberately simple (only takes a path as an argument) and ignores error (if the disk benches fail it'll set their results to None and only print out a warning in the logs) since it's meant to be run at startup to gather the data for telemetry.

Here however it'd be good to 1) have direct access to the errors (which the benchmark_* functions do return), and 2) be able to configure the benchmarks to be able to override the limit of how long they can run and for how many iterations since this subcommand doesn't have the same timing constraints as the telemetry (so we can just add an extra argument to the benchmark_* functions which would allow you to override that).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea getting some more configurations in there would be good.

utils/frame/benchmarking-cli/src/shared/mod.rs Outdated Show resolved Hide resolved
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@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 Apr 13, 2022
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
client/sysinfo/src/sysinfo.rs Outdated Show resolved Hide resolved
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@gilescope
Copy link
Contributor

In the spirit of pits of success, maybe add an extra line in the output if they're running in debug mode warning them that they should only trust the results if they are running in release mode?

.map(|s| s as u64)
}

pub fn benchmark_sr25519_verify(limit: ExecutionLimit) -> f64 {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: missing doc on public function. Does it need to be public?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will add 😄
Currently it needs to be public since I call these functions directly. IMHO it would be better to have a gather function which calls all of them and returns a struct,
but thats probably next.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO it would be better to have a gather function which calls all of them and returns a struct,

Hmm... although the downside of that is that you won't be able to configure them separately (set a different ExecutionLimit for each benchmark), unless you add another struct which will have multiple ExecutionLimits in it, but then if you're going to do that you might as well have them as a separate functions I guess?

(And it might make sense to set different limits on each benchmark because e.g. the disk benchmarks are going to require more time than the memory benchmark to make sure their result isn't noisy, if you absolutely want to minimize their error.)

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!

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

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 1e0807f into master Apr 15, 2022
@paritytech-processbot paritytech-processbot bot deleted the oty-benchmark-machine branch April 15, 2022 12:09
@ggwpez ggwpez changed the title Add benchmark machine placeholder Add benchmark machine Apr 19, 2022
@xlc xlc mentioned this pull request Aug 7, 2022
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
* Move new_rng to shared code

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

* Add bechmark machine command

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

* Use sc-sysinfo

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

* Add --no-hardware-benchmarks

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

* Lockfile

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

* Do not create components if not needed

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

* Fix tests

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

* Revert "Add --no-hardware-benchmarks"

This reverts commit d4ee982.

* Fix tests

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

* Update Cargo deps

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

* Move sr255119::verify bench to sc-sysinfo

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

* Move sr255119::verify bench to sc-sysinfo

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

* Switch benchmarks to return f64

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

* Review fixes

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

* fmt

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

* Hide command until completed

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

* Use concrete rand implementation

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

* Put clobber into a function

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

* Add test

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

* Add comment

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

* Update cargo to match polkadot

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

* Remove doc that does not format in the console

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

* Limit benchmark by time

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

* Add ExecutionLimit and make function infallible

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

* CI

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

* Add doc

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Move new_rng to shared code

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

* Add bechmark machine command

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

* Use sc-sysinfo

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

* Add --no-hardware-benchmarks

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

* Lockfile

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

* Do not create components if not needed

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

* Fix tests

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

* Revert "Add --no-hardware-benchmarks"

This reverts commit d4ee982.

* Fix tests

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

* Update Cargo deps

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

* Move sr255119::verify bench to sc-sysinfo

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

* Move sr255119::verify bench to sc-sysinfo

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

* Switch benchmarks to return f64

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

* Review fixes

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

* fmt

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

* Hide command until completed

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

* Use concrete rand implementation

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

* Put clobber into a function

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

* Add test

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

* Add comment

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

* Update cargo to match polkadot

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

* Remove doc that does not format in the console

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

* Limit benchmark by time

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

* Add ExecutionLimit and make function infallible

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

* CI

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

* Add doc

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants