improve runtime call, and its usage #9385

gui1117 · 2021-07-19T15:55:46Z

This reduce the size of runtime call to be less than 90.
It also improve its usage in frame-utility.

gui1117 · 2021-07-19T18:49:10Z

thinking again maybe we should introduce something similar to pallet::compact but to box the given arg when generating the call: #[pallet::box]. But I'm not sure it worth complexifying the pallet declaration.

shawntabrizi · 2021-07-19T19:16:43Z

frame/utility/src/lib.rs

+	impl<T: Config> Pallet<T> {
+		/// The limit on the number of batched calls.
+		fn batched_calls_limit() -> u32 {
+			let allocator_limit = 33554432;


Suggested change

let allocator_limit = 33554432;

let allocator_limit = 33554432;

feels this number should come from somewhere else?

indeed but we can't currently link to it because it is in the client.
Maybe the proper solution is to have a configuratble limit: type MaxBatched: Get<u32>, or move the allocator limit to the primitives

maybe better to set a hardcoded limit of 10_000 and then assert this in a test

I think it is safe to merge like this because the allocator limit should not decrease, that said I opened an issue #9392 and a PR #9393
once merge I'll update this code as well.

bin/node/runtime/src/lib.rs

kianenigma · 2021-07-20T08:09:19Z

frame/balances/src/lib.rs

 			origin: OriginFor<T>,
-			source: <T::Lookup as StaticLookup>::Source,
-			dest: <T::Lookup as StaticLookup>::Source,
+			source: Box<<T::Lookup as StaticLookup>::Source>,


curious: what is the static size of this? a bit surprised that it is part of the offending ones.

it is 40 bytes if I remember correctly, note that an account id is already 32 bytes

kianenigma

Didn't check all, but looks mostly good to me.

Would be good if we add some guidelines around this. Maybe you can compile a pallet with PRINT_CALL_SIZE=1 or similar, and it would report the call size.

This is something that needs to be communicated to devhub IMO.

athei · 2021-07-20T08:23:06Z

frame/contracts/src/lib.rs

+			code: Box<Vec<u8>>,
+			data: Box<Vec<u8>>,
+			salt: Box<Vec<u8>>,


Why do we box Vec? It already allocates its elements on the heap. There are more places where this happened. I just mark this one as example.

I was surprised too but a vec is 24 bytes https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=076c59fc288d9fe6bb17e1c7c92e35a7

so this extrinsic variant fields are: 16 + 8 + 24 * 3 added to the variant indices: 1 for pallet variant, 1 for call variant. It is 98 bytes

Ohh. I expected it to be much smaller. Then go ahead.

it is heap in both cases, but it is more with Vec https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=9f93db0b5c42a9ece4f8658a34d821d0

gui1117 · 2021-07-20T09:01:07Z

maybe better than this we can just box each pallets call.

pub enum RuntimeCall {
    PalletCall(Box<PalletCall>),
...
}

So that if some pallet call have big size it is not shared with all other pallet calls

athei · 2021-07-20T09:02:23Z

maybe better than this we can just box each pallets call.
pub enum RuntimeCall {
    PalletCall(Box<PalletCall>),
...
}
So that if some pallet call have big size it is not shared with all other pallet calls

I was about to make exactly this proposition but was unsure if it is possible with those macros. It makes the code really ugly to have Boxes everywhere. Would love to see it implemented in this way transparently for the pallets. Also this would be way less allocation pressure, right?

kianenigma · 2021-07-20T10:00:34Z

But the downside of that is that even for small data that are more efficiently managed on the stack, you force them to be on the heap. I recall that in another PR benchmarked showed 100% performance hit (twice larger weight) in boxing stuff.

I (unfortunately) think that manual boxing is better to begin with, and over time we improve it if we learn that a certain pattern is best.

and again I want to emphasize that some tool to print the size of call enum variants is very important.

athei · 2021-07-20T10:12:01Z

But the downside of that is that even for small data that are more efficiently managed on the stack, you force them to be on the heap. I recall that in another PR benchmarked showed 100% performance hit (twice larger weight) in boxing stuff.

You mean for dispatchables which have small parameters? Yeah that is an obvious drawback.

But maybe we can just make this more granular? Instead of boxing per pallet or per argument, boxing per dispatchable (you will opt-in with an attribute) might be the sweet spot?

enum RuntimeCall {
    CoolPallet(CoolPallet),
}

enum CoolPallet {
    BigDispatchable(Box<(BigArg, EvenBiggerArg)>),
    SmallDispatchable(u32),
}

mod cool_pallet {
    #[weight(1000), box_args]
    fn big_dispatchable(big_arg: BigArg, even_bigger_arg: EvenBiggerArg) {}

    #[weight(1000)]
    fn small_dispatchable(small_arg: u32) {}
}

gavofyork · 2021-07-20T10:13:53Z

frame/assets/src/tests.rs

 		assert_eq!(Asset::<Test>::get(0).unwrap().approvals, 1);
 		assert_eq!(Balances::reserved_balance(&1), 1);
-		assert_ok!(Assets::transfer_approved(Origin::signed(2), 0, 1, 3, 40));
+		assert_ok!(Assets::transfer_approved(Origin::signed(2), 0, Box::new(1), Box::new(3), 40));


this is a bit sad.

bkchr · 2021-07-20T10:19:56Z

maybe better than this we can just box each pallets call.
pub enum RuntimeCall {
    PalletCall(Box<PalletCall>),
...
}
So that if some pallet call have big size it is not shared with all other pallet calls

Yeah, I also thought that we should do this as a last step, but before maybe fix the individual pallet calls.

xlc · 2021-07-20T10:22:47Z

I kinda getting why this is needed but is there some proper analysis and explanation of why this is necessary and what should parachain do with their pallets?

gui1117 · 2021-07-20T10:35:52Z

I agree with what @athei propose: opt-in boxing of the whole call variant.
EDIT: second thought I feel like we add complexity for little benefit. Maybe reducing call to 200 bytes is ok and enough

athei · 2021-07-20T11:04:02Z

I agree with what @athei propose: opt-in boxing of the whole call variant.
EDIT: second thought I feel like we add complexity for little benefit. Maybe reducing call to 200 bytes is ok and enough

Complexity of the macro you mean? But these are one time costs. Burdening the pallet authors with boxing everything will be payed by more people and more often.

kianenigma · 2021-07-20T11:33:43Z

reading everything again, I see two ways to go:

Box each parameter individually. I honestly think adding macro syntax for this (e.g. (#[box] arg: Arg) instead of arg: Box<Arg>) is superfluous. If you want to box one argument, just do it manually, it is not that many extra characters and it makes the code easier to understand, with less macro magic. And you ONLY do this when you know that the size needs to be small. So this is not a cost for all users @athei, disagreeing with what you said:

Burdening the pallet authors with boxing everything will be payed by more people and more often.

Box the whole call of one pallet. I see this as a viable addition to the pallet macro. If you define

#[box_call]
#[pallet] 
mod pallet {
     // stuff
}

your entire pallet call is boxed when being amalgamated into the upper runtime call.

I think a combination of both is fine. If a pallet mostly have large calls, we can box it all. For other pallet, we manually box one or few arguments of individual dispatchables.

gui1117 · 2021-07-20T11:38:24Z

@ascjones had a usecase that requires to put all call variant into its own struct, so that generating type_info is easier.

If we accept this direction then we can add an opt-in attribute #[pallet::box_variant] for calls which allow to box an individual variant.

gui1117 · 2021-07-20T15:18:47Z

sorry for the back and forth but Andrew doesn't need to wrap in struct anymore. I think we can go for boxing the pallet calls in the runtime outer call. (We should probably also avoid more than 200 bytes pallet calls, there are a clippy lint for this, but it is less important).

gui1117 · 2021-07-20T15:37:27Z

This is the follow-up #9398

impl limit + reduce call size

6ac93bd

gui1117 requested review from andresilva, athei and kianenigma as code owners July 19, 2021 15:55

gui1117 added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jul 19, 2021

better doc

519d246

fix tests

00d8c32

gui1117 added the A7-needspolkadotpr label Jul 19, 2021

gui1117 added 2 commits July 19, 2021 19:11

fix tests

4701d8d

Better implementation of limit

6b04f07

gui1117 added 3 commits July 19, 2021 19:54

fix test

e734c0e

better doc and add limit to metadata

51dfb07

fix doc link

1233a69

fix benchmarks

7285356

shawntabrizi reviewed Jul 19, 2021

View reviewed changes

gui1117 added the A7-needspolkadotpr label Jul 19, 2021

kianenigma reviewed Jul 20, 2021

View reviewed changes

bin/node/runtime/src/lib.rs Outdated Show resolved Hide resolved

kianenigma reviewed Jul 20, 2021

View reviewed changes

athei reviewed Jul 20, 2021

View reviewed changes

improve test panic message

b56949a

gavofyork reviewed Jul 20, 2021

View reviewed changes

gui1117 closed this Jul 20, 2021

gui1117 deleted the gui-improve-call-usage branch July 20, 2021 15:37

louismerlin removed the D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited label Oct 13, 2022

	let allocator_limit = 33554432;
	let allocator_limit = 33554432;

improve runtime call, and its usage #9385

improve runtime call, and its usage #9385

Uh oh!

Conversation

gui1117 commented Jul 19, 2021

Uh oh!

gui1117 commented Jul 19, 2021

Uh oh!

shawntabrizi Jul 19, 2021

Choose a reason for hiding this comment

Uh oh!

gui1117 Jul 19, 2021

Choose a reason for hiding this comment

Uh oh!

gui1117 Jul 19, 2021

Choose a reason for hiding this comment

Uh oh!

gui1117 Jul 20, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kianenigma Jul 20, 2021

Choose a reason for hiding this comment

Uh oh!

gui1117 Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kianenigma left a comment

Choose a reason for hiding this comment

Uh oh!

athei Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gui1117 Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

athei Jul 20, 2021

Choose a reason for hiding this comment

Uh oh!

kianenigma Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gui1117 commented Jul 20, 2021

Uh oh!

athei commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kianenigma commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

athei commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gavofyork Jul 20, 2021

Choose a reason for hiding this comment

Uh oh!

bkchr commented Jul 20, 2021

Uh oh!

xlc commented Jul 20, 2021

Uh oh!

gui1117 commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

athei commented Jul 20, 2021

Uh oh!

kianenigma commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gui1117 commented Jul 20, 2021

Uh oh!

gui1117 commented Jul 20, 2021

Uh oh!

gui1117 commented Jul 20, 2021

Uh oh!

Reviewers

Assignees

gui1117 Jul 20, 2021 •

edited

Loading

athei Jul 20, 2021 •

edited

Loading

gui1117 Jul 20, 2021 •

edited

Loading

kianenigma Jul 20, 2021 •

edited

Loading

athei commented Jul 20, 2021 •

edited

Loading

kianenigma commented Jul 20, 2021 •

edited

Loading

athei commented Jul 20, 2021 •

edited

Loading

gui1117 commented Jul 20, 2021 •

edited

Loading

kianenigma commented Jul 20, 2021 •

edited

Loading