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

Collective: Benchmark with greater MaxProposals #12454

Merged
paritytech-processbot merged 19 commits into from
Nov 14, 2022
Merged

Collective: Benchmark with greater MaxProposals #12454

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
a8bc2cc
Collective: Benchmark with greated
Szegoo Oct 9, 2022
8ce5fa2
fix
Szegoo Oct 9, 2022
ddefb59
remove bs
Szegoo Oct 9, 2022
d3e2579
id_to_remark_data
Szegoo Oct 9, 2022
0830040
fix
Szegoo Oct 10, 2022
08fcd43
remove hardcoded
Szegoo Oct 10, 2022
a887818
clean up
Szegoo Oct 11, 2022
375f999
simplify
Szegoo Oct 11, 2022
93586a2
questionable renaming
Szegoo Oct 11, 2022
167efdb
better variable name
Szegoo Oct 11, 2022
80e9937
better solution
Szegoo Oct 11, 2022
bb65d9a
no need for large length
Szegoo Oct 11, 2022
61deeac
better solution
Szegoo Oct 12, 2022
79416e6
Update frame/collective/src/benchmarking.rs
Szegoo Oct 13, 2022
f57994a
fix
Szegoo Oct 13, 2022
1f5208d
test
Szegoo Oct 13, 2022
1769703
remove test
Szegoo Oct 13, 2022
bd89f9b
Merge branch 'paritytech:master' into collective-bench
Szegoo Oct 13, 2022
6e2de72
Merge branch 'paritytech:master' into collective-bench
Szegoo Nov 13, 2022
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
42 changes: 20 additions & 22 deletions frame/collective/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ fn assert_last_event<T: Config<I>, I: 'static>(generic_event: <T as Config<I>>::
frame_system::Pallet::<T>::assert_last_event(generic_event.into());
}

fn id_to_remark_data<T: Config<I>, I: 'static>(id: u32, length: usize) -> Vec<u8> {
id.to_le_bytes().into_iter().cycle().take(length).collect()
}

benchmarks_instance_pallet! {
set_members {
let m in 0 .. T::MaxMembers::get();
Expand Down Expand Up @@ -64,9 +68,7 @@ benchmarks_instance_pallet! {
let length = 100;
for i in 0 .. p {
// Proposals should be different so that different proposal hashes are generated
let proposal: T::Proposal = SystemCall::<T>::remark {
remark: vec![i as u8; length]
}.into();
let proposal: T::Proposal = SystemCall::<T>::remark { remark: id_to_remark_data::<T, I>(i, length) }.into();
Collective::<T, I>::propose(
SystemOrigin::Signed(old_members.last().unwrap().clone()).into(),
threshold,
Expand Down Expand Up @@ -105,7 +107,7 @@ benchmarks_instance_pallet! {
}

execute {
let b in 1 .. MAX_BYTES;
let b in 2 .. MAX_BYTES;
let m in 1 .. T::MaxMembers::get();

let bytes_in_storage = b + size_of::<u32>() as u32;
Expand All @@ -122,7 +124,7 @@ benchmarks_instance_pallet! {

Collective::<T, I>::set_members(SystemOrigin::Root.into(), members, None, T::MaxMembers::get())?;

let proposal: T::Proposal = SystemCall::<T>::remark { remark: vec![1; b as usize] }.into();
let proposal: T::Proposal = SystemCall::<T>::remark { remark: id_to_remark_data::<T, I>(1, b as usize) }.into();

}: _(SystemOrigin::Signed(caller), Box::new(proposal.clone()), bytes_in_storage)
verify {
Expand All @@ -135,7 +137,7 @@ benchmarks_instance_pallet! {

// This tests when execution would happen immediately after proposal
propose_execute {
let b in 1 .. MAX_BYTES;
let b in 2 .. MAX_BYTES;
let m in 1 .. T::MaxMembers::get();

let bytes_in_storage = b + size_of::<u32>() as u32;
Expand All @@ -152,7 +154,7 @@ benchmarks_instance_pallet! {

Collective::<T, I>::set_members(SystemOrigin::Root.into(), members, None, T::MaxMembers::get())?;

let proposal: T::Proposal = SystemCall::<T>::remark { remark: vec![1; b as usize] }.into();
let proposal: T::Proposal = SystemCall::<T>::remark { remark: id_to_remark_data::<T, I>(1, b as usize) }.into();
let threshold = 1;

}: propose(SystemOrigin::Signed(caller), threshold, Box::new(proposal.clone()), bytes_in_storage)
Expand All @@ -166,7 +168,7 @@ benchmarks_instance_pallet! {

// This tests when proposal is created and queued as "proposed"
propose_proposed {
let b in 1 .. MAX_BYTES;
let b in 2 .. MAX_BYTES;
let m in 2 .. T::MaxMembers::get();
let p in 1 .. T::MaxProposals::get();

Expand All @@ -186,7 +188,7 @@ benchmarks_instance_pallet! {
// Add previous proposals.
for i in 0 .. p - 1 {
// Proposals should be different so that different proposal hashes are generated
let proposal: T::Proposal = SystemCall::<T>::remark { remark: vec![i as u8; b as usize] }.into();
let proposal: T::Proposal = SystemCall::<T>::remark { remark: id_to_remark_data::<T, I>(i, b as usize) }.into();
Collective::<T, I>::propose(
SystemOrigin::Signed(caller.clone()).into(),
threshold,
Expand All @@ -197,7 +199,7 @@ benchmarks_instance_pallet! {

assert_eq!(Collective::<T, I>::proposals().len(), (p - 1) as usize);

let proposal: T::Proposal = SystemCall::<T>::remark { remark: vec![p as u8; b as usize] }.into();
let proposal: T::Proposal = SystemCall::<T>::remark { remark: id_to_remark_data::<T, I>(p, b as usize) }.into();

}: propose(SystemOrigin::Signed(caller.clone()), threshold, Box::new(proposal.clone()), bytes_in_storage)
verify {
Expand Down Expand Up @@ -234,7 +236,7 @@ benchmarks_instance_pallet! {
let mut last_hash = T::Hash::default();
for i in 0 .. p {
// Proposals should be different so that different proposal hashes are generated
let proposal: T::Proposal = SystemCall::<T>::remark { remark: vec![i as u8; b as usize] }.into();
let proposal: T::Proposal = SystemCall::<T>::remark { remark: id_to_remark_data::<T, I>(i, b as usize) }.into();
Collective::<T, I>::propose(
SystemOrigin::Signed(proposer.clone()).into(),
threshold,
Expand Down Expand Up @@ -309,9 +311,7 @@ benchmarks_instance_pallet! {
let mut last_hash = T::Hash::default();
for i in 0 .. p {
// Proposals should be different so that different proposal hashes are generated
let proposal: T::Proposal = SystemCall::<T>::remark {
remark: vec![i as u8; bytes as usize]
}.into();
let proposal: T::Proposal = SystemCall::<T>::remark { remark: id_to_remark_data::<T, I>(i, bytes as usize) }.into();
Collective::<T, I>::propose(
SystemOrigin::Signed(proposer.clone()).into(),
threshold,
Expand Down Expand Up @@ -364,7 +364,7 @@ benchmarks_instance_pallet! {
}

close_early_approved {
let b in 1 .. MAX_BYTES;
let b in 2 .. MAX_BYTES;
// We choose 4 as a minimum so we always trigger a vote in the voting loop (`for j in ...`)
let m in 4 .. T::MaxMembers::get();
let p in 1 .. T::MaxProposals::get();
Expand All @@ -388,7 +388,7 @@ benchmarks_instance_pallet! {
let mut last_hash = T::Hash::default();
for i in 0 .. p {
// Proposals should be different so that different proposal hashes are generated
let proposal: T::Proposal = SystemCall::<T>::remark { remark: vec![i as u8; b as usize] }.into();
let proposal: T::Proposal = SystemCall::<T>::remark { remark: id_to_remark_data::<T, I>(i, b as usize) }.into();
Collective::<T, I>::propose(
SystemOrigin::Signed(caller.clone()).into(),
threshold,
Expand Down Expand Up @@ -474,9 +474,7 @@ benchmarks_instance_pallet! {
let mut last_hash = T::Hash::default();
for i in 0 .. p {
// Proposals should be different so that different proposal hashes are generated
let proposal: T::Proposal = SystemCall::<T>::remark {
remark: vec![i as u8; bytes as usize]
}.into();
let proposal: T::Proposal = SystemCall::<T>::remark { remark: id_to_remark_data::<T, I>(i, bytes as usize) }.into();
Collective::<T, I>::propose(
SystemOrigin::Signed(caller.clone()).into(),
threshold,
Expand Down Expand Up @@ -519,7 +517,7 @@ benchmarks_instance_pallet! {
}

close_approved {
let b in 1 .. MAX_BYTES;
let b in 2 .. MAX_BYTES;
Copy link
Member

@bkchr bkchr Nov 14, 2022

Choose a reason for hiding this comment

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

Why these changes?

Copy link
Contributor Author

@Szegoo Szegoo Nov 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The goal of this PR was to be able to set the MaxProposal to a value higher than 255. In the benchmarks, we use the remark function to represent a proposal. Because we cannot have two identical proposals what we did until now is make the remark data be a variable number repeated a number of times(b times). This comes with a limit since the remark data is a Vec<u8> so it cannot accept a number greater than 255. To fix this we use little-endian to encode these greater numbers. So to represent a number greater than 255 we need a vector that is longer than 1. That is why we updated the range of b so that it starts from 2. @bkchr

I hope the explanation makes sense :)

Copy link
Member

@bkchr bkchr Nov 14, 2022

Choose a reason for hiding this comment

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

Then it should be changed to 4, because MaxMembers is an u32 which would lead to duplicate proposals when using more than 65k max members. You should also leave some comment on why we start at 4.

Copy link
Contributor Author

@Szegoo Szegoo Nov 14, 2022

Choose a reason for hiding this comment

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

#12454 (comment)

Copy link
Member

@bkchr bkchr Nov 14, 2022

Choose a reason for hiding this comment

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

Hmm okay 🙈

Copy link
Member

@bkchr bkchr Nov 14, 2022

Choose a reason for hiding this comment

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

I'm not happy with this, but okay :P

// We choose 4 as a minimum so we always trigger a vote in the voting loop (`for j in ...`)
let m in 4 .. T::MaxMembers::get();
let p in 1 .. T::MaxProposals::get();
Expand Down Expand Up @@ -548,7 +546,7 @@ benchmarks_instance_pallet! {
let mut last_hash = T::Hash::default();
for i in 0 .. p {
// Proposals should be different so that different proposal hashes are generated
let proposal: T::Proposal = SystemCall::<T>::remark { remark: vec![i as u8; b as usize] }.into();
let proposal: T::Proposal = SystemCall::<T>::remark { remark: id_to_remark_data::<T, I>(i, b as usize) }.into();
Collective::<T, I>::propose(
SystemOrigin::Signed(caller.clone()).into(),
threshold,
Expand Down Expand Up @@ -619,7 +617,7 @@ benchmarks_instance_pallet! {
let mut last_hash = T::Hash::default();
for i in 0 .. p {
// Proposals should be different so that different proposal hashes are generated
let proposal: T::Proposal = SystemCall::<T>::remark { remark: vec![i as u8; b as usize] }.into();
let proposal: T::Proposal = SystemCall::<T>::remark { remark: id_to_remark_data::<T, I>(i, b as usize) }.into();
Collective::<T, I>::propose(
SystemOrigin::Signed(caller.clone()).into(),
threshold,
Expand Down
2 changes: 1 addition & 1 deletion frame/collective/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ pub type MaxMembers = ConstU32<100>;

parameter_types! {
pub const MotionDuration: u64 = 3;
pub const MaxProposals: u32 = 100;
pub const MaxProposals: u32 = 512;
pub BlockWeights: frame_system::limits::BlockWeights =
frame_system::limits::BlockWeights::simple_max(frame_support::weights::Weight::from_ref_time(1024));
}
Expand Down