Use temporary db for benchmarking #12254

michalkucharczyk · 2022-09-13T12:50:06Z

If no db option was given benchmarks shall use temporary database. Otherwise the test can use locally stored database which maybe out-of-date causing test to fail.

ggwpez · 2022-09-13T12:58:10Z

If no db option was given benchmarks shall use temporary database.

This should happen automatically depending on whether a dev runtime or real runtime is used.

Otherwise the test can use locally stored database which maybe out-of-date causing test to fail.

In which cases did you observe this?

bkchr · 2022-09-13T13:04:47Z

utils/frame/benchmarking-cli/src/lib.rs

+			self, cmd, cmd.base_path()
+		};
+
+		match inner {


Some comment on why we are doing this would be nice.

michalkucharczyk · 2022-09-13T13:46:55Z

To reproduce the issue (assuming no local database stored):
Check out polkadot repo locally at 7222d32.
Execute tests cargo test --release --features=runtime-benchmarks -- all good so far.

Then switch to 00fed3b, and run tests again.
Tests should start to fail (it was my case).

The error message was:

...
2022-09-13 15:41:45 Running 1 warmups...    
2022-09-13 15:41:45 ParentBlockRandomness did not provide entropy    
2022-09-13 15:41:45 Executing block 1 times    
2022-09-13 15:41:45 ParentBlockRandomness did not provide entropy    
2022-09-13 15:41:45 Executing a balances::transfer_keep_alive extrinsic takes[ns]:
Total: 111840
Min: 111840, Max: 111840
Average: 111840, Median: 111840, Stddev: 0
Percentiles 99th, 95th, 75th: 111840, 111840, 111840    
2022-09-13 15:41:45 👶 Creating empty BABE epoch changes on what appears to be first startup.    
2022-09-13 15:41:45 Essential task `txpool-background` failed. Shutting down service.    
Error: 
   0: Extrinsic is not valid: TransactionValidityError::Invalid(InvalidTransaction::BadProof)
   1: Transaction has a bad signature

Backtrace omitted. Run with RUST_BACKTRACE=1 environment variable to display it.
Run with RUST_BACKTRACE=full to include source snippets.
2022-09-13 15:41:45 ParentBlockRandomness did not provide entropy    
2022-09-13 15:41:45 ParentBlockRandomness did not provide entropy    
2022-09-13 15:41:45 ParentBlockRandomness did not provide entropy    
2022-09-13 15:41:45 Running 1 warmups...    
2022-09-13 15:41:45 ParentBlockRandomness did not provide entropy    
2022-09-13 15:41:45 Executing block 1 times    
2022-09-13 15:41:45 ParentBlockRandomness did not provide entropy    
2022-09-13 15:41:45 ParentBlockRandomness did not provide entropy    
2022-09-13 15:41:45 ParentBlockRandomness did not provide entropy    
2022-09-13 15:41:45 ParentBlockRandomness did not provide entropy    
2022-09-13 15:41:45 Building block, this takes some time...    
test benchmark_extrinsic_works ... FAILED

failures:

---- benchmark_extrinsic_works stdout ----
thread 'benchmark_extrinsic_works' panicked at 'assertion failed: benchmark_extrinsic(&runtime, pallet, extrinsic).is_ok()', tests/benchmark_extrinsic.rs:31:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    benchmark_extrinsic_works

test result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 6.31s

error: test failed, to rerun pass '--test benchmark_extrinsic'

...

The single command line to reproduce the issue:
cargo run --features=runtime-benchmarks benchmark extrinsic --chain rococo-dev --pallet system --extrinsic remark --repeat=1 --warmup=1 --max-ext-per-block=1
Adding -d test-empty-db parameter makes tests passing.

Also removal of $HOME/.local/share/polkadot makes the tests passing again (I am on Ubuntu).

michalkucharczyk · 2022-09-13T14:28:45Z

This should happen automatically depending on whether a dev runtime or real runtime is used.

dev is not set to true in this case:

SharedParams { chain: Some("rococo-dev"), dev: false, base_path: None, log: [], detailed_log_output: false, disable_log_color: false, enable_log_reloading: false, tracing_targets: None, tracing_receiver: Log }

ggwpez · 2022-09-13T14:29:01Z

The single command line to reproduce the issue: cargo run --features=runtime-benchmarks benchmark extrinsic --chain rococo-dev

But then there is an issue with it not using a temporary directory for a dev runtime. IIUC rococo-dev should always use a temp directory, or not?
This is then not a bug in benchmarking per-se.

michalkucharczyk · 2022-09-13T14:35:03Z

The parameters are modifed to -dev here:
https://github.com/paritytech/polkadot/blob/2dda7bb0b806e0303d53a7884dceae5b13138d5d/tests/benchmark_extrinsic.rs#L30

And later executed w/o --dev:
https://github.com/paritytech/polkadot/blob/2dda7bb0b806e0303d53a7884dceae5b13138d5d/tests/benchmark_extrinsic.rs#L45-L49

ggwpez · 2022-09-13T14:48:03Z

dev is not set to true in this case:

SharedParams { chain: Some("rococo-dev"), dev: false, base_path: None, log: [], detailed_log_output: false, disable_log_color: false, enable_log_reloading: false, tracing_targets: None, tracing_receiver: Log }

Okay that is a surprise for me. I was under the impression all -dev runtimes always imply --dev, since it cannot be passed explicit: error: The argument '--chain <CHAIN_SPEC>' cannot be used with '--dev. Is this expected behaviour @bkchr?

And later executed w/o --dev:

Yea that test checks that the command succeeds only with -dev runtimes, but maybe it already created the dir after checking that…

michalkucharczyk · 2022-09-14T07:27:56Z

Okay that is a surprise for me. I was under the impression all -dev runtimes always imply --dev, since it cannot be passed explicit: error: The argument '--chain <CHAIN_SPEC>' cannot be used with '--dev. Is this expected behaviour @bkchr?

I think --dev option conflicts with --chain by design:

substrate/client/cli/src/params/shared_params.rs

Lines 36 to 39 in 23bb5a6

    
           	/// This flag sets `--chain=dev`, `--force-authoring`, `--rpc-cors=all`, 
        
           	/// `--alice`, and `--tmp` flags, unless explicitly overridden. 
        
           	#[clap(long, conflicts_with_all = &["chain"])] 
        
           	pub dev: bool,

From what I see the -dev suffix in chain name does not influence any of other parameters, the chain's name is not checked for presence of -dev, e.g:
https://github.com/paritytech/polkadot/blob/2b0d83258089578b65712e143c794cf4922941de/cli/src/command.rs#L101

bkchr · 2022-09-14T07:31:37Z

dev is not set to true in this case:
SharedParams { chain: Some("rococo-dev"), dev: false, base_path: None, log: [], detailed_log_output: false, disable_log_color: false, enable_log_reloading: false, tracing_targets: None, tracing_receiver: Log }
Okay that is a surprise for me. I was under the impression all -dev runtimes always imply --dev, since it cannot be passed explicit: error: The argument '--chain <CHAIN_SPEC>' cannot be used with '--dev. Is this expected behaviour @bkchr?

Yes, sort of. --dev had always this same behavior. The assumption that there is only one dev chain. One of the fuckups of the cli. Maybe we should just get entirely rid of it.

ggwpez

Yes, sort of. --dev had always this same behavior. The assumption that there is only one dev chain. One of the fuckups of the cli. Maybe we should just get entirely rid of it.

Yea either removing the -dev runtimes or the --dev flag would be nice.

michalkucharczyk · 2022-09-16T15:55:07Z

Not sure if doable, but how about extending --dev with chain name?
Default name could be dev.
e.g.
--dev -> --chain=dev --force-authoring --rpc-cors=all --alice --tmp ...
--dev=polkadot-dev -> --chain=polkadot-dev --force-authoring --rpc-cors=all --alice --tmp ...

Chain name given to --dev could be restricted to those ending with -dev. (Or alternatively -dev could be appended to the chain name provided).

ggwpez · 2022-09-16T16:01:20Z

Chain name given to --dev could be restricted to those ending with -dev. (Or alternatively -dev could be appended to the chain name provided).

Are --dev and chain not orthogonal to each other? I would rather delete the -dev chains.

michalkucharczyk · 2022-09-16T16:07:10Z

I may be wrong but -dev chains are about the chain parameters (authority set, wasm blob used).
--dev option forces some node's parameters.

https://github.com/paritytech/polkadot/blob/28acc786eedd607954a84078f15e6e27ab66d90f/cli/src/command.rs#L118https://github.com/paritytech/polkadot/blob/28acc786eedd607954a84078f15e6e27ab66d90f/node/service/src/chain_spec.rs#L1720

…es-tmp-db

utils/frame/benchmarking-cli/src/lib.rs

bkchr · 2022-09-20T20:59:06Z

bot merge

paritytech-processbot · 2022-09-20T20:59:13Z

Waiting for commit status.

* master: [Fix] parameter_types! dead code errors (#12340) [Feature] Sequential migration execution for try-runtime (#12319) bench: Use `_` instead of `::` in auto-generated file names (#12332) Fast Unstake Pallet (#12129) Rename anonymous to pure proxy (#12283) Migrate remaining old decl_* macros to the new pallet attribute macros (#12271) pallet-utility: Disallow none origin (#12321) Make automatic storage deposits resistant against changing deposit prices (#12083) Format templates and fix `--steps` default value (#12286) Bump `wasmtime` to 1.0.0 (#12317) Introduce 'intermediate_insert' method to hide implementation details (#12215) Bound staking storage items (#12230) Use `array-bytes` for All Array/Bytes/Hex Operations (#12190) BREAKING: Rename Origin (#12258) Use temporary db for benchmarking (#12254) rpc: Implement `chainSpec` RPC API (#12261) Import target block body during warp sync (#12300) Proper naming wrt expectations (#12311) [ci] Revert cancel-pipeline job (#12309)

* Use temporary db for benchmarking If no db option was given benchmarks shall use temporary database. Otherwise the test can use locally stored database which maybe out-of-date causing test to fail. * nicer syntax * explanatory comment added * Update utils/frame/benchmarking-cli/src/lib.rs Co-authored-by: Bastian Köcher <[email protected]>

Use temporary db for benchmarking

6e57431

If no db option was given benchmarks shall use temporary database. Otherwise the test can use locally stored database which maybe out-of-date causing test to fail.

github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Sep 13, 2022

nicer syntax

30d391e

bkchr reviewed Sep 13, 2022

View reviewed changes

explanatory comment added

58b2a8b

michalkucharczyk added 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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Sep 13, 2022

ggwpez approved these changes Sep 16, 2022

View reviewed changes

michalkucharczyk marked this pull request as ready for review September 19, 2022 07:10

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 Sep 19, 2022

michalkucharczyk added 2 commits September 19, 2022 12:39

Merge remote-tracking branch 'origin/master' into mku-benchmarking-us…

6c11bdc

…es-tmp-db

Merge remote-tracking branch 'origin/master' into mku-benchmarking-us…

46f84a3

…es-tmp-db

michalkucharczyk requested a review from bkchr September 19, 2022 15:03

bkchr approved these changes Sep 20, 2022

View reviewed changes

utils/frame/benchmarking-cli/src/lib.rs Outdated Show resolved Hide resolved

Update utils/frame/benchmarking-cli/src/lib.rs

5e37635

paritytech-processbot bot merged commit 006dbc0 into master Sep 20, 2022

paritytech-processbot bot deleted the mku-benchmarking-uses-tmp-db branch September 20, 2022 21:19

michalkucharczyk mentioned this pull request Nov 24, 2023

Do not pollute global base path with export genesis/wasm paritytech/polkadot-sdk#2487

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use temporary db for benchmarking #12254

Use temporary db for benchmarking #12254

michalkucharczyk commented Sep 13, 2022

ggwpez commented Sep 13, 2022

bkchr Sep 13, 2022

michalkucharczyk Sep 13, 2022

michalkucharczyk commented Sep 13, 2022

michalkucharczyk commented Sep 13, 2022

ggwpez commented Sep 13, 2022 •

edited

Loading

michalkucharczyk commented Sep 13, 2022 •

edited

Loading

ggwpez commented Sep 13, 2022

michalkucharczyk commented Sep 14, 2022 •

edited

Loading

bkchr commented Sep 14, 2022

ggwpez left a comment

michalkucharczyk commented Sep 16, 2022 •

edited

Loading

ggwpez commented Sep 16, 2022 •

edited

Loading

michalkucharczyk commented Sep 16, 2022 •

edited

Loading

bkchr commented Sep 20, 2022

paritytech-processbot bot commented Sep 20, 2022

Use temporary db for benchmarking #12254

Use temporary db for benchmarking #12254

Conversation

michalkucharczyk commented Sep 13, 2022

ggwpez commented Sep 13, 2022

bkchr Sep 13, 2022

Choose a reason for hiding this comment

michalkucharczyk Sep 13, 2022

Choose a reason for hiding this comment

michalkucharczyk commented Sep 13, 2022

michalkucharczyk commented Sep 13, 2022

ggwpez commented Sep 13, 2022 • edited Loading

michalkucharczyk commented Sep 13, 2022 • edited Loading

ggwpez commented Sep 13, 2022

michalkucharczyk commented Sep 14, 2022 • edited Loading

bkchr commented Sep 14, 2022

ggwpez left a comment

Choose a reason for hiding this comment

michalkucharczyk commented Sep 16, 2022 • edited Loading

ggwpez commented Sep 16, 2022 • edited Loading

michalkucharczyk commented Sep 16, 2022 • edited Loading

bkchr commented Sep 20, 2022

paritytech-processbot bot commented Sep 20, 2022

ggwpez commented Sep 13, 2022 •

edited

Loading

michalkucharczyk commented Sep 13, 2022 •

edited

Loading

michalkucharczyk commented Sep 14, 2022 •

edited

Loading

michalkucharczyk commented Sep 16, 2022 •

edited

Loading

ggwpez commented Sep 16, 2022 •

edited

Loading

michalkucharczyk commented Sep 16, 2022 •

edited

Loading