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

remove the uselsss weight return type from election provider API #9569

Merged
19 commits merged into from
Aug 19, 2021
Merged

remove the uselsss weight return type from election provider API #9569

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
1873a7a
remove the uselsss weight return type from election provider API
kianenigma Aug 16, 2021
bda35d5
Merge branch 'master' of github.com:paritytech/substrate into kiz-rem…
kianenigma Aug 17, 2021
101b5e8
fix everything, should be ready for final benchmark
kianenigma Aug 17, 2021
b71433c
simplify on_init a bit furhter
kianenigma Aug 17, 2021
87c2416
Merge branch 'master' of github.com:paritytech/substrate into kiz-rem…
kianenigma Aug 17, 2021
5707bce
cargo run --quiet --release --features=runtime-benchmarks --manifest-…
Aug 17, 2021
56796ac
Merge branch 'master' of https://github.com/paritytech/substrate into…
Aug 18, 2021
293c076
cargo run --quiet --release --features=runtime-benchmarks --manifest-…
Aug 18, 2021
e6eb8b0
remove unwraps
kianenigma Aug 18, 2021
924b884
Merge branch 'kiz-remove-election-weight-api' of github.com:paritytec…
kianenigma Aug 18, 2021
d3f4cc5
fmt
kianenigma Aug 18, 2021
ca0568b
Merge branch 'master' of github.com:paritytech/substrate into kiz-rem…
kianenigma Aug 18, 2021
1ce23e6
Update lock file
kianenigma Aug 18, 2021
731c4b2
whitelist block weight
kianenigma Aug 18, 2021
1dfaee2
cargo run --quiet --release --features=runtime-benchmarks --manifest-…
Aug 18, 2021
faa09b5
Merge branch 'master' of https://github.com/paritytech/substrate into…
Aug 18, 2021
779bb5f
cargo run --quiet --release --features=runtime-benchmarks --manifest-…
Aug 18, 2021
c1962c8
fix warning
kianenigma Aug 18, 2021
e5a0e12
Merge branch 'kiz-remove-election-weight-api' of github.com:paritytec…
kianenigma Aug 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion frame/babe/src/mock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,6 @@ parameter_types! {
impl onchain::Config for Test {
type AccountId = <Self as frame_system::Config>::AccountId;
type BlockNumber = <Self as frame_system::Config>::BlockNumber;
type BlockWeights = ();
type Accuracy = Perbill;
type DataProvider = Staking;
}
Expand Down
7 changes: 5 additions & 2 deletions frame/election-provider-multi-phase/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ sp-io = { version = "4.0.0-dev", path = "../../primitives/io" }
sp-npos-elections = { version = "4.0.0-dev", default-features = false, path = "../../primitives/npos-elections" }
sp-tracing = { version = "4.0.0-dev", path = "../../primitives/tracing" }
frame-election-provider-support = { version = "4.0.0-dev", features = [
"runtime-benchmarks",
], path = "../election-provider-support" }
pallet-balances = { version = "4.0.0-dev", path = "../balances" }
frame-benchmarking = { version = "4.0.0-dev", path = "../benchmarking" }
Expand All @@ -68,5 +67,9 @@ std = [
"frame-election-provider-support/std",
"log/std",
]
runtime-benchmarks = ["frame-benchmarking", "rand"]
runtime-benchmarks = [
"frame-benchmarking",
"frame-election-provider-support/runtime-benchmarks",
"rand",
]
try-runtime = ["frame-support/try-runtime"]
71 changes: 37 additions & 34 deletions frame/election-provider-multi-phase/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,6 @@ fn solution_with_size<T: Config>(
}

fn set_up_data_provider<T: Config>(v: u32, t: u32) {
// number of votes in snapshot.

T::DataProvider::clear();
log!(
info,
Expand Down Expand Up @@ -192,37 +190,22 @@ frame_benchmarking::benchmarks! {
}

on_initialize_open_signed {
// NOTE: this benchmark currently doesn't have any components because the length of a db
// read/write is not captured. Otherwise, it is quite influenced by how much data
// `T::ElectionDataProvider` is reading and passing on.
Comment on lines -195 to -197
Copy link
Member

Choose a reason for hiding this comment

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

this note is still relevant?

Could update benchmarks in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the past this benchmark created the snapshot as well, and therefore it was a function of the byte length. Now, it is not doing that anymore, therefore byte length is irrelevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get that the removal of these comments seem like mistakes, but they are (hopefully) exactly all signs of reduction of complexity: we no longer need to care about these details in this pallet. Whoever is the implementer of T::DataProvider will record its own appropriate weight.

assert!(<MultiPhase<T>>::snapshot().is_none());
assert!(<MultiPhase<T>>::current_phase().is_off());
}: {
<MultiPhase<T>>::on_initialize_open_signed().unwrap();
<MultiPhase<T>>::on_initialize_open_signed();
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
} verify {
assert!(<MultiPhase<T>>::snapshot().is_some());
assert!(<MultiPhase<T>>::snapshot().is_none());
assert!(<MultiPhase<T>>::current_phase().is_signed());
}

on_initialize_open_unsigned_with_snapshot {
on_initialize_open_unsigned {
assert!(<MultiPhase<T>>::snapshot().is_none());
assert!(<MultiPhase<T>>::current_phase().is_off());
}: {
<MultiPhase<T>>::on_initialize_open_unsigned(true, true, 1u32.into()).unwrap();
} verify {
assert!(<MultiPhase<T>>::snapshot().is_some());
assert!(<MultiPhase<T>>::current_phase().is_unsigned());
}

on_initialize_open_unsigned_without_snapshot {
// need to assume signed phase was open before
<MultiPhase<T>>::on_initialize_open_signed().unwrap();
assert!(<MultiPhase<T>>::snapshot().is_some());
assert!(<MultiPhase<T>>::current_phase().is_signed());
}: {
<MultiPhase<T>>::on_initialize_open_unsigned(false, true, 1u32.into()).unwrap();
<MultiPhase<T>>::on_initialize_open_unsigned(true, 1u32.into())
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
} verify {
assert!(<MultiPhase<T>>::snapshot().is_some());
assert!(<MultiPhase<T>>::snapshot().is_none());
assert!(<MultiPhase<T>>::current_phase().is_unsigned());
}

Expand Down Expand Up @@ -259,30 +242,51 @@ frame_benchmarking::benchmarks! {
assert_eq!(T::Currency::reserved_balance(&receiver), 0u32.into());
}

create_snapshot_internal {
// number of votes in snapshot. Fixed to maximum.
let v = T::BenchmarkingConfig::SNAPSHOT_MAXIMUM_VOTERS;
// number of targets in snapshot. Fixed to maximum.
let t = T::BenchmarkingConfig::MAXIMUM_TARGETS;
kianenigma marked this conversation as resolved.
Show resolved Hide resolved

// we don't directly need the data-provider to be populated, but it is just easy to use it.
set_up_data_provider::<T>(v, t);
let targets = T::DataProvider::targets(None).unwrap();
let voters = T::DataProvider::voters(None).unwrap();
let desired_targets = T::DataProvider::desired_targets().unwrap();
assert!(<MultiPhase<T>>::snapshot().is_none());
}: {
<MultiPhase::<T>>::create_snapshot_internal(targets, voters, desired_targets)
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
} verify {
assert!(<MultiPhase<T>>::snapshot().is_some());
assert_eq!(<MultiPhase<T>>::snapshot_metadata().unwrap().voters, v + t);
assert_eq!(<MultiPhase<T>>::snapshot_metadata().unwrap().targets, t);
}

// a call to `<Pallet as ElectionProvider>::elect` where we only return the queued solution.
elect_queued {
// number of votes in snapshot.
let v in (T::BenchmarkingConfig::VOTERS[0]) .. T::BenchmarkingConfig::VOTERS[1];
// number of targets in snapshot.
let t in (T::BenchmarkingConfig::TARGETS[0]) .. T::BenchmarkingConfig::TARGETS[1];
// number of assignments, i.e. solution.len(). This means the active nominators, thus must be
// a subset of `v` component.
// a subset of `v`.
let a in (T::BenchmarkingConfig::ACTIVE_VOTERS[0]) .. T::BenchmarkingConfig::ACTIVE_VOTERS[1];
// number of desired targets. Must be a subset of `t` component.
// number of desired targets. Must be a subset of `t`.
let d in (T::BenchmarkingConfig::DESIRED_TARGETS[0]) .. T::BenchmarkingConfig::DESIRED_TARGETS[1];
kianenigma marked this conversation as resolved.
Show resolved Hide resolved

// number of votes in snapshot. Not dominant.
let v = T::BenchmarkingConfig::VOTERS[1];
// number of targets in snapshot. Not dominant.
let t = T::BenchmarkingConfig::TARGETS[1];

let witness = SolutionOrSnapshotSize { voters: v, targets: t };
let raw_solution = solution_with_size::<T>(witness, a, d)?;
let ready_solution =
<MultiPhase<T>>::feasibility_check(raw_solution, ElectionCompute::Signed).unwrap();
<CurrentPhase<T>>::put(Phase::Signed);
// assume a queued solution is stored, regardless of where it comes from.
<QueuedSolution<T>>::put(ready_solution);

// these are set by the `solution_with_size` function.
assert!(<DesiredTargets<T>>::get().is_some());
assert!(<Snapshot<T>>::get().is_some());
assert!(<SnapshotMetadata<T>>::get().is_some());
<CurrentPhase<T>>::put(Phase::Signed);
// assume a queued solution is stored, regardless of where it comes from.
<QueuedSolution<T>>::put(ready_solution);
}: {
assert_ok!(<MultiPhase<T> as ElectionProvider<T::AccountId, T::BlockNumber>>::elect());
} verify {
Expand All @@ -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();
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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)

MultiPhase::<T>::on_initialize_open_signed();
<Round<T>>::put(1);

let mut signed_submissions = SignedSubmissions::<T>::get();
Expand Down Expand Up @@ -407,7 +412,6 @@ frame_benchmarking::benchmarks! {
// number of targets in snapshot. Fixed to maximum.
let t = T::BenchmarkingConfig::MAXIMUM_TARGETS;

T::DataProvider::clear();
set_up_data_provider::<T>(v, t);
let now = frame_system::Pallet::<T>::block_number();
<CurrentPhase<T>>::put(Phase::Unsigned((true, now)));
Expand All @@ -430,7 +434,6 @@ frame_benchmarking::benchmarks! {
// number of targets in snapshot. Fixed to maximum.
let t = T::BenchmarkingConfig::MAXIMUM_TARGETS;

T::DataProvider::clear();
set_up_data_provider::<T>(v, t);
assert!(<MultiPhase<T>>::snapshot().is_none());
}: {
Expand Down
Loading