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

More benchmark machine args #11428

Merged
merged 4 commits into from
May 17, 2022
Merged

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented May 16, 2022

I ran the benchmark machine command some dozen times on the ref hardware and the Blake2-256 seems to be the only failing one. It runs for only 100ms which is probably too short to get reproducible results.

Changes:

  • Increase benchmark durations
  • Add arguments to control all durations
  • Let sc-sysinfo return f64 for more precision
  • Add explicit default limits to sc-sysinfo

The benchmark worked 27 times in a row, then failed with 0.4% outside the tolerance for the disk benchmark. Should be good enough for now.

ggwpez added 3 commits May 16, 2022 14:32
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label May 16, 2022
@ggwpez ggwpez marked this pull request as ready for review May 16, 2022 13:21
@ggwpez ggwpez requested a review from koute May 16, 2022 13:21
@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 May 16, 2022
@ggwpez ggwpez added 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. A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes and removed A0-please_review Pull request needs code review. B5-clientnoteworthy labels May 16, 2022
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@shawntabrizi shawntabrizi changed the title More benchmark manchine args More benchmark machine args May 16, 2022
@koute
Copy link
Contributor

koute commented May 17, 2022

So what's the probability of it failing with only 100ms time? How does it change if you'd increase it to 200ms? (I'm thinking that we might consider increasing the benchmark time slightly for telemetry too, although when I was testing it out on the benchmarking machine it seemed stable enough.)

@ggwpez
Copy link
Member Author

ggwpez commented May 17, 2022

@koute yea good idea.
So with 100ms it often fails with down to just 73%. With 200ms the lowest that I am seeing is 85%. With 400ms it only dips to 92% and passes fine.
Not sure if the benchmarks influence each other or what is going on, but sometimes I can consistently reproduce these numbers and when I try it the next time; not at all.
Maybe we need some dedicated test for this, but for now I dont have the time.

@ggwpez
Copy link
Member Author

ggwpez commented May 17, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit c44cd63 into master May 17, 2022
@paritytech-processbot paritytech-processbot bot deleted the oty-benchmark-manchine-args branch May 17, 2022 13:57
godcodehunter pushed a commit to sensoriumxr/substrate that referenced this pull request Jun 22, 2022
* Add ExecutionLimits to sc-sysinfo and return float

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

* Increase benchmarking duration and add options

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

* Fix tests

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

* Fix tests

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
* Add ExecutionLimits to sc-sysinfo and return float

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

* Increase benchmarking duration and add options

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

* Fix tests

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

* Fix tests

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Add ExecutionLimits to sc-sysinfo and return float

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

* Increase benchmarking duration and add options

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

* Fix tests

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

* Fix tests

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. 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
Development

Successfully merging this pull request may close these issues.

3 participants