Browse Source

Merge pull request #1691 from conectado/proposals_update

Benchmarking `proposals/discussion`
shamil-gadelshin 4 years ago
parent
commit
ed9302a4c4

+ 11 - 3
Cargo.lock

@@ -1690,6 +1690,12 @@ dependencies = [
  "proc-macro-hack",
 ]
 
+[[package]]
+name = "hex-literal"
+version = "0.3.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "5af1f635ef1bc545d78392b136bfe1c9809e029023c84a3638a864a10b8819c8"
+
 [[package]]
 name = "hex-literal-impl"
 version = "0.2.2"
@@ -2061,6 +2067,7 @@ dependencies = [
  "frame-system",
  "frame-system-benchmarking",
  "frame-system-rpc-runtime-api",
+ "hex-literal 0.3.1",
  "pallet-authority-discovery",
  "pallet-authorship",
  "pallet-babe",
@@ -3573,8 +3580,9 @@ dependencies = [
 
 [[package]]
 name = "pallet-proposals-discussion"
-version = "3.0.0"
+version = "3.0.1"
 dependencies = [
+ "frame-benchmarking",
  "frame-support",
  "frame-system",
  "pallet-balances",
@@ -5077,7 +5085,7 @@ dependencies = [
  "fnv",
  "futures 0.3.4",
  "hash-db",
- "hex-literal",
+ "hex-literal 0.2.1",
  "kvdb",
  "lazy_static",
  "log",
@@ -5687,7 +5695,7 @@ dependencies = [
  "fdlimit",
  "futures 0.1.29",
  "futures 0.3.4",
- "hex-literal",
+ "hex-literal 0.2.1",
  "log",
  "parity-scale-codec",
  "parking_lot 0.10.2",

+ 4 - 2
runtime-modules/proposals/discussion/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = 'pallet-proposals-discussion'
-version = '3.0.0'
+version = '3.0.1'
 authors = ['Joystream contributors']
 edition = '2018'
 
@@ -12,6 +12,7 @@ frame-support = { package = 'frame-support', default-features = false, git = 'ht
 system = { package = 'frame-system', default-features = false, git = 'https://github.com/paritytech/substrate.git', rev = '00768a1f21a579c478fe5d4f51e1fa71f7db9fd4'}
 membership = { package = 'pallet-membership', default-features = false, path = '../../membership'}
 common = { package = 'pallet-common', default-features = false, path = '../../common'}
+frame-benchmarking = { package = 'frame-benchmarking', default-features = false, git = 'https://github.com/paritytech/substrate.git', rev = '00768a1f21a579c478fe5d4f51e1fa71f7db9fd4', optional = true}
 
 [dev-dependencies]
 sp-io = { package = 'sp-io', default-features = false, git = 'https://github.com/paritytech/substrate.git', rev = '00768a1f21a579c478fe5d4f51e1fa71f7db9fd4'}
@@ -22,6 +23,7 @@ balances = { package = 'pallet-balances', default-features = false, git = 'https
 
 [features]
 default = ['std']
+runtime-benchmarks = ['frame-benchmarking']
 std = [
 	'serde',
 	'codec/std',
@@ -30,4 +32,4 @@ std = [
 	'system/std',
     'membership/std',
     'common/std',
-]
+]

+ 222 - 0
runtime-modules/proposals/discussion/src/benchmarking.rs

@@ -0,0 +1,222 @@
+#![cfg(feature = "runtime-benchmarks")]
+use super::*;
+use crate::Module as ProposalsDiscussion;
+use core::convert::TryInto;
+use frame_benchmarking::{account, benchmarks};
+use membership::Module as Membership;
+use sp_std::cmp::min;
+use sp_std::prelude::*;
+use system as frame_system;
+use system::EventRecord;
+use system::Module as System;
+use system::RawOrigin;
+
+const SEED: u32 = 0;
+
+fn get_byte(num: u32, byte_number: u8) -> u8 {
+    ((num & (0xff << (8 * byte_number))) >> 8 * byte_number) as u8
+}
+
+// Method to generate a distintic valid handle
+// for a membership. For each index.
+fn handle_from_id<T: membership::Trait>(id: u32) -> Vec<u8> {
+    let min_handle_length = Membership::<T>::min_handle_length();
+
+    let mut handle = vec![];
+
+    for i in 0..min(Membership::<T>::max_handle_length().try_into().unwrap(), 4) {
+        handle.push(get_byte(id, i));
+    }
+
+    while handle.len() < (min_handle_length as usize) {
+        handle.push(0u8);
+    }
+
+    handle
+}
+
+fn assert_last_event<T: Trait>(generic_event: <T as Trait>::Event) {
+    let events = System::<T>::events();
+    let system_event: <T as frame_system::Trait>::Event = generic_event.into();
+    // compare to the last event record
+    let EventRecord { event, .. } = &events[events.len() - 1];
+    assert_eq!(event, &system_event);
+}
+
+fn member_account<T: membership::Trait>(
+    name: &'static str,
+    id: u32,
+) -> (T::AccountId, T::MemberId) {
+    let account_id = account::<T::AccountId>(name, id, SEED);
+    let handle = handle_from_id::<T>(id);
+
+    let authority_account = account::<T::AccountId>(name, 0, SEED);
+
+    Membership::<T>::set_screening_authority(RawOrigin::Root.into(), authority_account.clone())
+        .unwrap();
+
+    Membership::<T>::add_screened_member(
+        RawOrigin::Signed(authority_account.clone()).into(),
+        account_id.clone(),
+        Some(handle),
+        None,
+        None,
+    )
+    .unwrap();
+
+    (account_id, T::MemberId::from(id.try_into().unwrap()))
+}
+
+const MAX_BYTES: u32 = 16384;
+
+benchmarks! {
+    _ { }
+
+    add_post {
+        let i in 1 .. T::MaxWhiteListSize::get();
+
+        let j in 0 .. MAX_BYTES;
+
+        // We do this to ignore the id 0 because the `Test` runtime
+        // returns 0 as an invalid id but 1 as a valid one
+        let (_, _) = member_account::<T>("member", 0);
+        let (account_id, caller_member_id) = member_account::<T>("caller_member", 1);
+
+        let mut whitelisted_members = vec![caller_member_id];
+
+        // We start from 2 since we have previously created id 0 and not used it
+        // and used id 1 for the caller (see comment above)
+        for id in 2 .. i + 1 {
+            let (_, member_id) = member_account::<T>("member", id);
+            whitelisted_members.push(member_id);
+        }
+
+        let thread_id = ProposalsDiscussion::<T>::create_thread(
+            caller_member_id,
+            ThreadMode::Closed(whitelisted_members)
+        ).unwrap();
+
+        assert!(ThreadById::<T>::contains_key(thread_id), "Thread not created");
+
+        let text = vec![0u8; j.try_into().unwrap()];
+
+    }: _ (RawOrigin::Signed(account_id), caller_member_id, thread_id, text)
+    verify {
+        let post_id = T::PostId::from(1);
+
+        assert!(PostThreadIdByPostId::<T>::contains_key(thread_id, post_id), "Post not created");
+        assert_eq!(
+            PostThreadIdByPostId::<T>::get(thread_id, post_id).author_id,
+            caller_member_id,
+            "Post author isn't correct"
+        );
+
+        assert_last_event::<T>(RawEvent::PostCreated(post_id, caller_member_id).into());
+    }
+
+    update_post {
+        // TODO: this parameter doesn't affect the running time
+        // maybe we should bound it here with the UI limit?
+        let j in 0 .. MAX_BYTES;
+
+        // We do this to ignore the id 0 because the `Test` runtime
+        // returns 0 as an invalid id but 1 as a valid one
+        let (_, _) = member_account::<T>("caller_member", 0);
+        let (account_id, caller_member_id) = member_account::<T>("caller_member", 1);
+
+        let thread_id = ProposalsDiscussion::<T>::create_thread(
+            caller_member_id,
+            ThreadMode::Open
+        ).unwrap();
+
+        assert!(ThreadById::<T>::contains_key(thread_id), "Thread not created");
+
+        ProposalsDiscussion::<T>::add_post(
+            RawOrigin::Signed(account_id.clone()).into(),
+            caller_member_id,
+            thread_id,
+            vec![0u8]
+        ).unwrap();
+
+        let post_id = T::PostId::from(1);
+
+        assert!(PostThreadIdByPostId::<T>::contains_key(thread_id, post_id), "Post not created");
+
+        let new_text = vec![0u8; j.try_into().unwrap()];
+    }: _ (RawOrigin::Signed(account_id), caller_member_id, thread_id, post_id, new_text)
+    verify {
+        assert_last_event::<T>(RawEvent::PostUpdated(post_id, caller_member_id).into());
+    }
+
+    // TODO: Review this after changes to the governance/council are merged:
+    // this extrinsic uses `T::CouncilOriginValidator::ensure_actor_origin`
+    // this is a hook to the runtime. Since the pallet implementation shouldn't have any
+    // information on the runtime this hooks should be constant.
+    // However, the implementation in the runtime is linear in the number of council members.
+    // But since the size of the council should be completely filled over time we could
+    // always use the worst case scenario, still this would require to create an artificial
+    // dependency with the `governance` pallet to correctly benchmark this.
+    change_thread_mode {
+        let i in 1 .. T::MaxWhiteListSize::get();
+
+        // We do this to ignore the id 0 because the `Test` runtime
+        // returns 0 as an invalid id but 1 as a valid one
+        let (_, _) = member_account::<T>("member", 0);
+        let (account_id, caller_member_id) = member_account::<T>("caller_member", 1);
+
+        let thread_id = ProposalsDiscussion::<T>::create_thread(
+            caller_member_id,
+            ThreadMode::Open
+        ).unwrap();
+
+        assert!(ThreadById::<T>::contains_key(thread_id), "Thread not created");
+
+        let mut whitelisted_members = vec![caller_member_id];
+
+        // We start from 2 since we have previously created id 0 and not used it
+        // and used id 1 for the caller (see comment above)
+        for id in 2 .. i + 1 {
+            let (_, member_id) = member_account::<T>("member", id);
+            whitelisted_members.push(member_id);
+        }
+
+        let mode = ThreadMode::Closed(whitelisted_members);
+    }: _ (RawOrigin::Signed(account_id), caller_member_id, thread_id, mode.clone())
+    verify {
+        assert_eq!(
+            ProposalsDiscussion::<T>::thread_by_id(thread_id).mode,
+            mode.clone(),
+            "Thread not correctly updated"
+        );
+
+        assert_last_event::<T>(RawEvent::ThreadModeChanged(thread_id, mode).into());
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::tests::{initial_test_ext, Test};
+    use frame_support::assert_ok;
+
+    #[test]
+    fn test_add_post() {
+        initial_test_ext().execute_with(|| {
+            assert_ok!(test_benchmark_add_post::<Test>());
+        });
+    }
+
+    #[test]
+    fn test_update_post() {
+        initial_test_ext().execute_with(|| {
+            assert_ok!(test_benchmark_update_post::<Test>());
+        });
+    }
+
+    #[test]
+    fn test_change_thread_mode() {
+        initial_test_ext().execute_with(|| {
+            assert_ok!(test_benchmark_change_thread_mode::<Test>());
+        });
+    }
+}

+ 1 - 0
runtime-modules/proposals/discussion/src/lib.rs

@@ -45,6 +45,7 @@
 // Do not delete! Cannot be uncommented by default, because of Parity decl_module! issue.
 //#![warn(missing_docs)]
 
+mod benchmarking;
 #[cfg(test)]
 mod tests;
 mod types;

+ 2 - 1
runtime-modules/proposals/discussion/src/tests/mod.rs

@@ -5,7 +5,8 @@ use system::RawOrigin;
 use system::{EventRecord, Phase};
 
 use crate::*;
-use mock::*;
+
+pub(crate) use mock::*;
 
 struct EventFixture;
 impl EventFixture {

+ 3 - 0
runtime/Cargo.toml

@@ -10,6 +10,7 @@ version = '7.6.0'
 # Third-party dependencies
 serde = { version = "1.0.101", optional = true, features = ["derive"] }
 codec = { package = 'parity-scale-codec', version = '1.3.1', default-features = false, features = ['derive'] }
+hex-literal = { optional = true, version = '0.3.1' }
 
 # Substrate primitives
 sp-std = { package = 'sp-std', default-features = false, git = 'https://github.com/paritytech/substrate.git', rev = '00768a1f21a579c478fe5d4f51e1fa71f7db9fd4'}
@@ -173,6 +174,8 @@ runtime-benchmarks = [
     "pallet-offences-benchmarking",
 	"pallet-session-benchmarking",
     "pallet-utility/runtime-benchmarks",
+    "proposals-discussion/runtime-benchmarks",
+    "hex-literal",
 ]
 
 

+ 48 - 0
runtime/src/runtime_api.rs

@@ -201,4 +201,52 @@ impl_runtime_apis! {
             SessionKeys::decode_into_raw_public_keys(&encoded)
         }
     }
+
+     #[cfg(feature = "runtime-benchmarks")]
+    impl frame_benchmarking::Benchmark<Block> for Runtime {
+        fn dispatch_benchmark(
+            pallet: Vec<u8>,
+            benchmark: Vec<u8>,
+            lowest_range_values: Vec<u32>,
+            highest_range_values: Vec<u32>,
+            steps: Vec<u32>,
+            repeat: u32,
+        ) -> Result<Vec<frame_benchmarking::BenchmarkBatch>, sp_runtime::RuntimeString> {
+            /*
+             * TODO: remember to benchhmark every pallet
+             */
+            use sp_std::vec;
+            use frame_benchmarking::{Benchmarking, BenchmarkBatch, add_benchmark};
+            use frame_system_benchmarking::Module as SystemBench;
+            impl frame_system_benchmarking::Trait for Runtime {}
+
+            use crate::ProposalsDiscussion;
+
+            let whitelist: Vec<Vec<u8>> = vec![
+                // Block Number
+                hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef702a5c1b19ab7a04f536c519aca4983ac").to_vec(),
+                // Total Issuance
+                hex_literal::hex!("c2261276cc9d1f8598ea4b6a74b15c2f57c875e4cff74148e4628f264b974c80").to_vec(),
+                // Execution Phase
+                hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef7ff553b5a9862a516939d82b3d3d8661a").to_vec(),
+                // Event Count
+                hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef70a98fdbe9ce6c55837576c60c7af3850").to_vec(),
+                // System Events
+                hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef780d41e5e16056765bc8461851072c9d7").to_vec(),
+                // Caller 0 Account
+                hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef7b99d880ec681799c0cf30e8886371da946c154ffd9992e395af90b5b13cc6f295c77033fce8a9045824a6690bbf99c6db269502f0a8d1d2a008542d5690a0749").to_vec(),
+                // Treasury Account
+                hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef7b99d880ec681799c0cf30e8886371da95ecffd7b6c0f78751baa9d281e0bfa3a6d6f646c70792f74727372790000000000000000000000000000000000000000").to_vec(),
+            ];
+
+            let mut batches = Vec::<BenchmarkBatch>::new();
+            let params = (&pallet, &benchmark, &lowest_range_values, &highest_range_values, &steps, repeat, &whitelist);
+
+            add_benchmark!(params, batches, b"system", SystemBench::<Runtime>);
+            add_benchmark!(params, batches, b"proposals-discussion", ProposalsDiscussion);
+
+            if batches.is_empty() { return Err("Benchmark not found for this pallet.".into()) }
+            Ok(batches)
+        }
+    }
 }