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

Make election benchmarks more *memory-aware* #9286

Merged
13 commits merged into from
Jul 9, 2021
Merged

Make election benchmarks more *memory-aware* #9286

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
2130f1a
Make benchmarks a bit better with mem
kianenigma Jun 25, 2021
26be428
Make election benchmarks more *memory-aware*
kianenigma Jul 6, 2021
1c8c7e2
Master.into()
kianenigma Jul 6, 2021
53f0378
Fix a few errors
kianenigma Jul 6, 2021
54b6ab1
Merge branch 'master' of https://github.com/paritytech/substrate into…
Jul 7, 2021
ffe5e7a
cargo run --release --features=runtime-benchmarks --manifest-path=bin…
Jul 7, 2021
dfd34f7
Manually fix the weights
kianenigma Jul 8, 2021
1f9d911
Master.into()
kianenigma Jul 8, 2021
bce640b
Update lock file
kianenigma Jul 8, 2021
4e9061b
remove dupe
kianenigma Jul 8, 2021
06982f6
Fix tests
kianenigma Jul 8, 2021
1d2d10e
Master.into()
kianenigma Jul 9, 2021
1cc4a0b
cargo update pwasm
kianenigma Jul 9, 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
4 changes: 2 additions & 2 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 14 additions & 2 deletions bin/node/runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,6 +554,19 @@ sp_npos_elections::generate_solution_type!(
pub const MAX_NOMINATIONS: u32 =
<NposCompactSolution16 as sp_npos_elections::CompactSolution>::LIMIT as u32;

/// The numbers configured here should always be more than the the maximum limits of staking pallet
/// to ensure election snapshot will not run out of memory.
Comment on lines +557 to +558
Copy link
Member

Choose a reason for hiding this comment

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

I dont quite see how we can ensure this is always the case given the variability on chain.

I get if this is a temp solution, but i think we need to be more careful than to just add this code and forget about it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this comment no longer holds exactly. Given #9285, I had to #[ignore] these benchmarks again, so they are not always executed (which was my intention). My intention was that we always run them as we grow these parameters and it would automatically prevent us from making them too big.

Currently I am benchmarking only create_snapshot. If we do the same with on_initialize (which is only a smidgen different), and if it works here, I think then we can be pretty sure that the same success will hold on-chain as well.

how we can ensure this is always the case given the variability on chain.

Which part of it doesn't add up, and what vulnerability you mean?

Copy link
Member

Choose a reason for hiding this comment

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

The word was variability, that being that this 25_000 value can be changed on-chain, and nothing is really updating the value here in that case.

If we have some hardcoded value here, maybe we need to pass that hard-coded value through to the pallet and make sure the on-chain value is not set higher than this hard-coded value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since staking's 25_000 is a storage item and it can be changed on the fly, this is not possible. This will be some manual process that we run to ensure some new value that we might want to set in staking's storage is safe.

Copy link
Member

Choose a reason for hiding this comment

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

My thought is that we can also place a limit to the value set in storage, so it does not reach past any limit we have not prepared benchmarks for

pub struct BenchmarkConfig;
impl pallet_election_provider_multi_phase::BenchmarkingConfig for BenchmarkConfig {
const VOTERS: [u32; 2] = [5_000, 10_000];
const TARGETS: [u32; 2] = [1_000, 2_000];
const ACTIVE_VOTERS: [u32; 2] = [1000, 4_000];
const DESIRED_TARGETS: [u32; 2] = [400, 800];
const SNAPSHOT_MAXIMUM_VOTERS: u32 = 25_000;
const MINER_MAXIMUM_VOTERS: u32 = 15_000;
const MAXIMUM_TARGETS: u32 = 2000;
}

impl pallet_election_provider_multi_phase::Config for Runtime {
type Event = Event;
type Currency = Balances;
Expand All @@ -579,7 +592,7 @@ impl pallet_election_provider_multi_phase::Config for Runtime {
type Fallback = Fallback;
type WeightInfo = pallet_election_provider_multi_phase::weights::SubstrateWeight<Runtime>;
type ForceOrigin = EnsureRootOrHalfCouncil;
type BenchmarkingConfig = ();
type BenchmarkingConfig = BenchmarkConfig;
}

parameter_types! {
Expand Down Expand Up @@ -1578,7 +1591,6 @@ impl_runtime_apis! {
add_benchmark!(params, batches, pallet_uniques, Uniques);
add_benchmark!(params, batches, pallet_utility, Utility);
add_benchmark!(params, batches, pallet_vesting, Vesting);
add_benchmark!(params, batches, pallet_election_provider_multi_phase, ElectionProviderMultiPhase);

if batches.is_empty() { return Err("Benchmark not found for this pallet.".into()) }
Ok((batches, storage_info))
Expand Down
19 changes: 10 additions & 9 deletions client/allocator/src/freeing_bump.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@
//! allocation size is capped, therefore the number of orders and thus the linked lists is as well
//! limited. Currently, the maximum size of an allocation is 32 MiB.
//!
//! When the allocator serves an allocation request it first checks the linked list for the respective
//! order. If it doesn't have any free chunks, the allocator requests memory from the bump allocator.
//! In any case the order is stored in the header of the allocation.
//! When the allocator serves an allocation request it first checks the linked list for the
//! respective order. If it doesn't have any free chunks, the allocator requests memory from the
//! bump allocator. In any case the order is stored in the header of the allocation.
//!
//! Upon deallocation we get the order of the allocation from its header and then add that
//! allocation to the linked list for the respective order.
Expand All @@ -59,12 +59,13 @@
//! allocator was consumed by the 32 MiB allocation, allocations of all sizes except 32 MiB will
//! fail.
//!
//! - Sizes of allocations are rounded up to the nearest order. That is, an allocation of 2,00001 MiB
//! will be put into the bucket of 4 MiB. Therefore, any allocation of size `(N, 2N]` will take
//! up to `2N`, thus assuming a uniform distribution of allocation sizes, the average amount in use
//! of a `2N` space on the heap will be `(3N + ε) / 2`. So average utilisation is going to be around
//! 75% (`(3N + ε) / 2 / 2N`) meaning that around 25% of the space in allocation will be wasted.
//! This is more pronounced (in terms of absolute heap amounts) with larger allocation sizes.
//! - Sizes of allocations are rounded up to the nearest order. That is, an allocation of 2,00001
//! MiB will be put into the bucket of 4 MiB. Therefore, any allocation of size `(N, 2N]` will
//! take up to `2N`, thus assuming a uniform distribution of allocation sizes, the average amount
//! in use of a `2N` space on the heap will be `(3N + ε) / 2`. So average utilization is going to
//! be around 75% (`(3N + ε) / 2 / 2N`) meaning that around 25% of the space in allocation will be
//! wasted. This is more pronounced (in terms of absolute heap amounts) with larger allocation
//! sizes.

use crate::Error;
use std::{mem, convert::{TryFrom, TryInto}, ops::{Range, Index, IndexMut}};
Expand Down
177 changes: 122 additions & 55 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 @@ -20,10 +20,9 @@
use super::*;
use crate::{Pallet as MultiPhase, unsigned::IndexAssignmentOf};
use frame_benchmarking::{account, impl_benchmark_test_suite};
use frame_support::{assert_ok, traits::OnInitialize};
use frame_support::{assert_ok, traits::Hooks};
use frame_system::RawOrigin;
use rand::{prelude::SliceRandom, rngs::SmallRng, SeedableRng};
use frame_election_provider_support::Assignment;
use sp_arithmetic::{per_things::Percent, traits::One};
use sp_npos_elections::IndexAssignment;
use sp_runtime::InnerOf;
Expand All @@ -38,14 +37,14 @@ fn solution_with_size<T: Config>(
size: SolutionOrSnapshotSize,
active_voters_count: u32,
desired_targets: u32,
) -> RawSolution<CompactOf<T>> {
assert!(size.targets >= desired_targets, "must have enough targets");
assert!(
) -> Result<RawSolution<CompactOf<T>>, &'static str> {
ensure!(size.targets >= desired_targets, "must have enough targets");
ensure!(
size.targets >= (<CompactOf<T>>::LIMIT * 2) as u32,
"must have enough targets for unique votes."
);
assert!(size.voters >= active_voters_count, "must have enough voters");
assert!(
ensure!(size.voters >= active_voters_count, "must have enough voters");
ensure!(
(<CompactOf<T>>::LIMIT as u32) < desired_targets,
"must have enough winners to give them votes."
);
Expand Down Expand Up @@ -125,7 +124,7 @@ fn solution_with_size<T: Config>(
.map(|(voter, _stake, votes)| {
let percent_per_edge: InnerOf<CompactAccuracyOf<T>> =
(100 / votes.len()).try_into().unwrap_or_else(|_| panic!("failed to convert"));
Assignment {
crate::unsigned::Assignment::<T> {
who: voter.clone(),
distribution: votes
.iter()
Expand All @@ -141,7 +140,31 @@ fn solution_with_size<T: Config>(
let round = <MultiPhase<T>>::round();

assert!(score[0] > 0, "score is zero, this probably means that the stakes are not set.");
RawSolution { compact, score, round }
Ok(RawSolution { compact, score, round })
}

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

T::DataProvider::clear();
log!(info, "setting up with voters = {} [degree = {}], targets = {}", v, T::DataProvider::MAXIMUM_VOTES_PER_VOTER, t);

// fill targets.
let mut targets = (0..t).map(|i| {
let target = frame_benchmarking::account::<T::AccountId>("Target", i, SEED);
T::DataProvider::add_target(target.clone());
target
}).collect::<Vec<_>>();
// we should always have enough voters to fill.
assert!(targets.len() > T::DataProvider::MAXIMUM_VOTES_PER_VOTER as usize);
targets.truncate(T::DataProvider::MAXIMUM_VOTES_PER_VOTER as usize);

// fill voters.
(0..v).for_each(|i| {
let voter = frame_benchmarking::account::<T::AccountId>("Voter", i, SEED);
let weight = T::Currency::minimum_balance().saturated_into::<u64>() * 1000;
T::DataProvider::add_voter(voter, weight, targets.clone());
});
}

frame_benchmarking::benchmarks! {
Expand Down Expand Up @@ -223,14 +246,18 @@ frame_benchmarking::benchmarks! {

// a call to `<Pallet as ElectionProvider>::elect` where we only return the queued solution.
elect_queued {
// assume largest values for the election status. These will merely affect the decoding.
let v = T::BenchmarkingConfig::VOTERS[1];
let t = T::BenchmarkingConfig::TARGETS[1];
let a = T::BenchmarkingConfig::ACTIVE_VOTERS[1];
let d = T::BenchmarkingConfig::DESIRED_TARGETS[1];
// 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. compact.len(). This means the active nominators, thus must be
// a subset of `v` component.
let a in (T::BenchmarkingConfig::ACTIVE_VOTERS[0]) .. T::BenchmarkingConfig::ACTIVE_VOTERS[1];
// number of desired targets. Must be a subset of `t` component.
let d in (T::BenchmarkingConfig::DESIRED_TARGETS[0]) .. T::BenchmarkingConfig::DESIRED_TARGETS[1];

let witness = SolutionOrSnapshotSize { voters: v, targets: t };
let raw_solution = solution_with_size::<T>(witness, a, d);
let raw_solution = solution_with_size::<T>(witness, a, d)?;
let ready_solution =
<MultiPhase<T>>::feasibility_check(raw_solution, ElectionCompute::Signed).unwrap();

Expand All @@ -251,15 +278,6 @@ frame_benchmarking::benchmarks! {
assert_eq!(<CurrentPhase<T>>::get(), <Phase<T::BlockNumber>>::Off);
}

#[extra]
create_snapshot {
assert!(<MultiPhase<T>>::snapshot().is_none());
}: {
<MultiPhase::<T>>::create_snapshot().unwrap()
} verify {
assert!(<MultiPhase<T>>::snapshot().is_some());
}

submit {
let c in 1 .. (T::SignedMaxSubmissions::get() - 1);

Expand Down Expand Up @@ -307,7 +325,7 @@ frame_benchmarking::benchmarks! {
T::BenchmarkingConfig::DESIRED_TARGETS[1];

let witness = SolutionOrSnapshotSize { voters: v, targets: t };
let raw_solution = solution_with_size::<T>(witness, a, d);
let raw_solution = solution_with_size::<T>(witness, a, d)?;

assert!(<MultiPhase<T>>::queued_solution().is_none());
<CurrentPhase<T>>::put(Phase::Unsigned((true, 1u32.into())));
Expand All @@ -324,6 +342,84 @@ frame_benchmarking::benchmarks! {
assert!(<MultiPhase<T>>::queued_solution().is_some());
}

// This is checking a valid solution. The worse case is indeed a valid solution.
feasibility_check {
// 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. compact.len(). This means the active nominators, thus must be
// a subset of `v` component.
let a in (T::BenchmarkingConfig::ACTIVE_VOTERS[0]) .. T::BenchmarkingConfig::ACTIVE_VOTERS[1];
// number of desired targets. Must be a subset of `t` component.
let d in (T::BenchmarkingConfig::DESIRED_TARGETS[0]) .. T::BenchmarkingConfig::DESIRED_TARGETS[1];

let size = SolutionOrSnapshotSize { voters: v, targets: t };
let raw_solution = solution_with_size::<T>(size, a, d)?;

assert_eq!(raw_solution.compact.voter_count() as u32, a);
assert_eq!(raw_solution.compact.unique_targets().len() as u32, d);

// encode the most significant storage item that needs to be decoded in the dispatch.
let encoded_snapshot = <MultiPhase<T>>::snapshot().unwrap().encode();
}: {
assert_ok!(<MultiPhase<T>>::feasibility_check(raw_solution, ElectionCompute::Unsigned));
let _decoded_snap = <RoundSnapshot<T::AccountId> as Decode>::decode(&mut &*encoded_snapshot).unwrap();
}

// NOTE: this weight is not used anywhere, but the fact that it should succeed when execution in
// isolation is vital to ensure memory-safety. For the same reason, we don't care about the
// components iterating, we merely check that this operation will work with the "maximum"
// numbers.
//
// ONLY run this benchmark in isolation, and pass the `--extra` flag to enable it.
//
// NOTE: If this benchmark does not run out of memory with a given heap pages, it means that the
// OCW process can SURELY succeed with the given configuration, but the opposite is not true.
// This benchmark is doing more work than a raw call to `OffchainWorker_offchain_worker` runtime
// api call, since it is also setting up some mock data, which will itself exhaust the heap to
// some extent.
#[extra]
mine_solution_offchain_memory {
// number of votes in snapshot. Fixed to maximum.
let v = T::BenchmarkingConfig::MINER_MAXIMUM_VOTERS;
// 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)));
<MultiPhase::<T>>::create_snapshot().unwrap();
}: {
// we can't really verify this as it won't write anything to state, check logs.
<MultiPhase::<T>>::offchain_worker(now)
}

// NOTE: this weight is not used anywhere, but the fact that it should succeed when execution in
// isolation is vital to ensure memory-safety. For the same reason, we don't care about the
// components iterating, we merely check that this operation will work with the "maximum"
// numbers.
//
// ONLY run this benchmark in isolation, and pass the `--extra` flag to enable it.
#[extra]
create_snapshot_memory {
Comment on lines +404 to +406
Copy link
Member

Choose a reason for hiding this comment

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

do we have some process that will execute this when things change? or just internal knowledge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yet, internal knowledge.

// 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;

T::DataProvider::clear();
set_up_data_provider::<T>(v, t);
assert!(<MultiPhase<T>>::snapshot().is_none());
}: {
<MultiPhase::<T>>::create_snapshot().unwrap()
} 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);
}

#[extra]
trim_assignments_length {
// number of votes in snapshot.
Expand All @@ -344,7 +440,7 @@ frame_benchmarking::benchmarks! {
// Compute a random solution, then work backwards to get the lists of voters, targets, and
// assignments
let witness = SolutionOrSnapshotSize { voters: v, targets: t };
let RawSolution { compact, .. } = solution_with_size::<T>(witness, a, d);
let RawSolution { compact, .. } = solution_with_size::<T>(witness, a, d)?;
let RoundSnapshot { voters, targets } = MultiPhase::<T>::snapshot().unwrap();
let voter_at = helpers::voter_at_fn::<T>(&voters);
let target_at = helpers::target_at_fn::<T>(&targets);
Expand Down Expand Up @@ -394,35 +490,6 @@ frame_benchmarking::benchmarks! {
log!(trace, "actual encoded size = {}", encoding.len());
assert!(encoding.len() <= desired_size);
}

// This is checking a valid solution. The worse case is indeed a valid solution.
feasibility_check {
// 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. compact.len(). This means the active nominators, thus must be
// a subset of `v` component.
let a in
(T::BenchmarkingConfig::ACTIVE_VOTERS[0]) .. T::BenchmarkingConfig::ACTIVE_VOTERS[1];
// number of desired targets. Must be a subset of `t` component.
let d in
(T::BenchmarkingConfig::DESIRED_TARGETS[0]) ..
T::BenchmarkingConfig::DESIRED_TARGETS[1];

let size = SolutionOrSnapshotSize { voters: v, targets: t };
let raw_solution = solution_with_size::<T>(size, a, d);

assert_eq!(raw_solution.compact.voter_count() as u32, a);
assert_eq!(raw_solution.compact.unique_targets().len() as u32, d);

// encode the most significant storage item that needs to be decoded in the dispatch.
let encoded_snapshot = <MultiPhase<T>>::snapshot().unwrap().encode();
}: {
assert_ok!(<MultiPhase<T>>::feasibility_check(raw_solution, ElectionCompute::Unsigned));
let _decoded_snap = <RoundSnapshot<T::AccountId> as Decode>::decode(&mut &*encoded_snapshot)
.unwrap();
}
}

impl_benchmark_test_suite!(
Expand Down
Loading