From 05f24931a9f1e65865b1ea7e762132db9d8239ba Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Thu, 18 Mar 2021 09:38:58 +0100 Subject: [PATCH] Fast CompactAssignment search (#8385) * use more efficient search through candidates in offchain-election * mark linear accessors as test-only in election-provider-multi-phase This prevents production code which uses them from compiling. Also write an efficient helper for getting the target index. * doc grammar * use faster target_index_fn in benchmarks * unbox helper functions * remove unnecessary import * write lifetime after primary trait Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- .../src/benchmarking.rs | 2 +- .../src/helpers.rs | 78 ++++++++++++------- .../src/unsigned.rs | 2 +- .../frame/staking/src/offchain_election.rs | 22 +++--- 4 files changed, 67 insertions(+), 37 deletions(-) diff --git a/substrate/frame/election-provider-multi-phase/src/benchmarking.rs b/substrate/frame/election-provider-multi-phase/src/benchmarking.rs index 3b1b7bd7a2..0a0f0f30c3 100644 --- a/substrate/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/substrate/frame/election-provider-multi-phase/src/benchmarking.rs @@ -115,7 +115,7 @@ fn solution_with_size( let cache = helpers::generate_voter_cache::(&all_voters); let stake_of = helpers::stake_of_fn::(&all_voters, &cache); let voter_index = helpers::voter_index_fn::(&cache); - let target_index = helpers::target_index_fn_linear::(&targets); + let target_index = helpers::target_index_fn::(&targets); let voter_at = helpers::voter_at_fn::(&all_voters); let target_at = helpers::target_at_fn::(&targets); diff --git a/substrate/frame/election-provider-multi-phase/src/helpers.rs b/substrate/frame/election-provider-multi-phase/src/helpers.rs index 41d17e6aa9..a1e0c5f248 100644 --- a/substrate/frame/election-provider-multi-phase/src/helpers.rs +++ b/substrate/frame/election-provider-multi-phase/src/helpers.rs @@ -18,7 +18,7 @@ //! Some helper functions/macros for this crate. use super::{Config, VoteWeight, CompactVoterIndexOf, CompactTargetIndexOf}; -use sp_std::{collections::btree_map::BTreeMap, convert::TryInto, boxed::Box, prelude::*}; +use sp_std::{collections::btree_map::BTreeMap, convert::TryInto, prelude::*}; #[macro_export] macro_rules! log { @@ -56,10 +56,10 @@ pub fn generate_voter_cache( /// The snapshot must be the same is the one used to create `cache`. pub fn voter_index_fn( cache: &BTreeMap, -) -> Box Option> + '_> { - Box::new(move |who| { +) -> impl Fn(&T::AccountId) -> Option> + '_ { + move |who| { cache.get(who).and_then(|i| >>::try_into(*i).ok()) - }) + } } /// Same as [`voter_index_fn`], but the returning index is converted into usize, if possible. @@ -69,8 +69,8 @@ pub fn voter_index_fn( /// The snapshot must be the same is the one used to create `cache`. pub fn voter_index_fn_usize( cache: &BTreeMap, -) -> Box Option + '_> { - Box::new(move |who| cache.get(who).cloned()) +) -> impl Fn(&T::AccountId) -> Option + '_ { + move |who| cache.get(who).cloned() } /// A non-optimized, linear version of [`voter_index_fn`] that does not need a cache and does a @@ -79,64 +79,90 @@ pub fn voter_index_fn_usize( /// ## Warning /// /// Not meant to be used in production. +#[cfg(test)] pub fn voter_index_fn_linear( snapshot: &Vec<(T::AccountId, VoteWeight, Vec)>, -) -> Box Option> + '_> { - Box::new(move |who| { +) -> impl Fn(&T::AccountId) -> Option> + '_ { + move |who| { snapshot .iter() .position(|(x, _, _)| x == who) .and_then(|i| >>::try_into(i).ok()) - }) + } } -/// Create a function the returns the index a targets in the snapshot. +/// Create a function the returns the index to a target in the snapshot. /// -/// The returning index type is the same as the one defined in `T::CompactSolution::Target`. +/// The returned index type is the same as the one defined in `T::CompactSolution::Target`. +/// +/// Note: to the extent possible, the returned function should be cached and reused. Producing that +/// function requires a `O(n log n)` data transform. Each invocation of that function completes +/// in `O(log n)`. +pub fn target_index_fn( + snapshot: &Vec, +) -> impl Fn(&T::AccountId) -> Option> + '_ { + let cache: BTreeMap<_, _> = + snapshot.iter().enumerate().map(|(idx, account_id)| (account_id, idx)).collect(); + move |who| { + cache + .get(who) + .and_then(|i| >>::try_into(*i).ok()) + } +} + +/// Create a function the returns the index to a target in the snapshot. +/// +/// The returned index type is the same as the one defined in `T::CompactSolution::Target`. +/// +/// ## Warning +/// +/// Not meant to be used in production. +#[cfg(test)] pub fn target_index_fn_linear( snapshot: &Vec, -) -> Box Option> + '_> { - Box::new(move |who| { +) -> impl Fn(&T::AccountId) -> Option> + '_ { + move |who| { snapshot .iter() .position(|x| x == who) .and_then(|i| >>::try_into(i).ok()) - }) + } } /// Create a function that can map a voter index ([`CompactVoterIndexOf`]) to the actual voter /// account using a linearly indexible snapshot. pub fn voter_at_fn( snapshot: &Vec<(T::AccountId, VoteWeight, Vec)>, -) -> Box) -> Option + '_> { - Box::new(move |i| { +) -> impl Fn(CompactVoterIndexOf) -> Option + '_ { + move |i| { as TryInto>::try_into(i) .ok() .and_then(|i| snapshot.get(i).map(|(x, _, _)| x).cloned()) - }) + } } /// Create a function that can map a target index ([`CompactTargetIndexOf`]) to the actual target /// account using a linearly indexible snapshot. pub fn target_at_fn( snapshot: &Vec, -) -> Box) -> Option + '_> { - Box::new(move |i| { +) -> impl Fn(CompactTargetIndexOf) -> Option + '_ { + move |i| { as TryInto>::try_into(i) .ok() .and_then(|i| snapshot.get(i).cloned()) - }) + } } /// Create a function to get the stake of a voter. /// /// This is not optimized and uses a linear search. +#[cfg(test)] pub fn stake_of_fn_linear( snapshot: &Vec<(T::AccountId, VoteWeight, Vec)>, -) -> Box VoteWeight + '_> { - Box::new(move |who| { +) -> impl Fn(&T::AccountId) -> VoteWeight + '_ { + move |who| { snapshot.iter().find(|(x, _, _)| x == who).map(|(_, x, _)| *x).unwrap_or_default() - }) + } } /// Create a function to get the stake of a voter. @@ -148,12 +174,12 @@ pub fn stake_of_fn_linear( pub fn stake_of_fn<'a, T: Config>( snapshot: &'a Vec<(T::AccountId, VoteWeight, Vec)>, cache: &'a BTreeMap, -) -> Box VoteWeight + 'a> { - Box::new(move |who| { +) -> impl Fn(&T::AccountId) -> VoteWeight + 'a { + move |who| { if let Some(index) = cache.get(who) { snapshot.get(*index).map(|(_, x, _)| x).cloned().unwrap_or_default() } else { 0 } - }) + } } diff --git a/substrate/frame/election-provider-multi-phase/src/unsigned.rs b/substrate/frame/election-provider-multi-phase/src/unsigned.rs index 3004e69c23..4ff224d860 100644 --- a/substrate/frame/election-provider-multi-phase/src/unsigned.rs +++ b/substrate/frame/election-provider-multi-phase/src/unsigned.rs @@ -145,7 +145,7 @@ impl Pallet { // closures. let cache = helpers::generate_voter_cache::(&voters); let voter_index = helpers::voter_index_fn::(&cache); - let target_index = helpers::target_index_fn_linear::(&targets); + let target_index = helpers::target_index_fn::(&targets); let voter_at = helpers::voter_at_fn::(&voters); let target_at = helpers::target_at_fn::(&targets); let stake_of = helpers::stake_of_fn::(&voters, &cache); diff --git a/substrate/frame/staking/src/offchain_election.rs b/substrate/frame/staking/src/offchain_election.rs index cacfe454ec..8fe3fc7367 100644 --- a/substrate/frame/staking/src/offchain_election.rs +++ b/substrate/frame/staking/src/offchain_election.rs @@ -31,7 +31,7 @@ use sp_npos_elections::{ use sp_runtime::{ offchain::storage::StorageValueRef, traits::TrailingZeroInput, RuntimeDebug, }; -use sp_std::{convert::TryInto, prelude::*}; +use sp_std::{convert::TryInto, prelude::*, collections::btree_map::BTreeMap}; /// Error types related to the offchain election machinery. #[derive(RuntimeDebug)] @@ -331,18 +331,22 @@ pub fn prepare_submission( let snapshot_nominators = >::snapshot_nominators().ok_or(OffchainElectionError::SnapshotUnavailable)?; + // indexing caches + let nominator_indices: BTreeMap<_, _> = + snapshot_nominators.iter().enumerate().map(|(idx, account_id)| (account_id, idx)).collect(); + let validator_indices: BTreeMap<_, _> = + snapshot_validators.iter().enumerate().map(|(idx, account_id)| (account_id, idx)).collect(); + // all helper closures that we'd ever need. let nominator_index = |a: &T::AccountId| -> Option { - snapshot_nominators - .iter() - .position(|x| x == a) - .and_then(|i| >::try_into(i).ok()) + nominator_indices + .get(a) + .and_then(|i| >::try_into(*i).ok()) }; let validator_index = |a: &T::AccountId| -> Option { - snapshot_validators - .iter() - .position(|x| x == a) - .and_then(|i| >::try_into(i).ok()) + validator_indices + .get(a) + .and_then(|i| >::try_into(*i).ok()) }; let nominator_at = |i: NominatorIndex| -> Option { snapshot_nominators.get(i as usize).cloned()