-
Notifications
You must be signed in to change notification settings - Fork 2.6k
remove the uselsss weight return type from election provider API #9569
Conversation
…ove-election-weight-api
/benchmark runtime pallet pallet_staking |
Benchmark Runtime Pallet for branch "kiz-remove-election-weight-api" with command cargo run --quiet --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_staking --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/staking/src/weights.rs --template=./.maintain/frame-weight-template.hbs Results
|
/benchmark runtime pallet pallet_election_provider_multi_phase |
Benchmark Runtime Pallet for branch "kiz-remove-election-weight-api" with command cargo run --quiet --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_election_provider_multi_phase --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/election-provider-multi-phase/src/weights.rs --template=./.maintain/frame-weight-template.hbs Results
|
…ove-election-weight-api
/benchmark runtime pallet pallet_election_provider_multi_phase |
Benchmark Runtime Pallet for branch "kiz-remove-election-weight-api" with command cargo run --quiet --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_election_provider_multi_phase --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/election-provider-multi-phase/src/weights.rs --template=./.maintain/frame-weight-template.hbs Results
|
@@ -18,22 +18,22 @@ | |||
//! Autogenerated weights for pallet_staking | |||
//! | |||
//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev | |||
//! DATE: 2021-08-07, STEPS: `50`, REPEAT: 20, LOW RANGE: `[]`, HIGH RANGE: `[]` | |||
//! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled, CHAIN: Some("dev"), DB CACHE: 128 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
native?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are numbers that I ran locally, I am waiting for the bot to override them.
…path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_election_provider_multi_phase --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/election-provider-multi-phase/src/weights.rs --template=./.maintain/frame-weight-template.hbs
@@ -303,7 +307,8 @@ frame_benchmarking::benchmarks! { | |||
..Default::default() | |||
}; | |||
|
|||
MultiPhase::<T>::on_initialize_open_signed().expect("should be ok to start signed phase"); | |||
<MultiPhase<T>>::create_snapshot().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not have all these unwraps be a little more descriptive about what went wrong?
either with an Err
or an expect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless if the expect
is going to a multi-line, comprehensive description of why something failed, writing some rudimentary .expect(<foo failed>)
is not at all more descriptive than the standard error message of unwrap
.
Err
would have been nicer, but not in this case because most of these unwraps are not on dispatchables, therefore their error is not Into<&'static str>
and needs a map_err
or an implementation of that just for benchmarks.
I will look into the second option, though my opinion is still that unwrap is fine in benchmark setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with the help of strum_macros
: e6eb8b0
(#9569)
…h/substrate into kiz-remove-election-weight-api
.saturating_add(RocksDbWeight::get().reads(1 as Weight)) | ||
.saturating_add(RocksDbWeight::get().writes(1 as Weight)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its not clear to me where all these reads and writes went to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this benchmark used to be: create snapshot, and open signed phase. Now it is only "open the signed phase", and therefore way less storage.
The snapshotting operation itself has become self-weighing, and we no longer track it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall looks good.
/benchmark runtime pallet pallet_election_provider_multi_phase |
Benchmark Runtime Pallet for branch "kiz-remove-election-weight-api" with command cargo run --quiet --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_election_provider_multi_phase --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/election-provider-multi-phase/src/weights.rs --template=./.maintain/frame-weight-template.hbs Results
|
/benchmark runtime pallet pallet_staking |
Benchmark Runtime Pallet for branch "kiz-remove-election-weight-api" with command cargo run --quiet --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_staking --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/staking/src/weights.rs --template=./.maintain/frame-weight-template.hbs Results
|
…path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_election_provider_multi_phase --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/election-provider-multi-phase/src/weights.rs --template=./.maintain/frame-weight-template.hbs
… kiz-remove-election-weight-api
…path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_staking --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/staking/src/weights.rs --template=./.maintain/frame-weight-template.hbs
…h/substrate into kiz-remove-election-weight-api
bot merge |
Trying merge. |
* Fix weights * Fix more * update Substrate * fmt Co-authored-by: parity-processbot <>
* Fix weights * Fix more * update Substrate * fmt Co-authored-by: parity-processbot <>
* Fix weights * Fix more * update Substrate * fmt Co-authored-by: parity-processbot <>
This is my retaliation against my current way of benchmarking intra-pallet hooks.
In the past, I tried to push such operations toward returning a weight. This worked fine, but it made all the APIs quite ugly, with no clear benefit to compensate for it.
Over time my sentiment changed and I disliked this way of weighing things more and more. Now, I strongly think that a combination of the below are much better:
trait ElectionDataProvider::add_voter
etc.).In this PR, I move all of these
on_initialize
intra-pallet interactions to be self-weighing. This means that the destination of a call will measure its own weight and directly register it with the system pallet (asmandatory
, because this assumption is only foron_initialize
stuff). The only responsibility of the call site is to ensure it is NOT benchmarking that piece of the code twice. This is why you see functions likecreate_snapshot
are now split intocreate_snapshot_internal
andcreate_snapshot_external
.part of paritytech/polkadot-sdk#461
polkadot companion paritytech/polkadot#3662