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

Optimize calls for utility and lottery #9339

Closed
gui1117 wants to merge 10 commits into from
Closed

Optimize calls for utility and lottery #9339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4f63cb0
optimize calls
gui1117 Jul 13, 2021
6603341
fix benchmarks
gui1117 Jul 13, 2021
f8ed16e
Merge branch 'master' of https://github.com/paritytech/substrate into…
Jul 13, 2021
ec93539
cargo run --release --features=runtime-benchmarks --manifest-path=bin…
Jul 13, 2021
0510139
cargo run --release --features=runtime-benchmarks --manifest-path=bin…
Jul 13, 2021
8bc21dd
fix tests
gui1117 Jul 13, 2021
5a93791
improve benchmarks
gui1117 Jul 13, 2021
40f6df7
Merge branch 'master' of https://github.com/paritytech/substrate into…
Jul 13, 2021
ee9aabf
cargo run --release --features=runtime-benchmarks --manifest-path=bin…
Jul 13, 2021
2832e55
cargo run --release --features=runtime-benchmarks --manifest-path=bin…
Jul 13, 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
2 changes: 1 addition & 1 deletion frame/contracts/src/exec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2575,7 +2575,7 @@ mod tests {
// as part of a patch: return is OK (but it interrupted the batch)
assert_ok!(
ctx.ext.call_runtime(Call::Utility(UtilCall::batch(vec![
allowed_call.clone(), forbidden_call, allowed_call
Box::new(allowed_call.clone()), Box::new(forbidden_call), Box::new(allowed_call)
]))),
);

Expand Down
12 changes: 8 additions & 4 deletions frame/lottery/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@ fn setup_lottery<T: Config>(repeat: bool) -> Result<(), &'static str> {
let delay = 5u32.into();
// Calls will be maximum length...
let mut calls = vec![
frame_system::Call::<T>::set_code(vec![]).into();
Box::new(frame_system::Call::<T>::set_code(vec![]).into());
T::MaxCalls::get().saturating_sub(1) as usize
];
// Last call will be the match for worst case scenario.
calls.push(frame_system::Call::<T>::remark(vec![]).into());
calls.push(Box::new(frame_system::Call::<T>::remark(vec![]).into()));
let origin = T::ManagerOrigin::successful_origin();
Lottery::<T>::set_calls(origin.clone(), calls)?;
Lottery::<T>::start_lottery(origin, price, length, delay, repeat)?;
Expand Down Expand Up @@ -72,12 +72,16 @@ benchmarks! {

set_calls {
let n in 0 .. T::MaxCalls::get() as u32;
let calls = vec![frame_system::Call::<T>::remark(vec![]).into(); n as usize];
let calls = vec![Box::new(frame_system::Call::<T>::remark(vec![]).into()); n as usize];

let call = Call::<T>::set_calls(calls);
let origin = T::ManagerOrigin::successful_origin();
assert!(CallIndices::<T>::get().is_empty());
}: { call.dispatch_bypass_filter(origin)? }
let call = call.encode();
}: {
let call = <Call<T>>::decode(&mut &call[..]).expect("encoding is valid");
call.dispatch_bypass_filter(origin)?;
}
verify {
if !n.is_zero() {
assert!(!CallIndices::<T>::get().is_empty());
Expand Down
4 changes: 2 additions & 2 deletions frame/lottery/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ pub mod pallet {
///
/// This extrinsic must be called by the Manager origin.
#[pallet::weight(T::WeightInfo::set_calls(calls.len() as u32))]
pub fn set_calls(origin: OriginFor<T>, calls: Vec<<T as Config>::Call>) -> DispatchResult {
pub fn set_calls(origin: OriginFor<T>, calls: Vec<Box<<T as Config>::Call>>) -> DispatchResult {
T::ManagerOrigin::ensure_origin(origin)?;
ensure!(calls.len() <= T::MaxCalls::get() as usize, Error::<T>::TooManyCalls);
if calls.is_empty() {
Expand Down Expand Up @@ -395,7 +395,7 @@ impl<T: Config> Pallet<T> {
}

// Converts a vector of calls into a vector of call indices.
fn calls_to_indices(calls: &[<T as Config>::Call]) -> Result<Vec<CallIndex>, DispatchError> {
fn calls_to_indices(calls: &[Box<<T as Config>::Call>]) -> Result<Vec<CallIndex>, DispatchError> {
let mut indices = Vec::with_capacity(calls.len());
for c in calls.iter() {
let index = Self::call_to_index(c)?;
Expand Down
22 changes: 11 additions & 11 deletions frame/lottery/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ fn basic_end_to_end_works() {
let length = 20;
let delay = 5;
let calls = vec![
Call::Balances(BalancesCall::force_transfer(0, 0, 0)),
Call::Balances(BalancesCall::transfer(0, 0)),
Box::new(Call::Balances(BalancesCall::force_transfer(0, 0, 0))),
Box::new(Call::Balances(BalancesCall::transfer(0, 0))),
];

// Set calls for the lottery
Expand Down Expand Up @@ -103,17 +103,17 @@ fn set_calls_works() {
assert!(!CallIndices::<Test>::exists());

let calls = vec![
Call::Balances(BalancesCall::force_transfer(0, 0, 0)),
Call::Balances(BalancesCall::transfer(0, 0)),
Box::new(Call::Balances(BalancesCall::force_transfer(0, 0, 0))),
Box::new(Call::Balances(BalancesCall::transfer(0, 0))),
];

assert_ok!(Lottery::set_calls(Origin::root(), calls));
assert!(CallIndices::<Test>::exists());

let too_many_calls = vec![
Call::Balances(BalancesCall::force_transfer(0, 0, 0)),
Call::Balances(BalancesCall::transfer(0, 0)),
Call::System(SystemCall::remark(vec![])),
Box::new(Call::Balances(BalancesCall::force_transfer(0, 0, 0))),
Box::new(Call::Balances(BalancesCall::transfer(0, 0))),
Box::new(Call::System(SystemCall::remark(vec![]))),
];

assert_noop!(
Expand Down Expand Up @@ -165,8 +165,8 @@ fn buy_ticket_works_as_simple_passthrough() {

// Lottery is set up, but too expensive to enter, so `do_buy_ticket` fails.
let calls = vec![
Call::Balances(BalancesCall::force_transfer(0, 0, 0)),
Call::Balances(BalancesCall::transfer(0, 0)),
Box::new(Call::Balances(BalancesCall::force_transfer(0, 0, 0))),
Box::new(Call::Balances(BalancesCall::transfer(0, 0))),
];
assert_ok!(Lottery::set_calls(Origin::root(), calls));

Expand Down Expand Up @@ -205,8 +205,8 @@ fn buy_ticket_works() {
new_test_ext().execute_with(|| {
// Set calls for the lottery.
let calls = vec![
Call::System(SystemCall::remark(vec![])),
Call::Balances(BalancesCall::transfer(0, 0)),
Box::new(Call::System(SystemCall::remark(vec![]))),
Box::new(Call::Balances(BalancesCall::transfer(0, 0))),
];
assert_ok!(Lottery::set_calls(Origin::root(), calls));

Expand Down
32 changes: 16 additions & 16 deletions frame/lottery/src/weights.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@

//! Autogenerated weights for pallet_lottery
//!
//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 3.0.0
//! DATE: 2021-06-19, STEPS: `[50, ]`, REPEAT: 20, LOW RANGE: `[]`, HIGH RANGE: `[]`
//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev
//! DATE: 2021-07-13, STEPS: `[50, ]`, REPEAT: 20, LOW RANGE: `[]`, HIGH RANGE: `[]`
//! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled, CHAIN: Some("dev"), DB CACHE: 128

// Executed Command:
Expand Down Expand Up @@ -56,33 +56,33 @@ pub trait WeightInfo {
pub struct SubstrateWeight<T>(PhantomData<T>);
impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
fn buy_ticket() -> Weight {
(71_604_000 as Weight)
(72_703_000 as Weight)
.saturating_add(T::DbWeight::get().reads(6 as Weight))
.saturating_add(T::DbWeight::get().writes(4 as Weight))
}
fn set_calls(n: u32, ) -> Weight {
(15_015_000 as Weight)
(15_536_000 as Weight)
// Standard Error: 5_000
.saturating_add((301_000 as Weight).saturating_mul(n as Weight))
.saturating_add((786_000 as Weight).saturating_mul(n as Weight))
Copy link
Member

@shawntabrizi shawntabrizi Jul 13, 2021

Choose a reason for hiding this comment

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

This seems to be the difference with encoding? Double the weight per call

Copy link
Contributor Author

@gui1117 gui1117 Jul 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

we can't say for sure until we compare with the benchmark with the decoding but without the Box, but anyway this seems ok to me

comparing to the benchmark result in this PR #9343 it is indeed from the difference in decoding.

.saturating_add(T::DbWeight::get().writes(1 as Weight))
}
fn start_lottery() -> Weight {
(58_855_000 as Weight)
(57_937_000 as Weight)
.saturating_add(T::DbWeight::get().reads(3 as Weight))
.saturating_add(T::DbWeight::get().writes(3 as Weight))
}
fn stop_repeat() -> Weight {
(7_524_000 as Weight)
(7_468_000 as Weight)
.saturating_add(T::DbWeight::get().reads(1 as Weight))
.saturating_add(T::DbWeight::get().writes(1 as Weight))
}
fn on_initialize_end() -> Weight {
(114_766_000 as Weight)
(114_880_000 as Weight)
.saturating_add(T::DbWeight::get().reads(6 as Weight))
.saturating_add(T::DbWeight::get().writes(4 as Weight))
}
fn on_initialize_repeat() -> Weight {
(119_402_000 as Weight)
(119_713_000 as Weight)
.saturating_add(T::DbWeight::get().reads(7 as Weight))
.saturating_add(T::DbWeight::get().writes(5 as Weight))
}
Expand All @@ -91,33 +91,33 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
// For backwards compatibility and tests
impl WeightInfo for () {
fn buy_ticket() -> Weight {
(71_604_000 as Weight)
(72_703_000 as Weight)
.saturating_add(RocksDbWeight::get().reads(6 as Weight))
.saturating_add(RocksDbWeight::get().writes(4 as Weight))
}
fn set_calls(n: u32, ) -> Weight {
(15_015_000 as Weight)
(15_536_000 as Weight)
// Standard Error: 5_000
.saturating_add((301_000 as Weight).saturating_mul(n as Weight))
.saturating_add((786_000 as Weight).saturating_mul(n as Weight))
.saturating_add(RocksDbWeight::get().writes(1 as Weight))
}
fn start_lottery() -> Weight {
(58_855_000 as Weight)
(57_937_000 as Weight)
.saturating_add(RocksDbWeight::get().reads(3 as Weight))
.saturating_add(RocksDbWeight::get().writes(3 as Weight))
}
fn stop_repeat() -> Weight {
(7_524_000 as Weight)
(7_468_000 as Weight)
.saturating_add(RocksDbWeight::get().reads(1 as Weight))
.saturating_add(RocksDbWeight::get().writes(1 as Weight))
}
fn on_initialize_end() -> Weight {
(114_766_000 as Weight)
(114_880_000 as Weight)
.saturating_add(RocksDbWeight::get().reads(6 as Weight))
.saturating_add(RocksDbWeight::get().writes(4 as Weight))
}
fn on_initialize_repeat() -> Weight {
(119_402_000 as Weight)
(119_713_000 as Weight)
.saturating_add(RocksDbWeight::get().reads(7 as Weight))
.saturating_add(RocksDbWeight::get().writes(5 as Weight))
}
Expand Down
4 changes: 2 additions & 2 deletions frame/proxy/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ fn filtering_works() {
assert_ok!(Proxy::proxy(Origin::signed(4), 1, None, call.clone()));
System::assert_last_event(ProxyEvent::ProxyExecuted(Err(DispatchError::BadOrigin)).into());

let call = Box::new(Call::Utility(UtilityCall::batch(vec![*inner])));
let call = Box::new(Call::Utility(UtilityCall::batch(vec![inner])));
assert_ok!(Proxy::proxy(Origin::signed(2), 1, None, call.clone()));
expect_events(vec![UtilityEvent::BatchCompleted.into(), ProxyEvent::ProxyExecuted(Ok(())).into()]);
assert_ok!(Proxy::proxy(Origin::signed(3), 1, None, call.clone()));
Expand All @@ -340,7 +340,7 @@ fn filtering_works() {
]);

let inner = Box::new(Call::Proxy(ProxyCall::add_proxy(5, ProxyType::Any, 0)));
let call = Box::new(Call::Utility(UtilityCall::batch(vec![*inner])));
let call = Box::new(Call::Utility(UtilityCall::batch(vec![inner])));
assert_ok!(Proxy::proxy(Origin::signed(2), 1, None, call.clone()));
expect_events(vec![UtilityEvent::BatchCompleted.into(), ProxyEvent::ProxyExecuted(Ok(())).into()]);
assert_ok!(Proxy::proxy(Origin::signed(3), 1, None, call.clone()));
Expand Down
1 change: 1 addition & 0 deletions frame/utility/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ std = [
]
runtime-benchmarks = [
"frame-benchmarking",
"frame-system/runtime-benchmarks",
"frame-support/runtime-benchmarks",
]
try-runtime = ["frame-support/try-runtime"]
22 changes: 14 additions & 8 deletions frame/utility/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,16 @@ fn assert_last_event<T: Config>(generic_event: <T as Config>::Event) {
benchmarks! {
batch {
let c in 0 .. 1000;
let mut calls: Vec<<T as Config>::Call> = Vec::new();
let mut calls = Vec::new();
for i in 0 .. c {
let call = frame_system::Call::remark(vec![]).into();
calls.push(call);
calls.push(Box::new(frame_system::Call::remark(vec![]).into()));
}
let caller = whitelisted_caller();
}: _(RawOrigin::Signed(caller), calls)
let call = <Call<T>>::batch(calls).encode();
}: {
let call = <Call<T>>::decode(&mut &call[..]).expect("encoding is valid");
call.dispatch_bypass_filter(RawOrigin::Signed(caller).into())?;
}
verify {
assert_last_event::<T>(Event::BatchCompleted.into())
}
Expand All @@ -53,13 +56,16 @@ benchmarks! {

batch_all {
let c in 0 .. 1000;
let mut calls: Vec<<T as Config>::Call> = Vec::new();
let mut calls = Vec::new();
for i in 0 .. c {
let call = frame_system::Call::remark(vec![]).into();
calls.push(call);
calls.push(Box::new(frame_system::Call::remark(vec![]).into()));
}
let caller = whitelisted_caller();
}: _(RawOrigin::Signed(caller), calls)
let call = <Call<T>>::batch_all(calls).encode();
}: {
let call = <Call<T>>::decode(&mut &call[..]).expect("encoding is valid");
call.dispatch_bypass_filter(RawOrigin::Signed(caller).into())?;
}
verify {
assert_last_event::<T>(Event::BatchCompleted.into())
}
Expand Down
4 changes: 2 additions & 2 deletions frame/utility/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ pub mod pallet {
})]
pub fn batch(
origin: OriginFor<T>,
calls: Vec<<T as Config>::Call>,
calls: Vec<Box<<T as Config>::Call>>,
Copy link
Member

@bkchr bkchr Jul 19, 2021

Choose a reason for hiding this comment

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

This doesn't make any sense. When you use a Vec, these values are already "boxed". What you want to achieve here?

Copy link
Contributor Author

@gui1117 gui1117 Jul 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

inside a vec, the array of element is boxed, not each element individually.
because Call is around 400B, for a Vec<Call> of capacity 10 allocate an array of size 10*400 B.
whereas a Vec<Box<Call>> of capacity 10 allocate an array of size 10*size_of<usize>() B and also allocate 10 individual calls.
This way the maximum length of an individual allocation is minimized.

Copy link
Member

@bkchr bkchr Jul 19, 2021

Choose a reason for hiding this comment

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

Yeah that is right, but what is the reason for doing this?

We will not have a call vec with 83886 calls anyway?

) -> DispatchResultWithPostInfo {
let is_root = ensure_root(origin.clone()).is_ok();
let calls_len = calls.len();
Expand Down Expand Up @@ -256,7 +256,7 @@ pub mod pallet {
#[transactional]
pub fn batch_all(
origin: OriginFor<T>,
calls: Vec<<T as Config>::Call>,
calls: Vec<Box<<T as Config>::Call>>,
) -> DispatchResultWithPostInfo {
let is_root = ensure_root(origin.clone()).is_ok();
let calls_len = calls.len();
Expand Down
Loading