Skip to content

Commit

Permalink
Improve complexity of CompactAssignments::unique_targets (paritytech#…
Browse files Browse the repository at this point in the history
…8314)

* Improve complexity of CompactAssignments::unique_targets

Original implementation was O(n**2). Current impl is O(n log n).

Avoided the original proposed mitigation because it does not retain
the de-duplicating property present in the original implementation.
This implementation does a little more work, but retains that property.

* Explicitly choose sp_std Vec and BTreeSet

Ensures that the macro still works if someone uses it in a context
in which sp_std is not imported or is renamed.

* explicitly use sp_std vectors throughout compact macro
  • Loading branch information
coriolinus authored and hirschenberger committed Apr 14, 2021
1 parent 47acabc commit 6c251c3
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 24 deletions.
2 changes: 1 addition & 1 deletion primitives/npos-elections/compact/src/assignment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ pub(crate) fn into_impl(count: usize, per_thing: syn::Type) -> TokenStream2 {
let target = target_at(*t_idx).or_invalid_index()?;
Ok((target, *p))
})
.collect::<Result<Vec<(A, #per_thing)>, _npos::Error>>()?;
.collect::<Result<_npos::sp_std::prelude::Vec<(A, #per_thing)>, _npos::Error>>()?;

if sum >= #per_thing::one() {
return Err(_npos::Error::CompactStakeOverflow);
Expand Down
20 changes: 10 additions & 10 deletions primitives/npos-elections/compact/src/codec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,14 @@ fn decode_impl(
quote! {
let #name =
<
Vec<(_npos::codec::Compact<#voter_type>, _npos::codec::Compact<#target_type>)>
_npos::sp_std::prelude::Vec<(_npos::codec::Compact<#voter_type>, _npos::codec::Compact<#target_type>)>
as
_npos::codec::Decode
>::decode(value)?;
let #name = #name
.into_iter()
.map(|(v, t)| (v.0, t.0))
.collect::<Vec<_>>();
.collect::<_npos::sp_std::prelude::Vec<_>>();
}
};

Expand All @@ -65,7 +65,7 @@ fn decode_impl(
quote! {
let #name =
<
Vec<(
_npos::sp_std::prelude::Vec<(
_npos::codec::Compact<#voter_type>,
(_npos::codec::Compact<#target_type>, _npos::codec::Compact<#weight_type>),
_npos::codec::Compact<#target_type>,
Expand All @@ -76,7 +76,7 @@ fn decode_impl(
let #name = #name
.into_iter()
.map(|(v, (t1, w), t2)| (v.0, (t1.0, w.0), t2.0))
.collect::<Vec<_>>();
.collect::<_npos::sp_std::prelude::Vec<_>>();
}
};

Expand All @@ -90,7 +90,7 @@ fn decode_impl(
quote! {
let #name =
<
Vec<(
_npos::sp_std::prelude::Vec<(
_npos::codec::Compact<#voter_type>,
[(_npos::codec::Compact<#target_type>, _npos::codec::Compact<#weight_type>); #c-1],
_npos::codec::Compact<#target_type>,
Expand All @@ -104,7 +104,7 @@ fn decode_impl(
[ #inner_impl ],
t_last.0,
))
.collect::<Vec<_>>();
.collect::<_npos::sp_std::prelude::Vec<_>>();
}
}).collect::<TokenStream2>();

Expand Down Expand Up @@ -142,7 +142,7 @@ fn encode_impl(ident: syn::Ident, count: usize) -> TokenStream2 {
_npos::codec::Compact(v.clone()),
_npos::codec::Compact(t.clone()),
))
.collect::<Vec<_>>();
.collect::<_npos::sp_std::prelude::Vec<_>>();
#name.encode_to(&mut r);
}
};
Expand All @@ -160,7 +160,7 @@ fn encode_impl(ident: syn::Ident, count: usize) -> TokenStream2 {
),
_npos::codec::Compact(t2.clone()),
))
.collect::<Vec<_>>();
.collect::<_npos::sp_std::prelude::Vec<_>>();
#name.encode_to(&mut r);
}
};
Expand All @@ -184,14 +184,14 @@ fn encode_impl(ident: syn::Ident, count: usize) -> TokenStream2 {
[ #inners_compact_array ],
_npos::codec::Compact(t_last.clone()),
))
.collect::<Vec<_>>();
.collect::<_npos::sp_std::prelude::Vec<_>>();
#name.encode_to(&mut r);
}
}).collect::<TokenStream2>();

quote!(
impl _npos::codec::Encode for #ident {
fn encode(&self) -> Vec<u8> {
fn encode(&self) -> _npos::sp_std::prelude::Vec<u8> {
let mut r = vec![];
#encode_impl_single
#encode_impl_double
Expand Down
25 changes: 12 additions & 13 deletions primitives/npos-elections/compact/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,14 +119,14 @@ fn struct_def(
let name = field_name_for(1);
// NOTE: we use the visibility of the struct for the fields as well.. could be made better.
quote!(
#vis #name: Vec<(#voter_type, #target_type)>,
#vis #name: _npos::sp_std::prelude::Vec<(#voter_type, #target_type)>,
)
};

let doubles = {
let name = field_name_for(2);
quote!(
#vis #name: Vec<(#voter_type, (#target_type, #weight_type), #target_type)>,
#vis #name: _npos::sp_std::prelude::Vec<(#voter_type, (#target_type, #weight_type), #target_type)>,
)
};

Expand All @@ -135,7 +135,7 @@ fn struct_def(
let field_name = field_name_for(c);
let array_len = c - 1;
quote!(
#vis #field_name: Vec<(
#vis #field_name: _npos::sp_std::prelude::Vec<(
#voter_type,
[(#target_type, #weight_type); #array_len],
#target_type
Expand Down Expand Up @@ -194,20 +194,19 @@ fn struct_def(
all_edges
}

fn unique_targets(&self) -> Vec<Self::Target> {
fn unique_targets(&self) -> _npos::sp_std::prelude::Vec<Self::Target> {
// NOTE: this implementation returns the targets sorted, but we don't use it yet per
// se, nor is the API enforcing it.
let mut all_targets: Vec<Self::Target> = Vec::with_capacity(self.average_edge_count());
use _npos::sp_std::collections::btree_set::BTreeSet;

let mut all_targets: BTreeSet<Self::Target> = BTreeSet::new();
let mut maybe_insert_target = |t: Self::Target| {
match all_targets.binary_search(&t) {
Ok(_) => (),
Err(pos) => all_targets.insert(pos, t)
}
all_targets.insert(t);
};

#unique_targets_impl

all_targets
all_targets.into_iter().collect()
}

fn remove_voter(&mut self, to_remove: Self::Voter) -> bool {
Expand All @@ -216,7 +215,7 @@ fn struct_def(
}

fn from_assignment<FV, FT, A>(
assignments: Vec<_npos::Assignment<A, #weight_type>>,
assignments: _npos::sp_std::prelude::Vec<_npos::Assignment<A, #weight_type>>,
index_of_voter: FV,
index_of_target: FT,
) -> Result<Self, _npos::Error>
Expand All @@ -243,8 +242,8 @@ fn struct_def(
self,
voter_at: impl Fn(Self::Voter) -> Option<A>,
target_at: impl Fn(Self::Target) -> Option<A>,
) -> Result<Vec<_npos::Assignment<A, #weight_type>>, _npos::Error> {
let mut assignments: Vec<_npos::Assignment<A, #weight_type>> = Default::default();
) -> Result<_npos::sp_std::prelude::Vec<_npos::Assignment<A, #weight_type>>, _npos::Error> {
let mut assignments: _npos::sp_std::prelude::Vec<_npos::Assignment<A, #weight_type>> = Default::default();
#into_impl
Ok(assignments)
}
Expand Down
2 changes: 2 additions & 0 deletions primitives/npos-elections/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,8 @@ pub use pjr::*;
pub use codec;
#[doc(hidden)]
pub use sp_arithmetic;
#[doc(hidden)]
pub use sp_std;

/// Simple Extension trait to easily convert `None` from index closures to `Err`.
///
Expand Down

0 comments on commit 6c251c3

Please sign in to comment.