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

contracts: Simplify benchmarks #13595

Merged
merged 11 commits into from
Mar 16, 2023
Merged

contracts: Simplify benchmarks #13595

merged 11 commits into from
Mar 16, 2023

Conversation

athei
Copy link
Member

@athei athei commented Mar 14, 2023

Inspired by #13581 I reworked the contract benchmarks to be much simpler and hopefully work better with the linear regression algorithm:

  • Remove batching: Every r component is now exactly one iteration. *_per_byte benchmarks (which don't have r) just perform their operation once. I increased the upper limit for r to compensate for the removed batching.
  • Switch to bytes as unit for components (from kilobytes).

I implemented those strategies when writing those benchmarks years ago because back then they seemed to improve the stability of results. I hope they are no longer necessary or may even be harmful.

cc @ggwpez

@athei
Copy link
Member Author

athei commented Mar 14, 2023

bot bench $ pallet dev pallet_contracts
bot bench $ pallet dev pallet_contracts

@ggwpez
Copy link
Member

ggwpez commented Mar 14, 2023

Looks good so far, going to re-run two more times to be sure
image

@ggwpez
Copy link
Member

ggwpez commented Mar 14, 2023

bot bench $ pallet dev pallet_contracts
bot bench $ pallet dev pallet_contracts

@ggwpez
Copy link
Member

ggwpez commented Mar 14, 2023

bot clean

@command-bot
Copy link

command-bot bot commented Mar 14, 2023

@ggwpez Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_contracts has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2519967 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2519967/artifacts/download.

@command-bot
Copy link

command-bot bot commented Mar 14, 2023

@ggwpez Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_contracts has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2519968 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2519968/artifacts/download.

@ggwpez
Copy link
Member

ggwpez commented Mar 15, 2023

Last two changes look clean as well
image

image

@athei
Copy link
Member Author

athei commented Mar 15, 2023

Nice. Looks like a clean boy. I think we can go ahead with this PR.

@athei athei 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. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Mar 15, 2023
@@ -47,10 +46,10 @@ use sp_std::prelude::*;
use wasm_instrument::parity_wasm::elements::{BlockType, BrTableData, Instruction, ValueType};

/// How many batches we do per API benchmark.
const API_BENCHMARK_BATCHES: u32 = 20;
const API_BENCHMARK_BATCHES: u32 = 1600;
Copy link
Member

Choose a reason for hiding this comment

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

Could mention in the comment why these specific values were picked

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@athei
Copy link
Member Author

athei commented Mar 16, 2023

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot paritytech-processbot bot merged commit aaa9a36 into master Mar 16, 2023
@paritytech-processbot paritytech-processbot bot deleted the at/no-bench-batch branch March 16, 2023 19:19
/// This is picked more or less arbitrary. We experimented with different numbers until
/// the results appeared to be stable. Reducing the number would speed up the benchmarks
/// but might make the results less precise.
const API_BENCHMARK_BATCHES: u32 = 1600;
Copy link
Contributor

Choose a reason for hiding this comment

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

Strictly speaking, this is not really batches anymore, is it?

Suggested change
const API_BENCHMARK_BATCHES: u32 = 1600;
const API_BENCHMARK_RUNS: u32 = 1600;

Copy link
Member Author

Choose a reason for hiding this comment

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

breathx pushed a commit to gear-tech/substrate that referenced this pull request Apr 22, 2023
* Remove batching

* Benchmark in bytes not kilobytes

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts

* Add rationale for picking batch numbers

---------

Co-authored-by: command-bot <>
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* Remove batching

* Benchmark in bytes not kilobytes

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts

* Add rationale for picking batch numbers

---------

Co-authored-by: command-bot <>
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. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants