XCM Benchmarks for Asset Transactor w/ Fungible Asset #3818

shawntabrizi · 2021-09-09T06:18:07Z

This PR includes benchmarks for XCMs which use the AssetTransactor trait, implemented for an asset which implements the Fungible trait.

shawntabrizi · 2021-09-09T06:36:13Z

/benchmark runtime custom --chain=westend-dev --pallet=pallet_xcm_benchmarks::fungible --extrinsic="*" --steps=10 --repeat=10 --template=./xcm/pallet-xcm-benchmarks/template.hbs --output=./ --execution=wasm --wasm-execution=compiled

parity-benchapp · 2021-09-09T06:36:15Z

Benchmark Runtime Custom for branch "shawntabrizi-fungible-benchmarks" with command cargo run --quiet --release --features runtime-benchmarks -- benchmark --chain=westend-dev --pallet=pallet_xcm_benchmarks::fungible --extrinsic="*" --steps=10 --repeat=10 --template=./xcm/pallet-xcm-benchmarks/template.hbs --output=./ --execution=wasm --wasm-execution=compiled

Results

Pallet: "pallet_xcm_benchmarks::fungible", Extrinsic: "withdraw_asset", Lowest values: [], Highest values: [], Steps: 10, Repeat: 10
Raw Storage Info
========
Storage: System Account (r:1 w:1)

Median Slopes Analysis
========
-- Extrinsic Time --

Model:
Time ~=     39.3
              µs

Reads = 1
Writes = 1

Min Squares Analysis
========
-- Extrinsic Time --

Model:
Time ~=     39.3
              µs

Reads = 1
Writes = 1

Pallet: "pallet_xcm_benchmarks::fungible", Extrinsic: "transfer_asset", Lowest values: [], Highest values: [], Steps: 10, Repeat: 10
Raw Storage Info
========
Storage: System Account (r:2 w:2)

Median Slopes Analysis
========
-- Extrinsic Time --

Model:
Time ~=    60.53
              µs

Reads = 2
Writes = 2

Min Squares Analysis
========
-- Extrinsic Time --

Model:
Time ~=    60.53
              µs

Reads = 2
Writes = 2

Pallet: "pallet_xcm_benchmarks::fungible", Extrinsic: "transfer_reserve_asset", Lowest values: [], Highest values: [], Steps: 10, Repeat: 10
Raw Storage Info
========
Storage: System Account (r:2 w:2)
Storage: Dmp DownwardMessageQueueHeads (r:1 w:1)
Storage: Dmp DownwardMessageQueues (r:1 w:1)

Median Slopes Analysis
========
-- Extrinsic Time --

Model:
Time ~=    83.37
              µs

Reads = 4
Writes = 4

Min Squares Analysis
========
-- Extrinsic Time --

Model:
Time ~=    83.37
              µs

Reads = 4
Writes = 4

Pallet: "pallet_xcm_benchmarks::fungible", Extrinsic: "receive_teleported_asset", Lowest values: [], Highest values: [], Steps: 10, Repeat: 10
Raw Storage Info
========
Storage: Benchmark Override (r:0 w:0)

Median Slopes Analysis
========
-- Extrinsic Time --

Model:
Time ~= 18446744073700
              µs

Reads = 0
Writes = 0

Min Squares Analysis
========
-- Extrinsic Time --

Model:
Time ~= 18446744073700
              µs

Reads = 0
Writes = 0

Pallet: "pallet_xcm_benchmarks::fungible", Extrinsic: "deposit_asset", Lowest values: [], Highest values: [], Steps: 10, Repeat: 10
Raw Storage Info
========
Storage: System Account (r:1 w:1)

Median Slopes Analysis
========
-- Extrinsic Time --

Model:
Time ~=    49.37
              µs

Reads = 1
Writes = 1

Min Squares Analysis
========
-- Extrinsic Time --

Model:
Time ~=    49.37
              µs

Reads = 1
Writes = 1

Pallet: "pallet_xcm_benchmarks::fungible", Extrinsic: "deposit_reserve_asset", Lowest values: [], Highest values: [], Steps: 10, Repeat: 10
Raw Storage Info
========
Storage: System Account (r:1 w:1)
Storage: Dmp DownwardMessageQueueHeads (r:1 w:1)
Storage: Dmp DownwardMessageQueues (r:1 w:1)

Median Slopes Analysis
========
-- Extrinsic Time --

Model:
Time ~=    75.99
              µs

Reads = 3
Writes = 3

Min Squares Analysis
========
-- Extrinsic Time --

Model:
Time ~=    75.99
              µs

Reads = 3
Writes = 3

Pallet: "pallet_xcm_benchmarks::fungible", Extrinsic: "initiate_teleport", Lowest values: [], Highest values: [], Steps: 10, Repeat: 10
Raw Storage Info
========
Storage: System Account (r:1 w:1)
Storage: Dmp DownwardMessageQueueHeads (r:1 w:1)
Storage: Dmp DownwardMessageQueues (r:1 w:1)

Median Slopes Analysis
========
-- Extrinsic Time --

Model:
Time ~=    76.46
              µs

Reads = 3
Writes = 3

Min Squares Analysis
========
-- Extrinsic Time --

Model:
Time ~=    76.46
              µs

Reads = 3
Writes = 3

…k --chain=westend-dev --pallet=pallet_xcm_benchmarks::fungible --extrinsic=* --steps=10 --repeat=10 --template=./xcm/pallet-xcm-benchmarks/template.hbs --output=./ --execution=wasm --wasm-execution=compiled

shawntabrizi · 2021-09-10T12:22:50Z

/benchmark runtime custom --chain=westend-dev --pallet=pallet_xcm_benchmarks::fungible --extrinsic="*" --steps=50 --repeat=20 --template=./xcm/pallet-xcm-benchmarks/template.hbs --output=./ --execution=wasm --wasm-execution=compiled

parity-benchapp · 2021-09-10T12:22:51Z

Benchmark Runtime Custom for branch "shawntabrizi-fungible-benchmarks" with command cargo run --quiet --release --features runtime-benchmarks -- benchmark --chain=westend-dev --pallet=pallet_xcm_benchmarks::fungible --extrinsic="*" --steps=50 --repeat=20 --template=./xcm/pallet-xcm-benchmarks/template.hbs --output=./ --execution=wasm --wasm-execution=compiled

Results

2021-09-10 12:38:05 Took active validators from set with wrong size    
2021-09-10 12:38:05 Took active validators from set with wrong size    
2021-09-10 12:38:06 WARNING: benchmark error skipped - receive_teleported_asset    
2021-09-10 12:38:06 WARNING: benchmark error skipped - receive_teleported_asset    
Error: 
   0: �[91mInvalid input: Benchmark not found for this pallet.�[0m

Backtrace omitted.
Run with RUST_BACKTRACE=1 environment variable to display it.
Run with RUST_BACKTRACE=full to include source snippets.

ERROR: Unable to commit file ./

shawntabrizi · 2021-09-17T01:32:00Z

/benchmark xcm westend pallet_xcm_benchmarks::fungible

shawntabrizi · 2021-09-17T04:36:26Z

/benchmark xcm westend pallet_xcm_benchmarks::fungible

parity-benchapp · 2021-09-17T04:36:27Z

Benchmark Westend XCM for branch "shawntabrizi-fungible-benchmarks" with command cargo run --quiet --release --features=runtime-benchmarks -- benchmark --chain=westend-dev --steps=50 --repeat=20 --pallet=pallet_xcm_benchmarks::fungible --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --template=./xcm/pallet-xcm-benchmarks/template.hbs --output=./runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_fungible.rs

Results

2021-09-17 04:46:50 [0] 💸 generated 1 npos voters, 1 from validators and 0 nominators    
2021-09-17 04:46:50 Took active validators from set with wrong size    
2021-09-17 04:46:50 Took active validators from set with wrong size.    
2021-09-17 04:46:50 Took active validators from set with wrong size    
2021-09-17 04:46:52 WARNING: benchmark error skipped - receive_teleported_asset    
2021-09-17 04:46:52 WARNING: benchmark error skipped - receive_teleported_asset    
Error: 
   0: �[91mInvalid input: Benchmark not found for this pallet.�[0m

Backtrace omitted.
Run with RUST_BACKTRACE=1 environment variable to display it.
Run with RUST_BACKTRACE=full to include source snippets.

ERROR: Unable to commit file ./runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_fungible.rs

kianenigma · 2021-09-22T12:23:36Z

xcm/pallet-xcm-benchmarks/Cargo.toml

+# temp
+pallet-xcm = { path = "../pallet-xcm" }
+polkadot-runtime-common = { path = "../../runtime/common" }
+# westend-runtime = { path = "../../runtime/westend", features = ["runtime-benchmarks"] }


TODO remove me later

kianenigma · 2021-09-22T12:53:05Z

xcm/pallet-xcm-benchmarks/src/lib.rs

+const HOLDING_FUNGIBLES: u32 = 99;
+const HOLDING_NON_FUNGIBLES: u32 = 99;


Should go into the config of the corresponding xcm_benchmark pallet, right?

kianenigma · 2021-09-22T12:53:39Z

xcm/pallet-xcm-benchmarks/src/lib.rs

+const HOLDING_NON_FUNGIBLES: u32 = 99;
+
+pub fn worst_case_holding() -> Assets {
+	let fungibles_amount: u128 = 100; // TODO probably update


your opinion was always sticking to ED and multiplies thereof, which should be possible here if you pass in a T.

kianenigma · 2021-09-22T13:04:58Z

xcm/pallet-xcm-benchmarks/src/fungible/benchmarking.rs

+		let instruction = Instruction::TransferReserveAsset {
+			assets,
+			dest: dest_location,
+			xcm: Xcm::new()


so the cost of this nested send is already incorporated here, this is much simpler than the old scheme of order and effect.

kianenigma · 2021-09-22T13:06:46Z

xcm/pallet-xcm-benchmarks/src/fungible/benchmarking.rs

+		let dest_account = T::AccountIdConverter::convert(dest_location.clone()).unwrap();
+		assert!(T::TransactAsset::balance(&dest_account).is_zero());
+
+		let mut executor = new_executor::<T>(Default::default());


nit: might want to make it fn new_executor<T: Config>(_: MultiLocation, initial_holding: Assets)

xcm/pallet-xcm-benchmarks/src/fungible/benchmarking.rs

kianenigma · 2021-09-22T13:13:37Z

xcm/pallet-xcm-benchmarks/src/fungible/mock.rs

@@ -0,0 +1,180 @@
+// Copyright 2021 Parity Technologies (UK) Ltd.


not super relevant yet since this PR is only about the fungible, but once a fungibles has been added as well, it would be sad to see this mock almost entirely duplicated. I think one mock runtime that implements both xcm_benchamrk::fungible::Config and xcm_benchamrk::fungibles::Config should work fine.

kianenigma

LGTM, most TODOs seem to be related to the next steps, I think we can leave them, try and merge this, to keep the PRs within bite-size pieces.

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

runtime/westend/src/lib.rs

apopiak · 2021-09-23T08:49:56Z

runtime/westend/src/weights/xcm/mod.rs

+	fn report_error(
+		_query_id: &QueryId,
+		_dest: &MultiLocation,
+		max_response_weight: &u64,
+	) -> Weight {
+		*max_response_weight
+	}


mark this as preliminary?
the max_response_weight is about the response, no the error itself, right?
there should probably be a XcmGeneric::<Runtime>::report_error(query_id, dest) or similar?

apopiak

👍 for XCM weights/benchmarks! 🎉
Seems to me this has a few rough edges. Unclear to me whether you want to smooth those out now or in a follow up PR.

apopiak · 2021-09-24T07:56:01Z

runtime/westend/src/lib.rs

+			impl pallet_xcm_benchmarks::Config for Runtime {
+				type XcmConfig = XcmConfig;
+				type AccountIdConverter = LocationConverter;
+				fn valid_destination() -> Result<MultiLocation, sp_runtime::DispatchError> {


DQ: Why does this return a result?

There may need to be some setup required to make a destination valid, for example calling some dispatchables which sets up UMP/DMP settings, or gives a chain funds or anything. Also there may be no valid destination for a chain which doesnt communicate to anyone.

apopiak · 2021-09-24T08:04:47Z

runtime/westend/src/weights/xcm/mod.rs

+	}
+	fn deposit_reserve_asset(
+		assets: &MultiAssetFilter,
+		_max_assets: &u32, // TODO use max assets?


100%, would be a nice reduction in weight

apopiak · 2021-09-24T08:08:25Z

runtime/westend/src/weights/xcm/mod.rs

+	fn initiate_teleport(
+		assets: &MultiAssetFilter,
+		_dest: &MultiLocation,
+		_xcm: &Xcm<()>,


I guess we're going to weigh intiate_teleport based on the worst-case holding?
a max_assets would be nice here as well, right?

apopiak · 2021-09-24T08:18:24Z

xcm/pallet-xcm-benchmarks/src/fungible/benchmarking.rs

+			BenchmarkError::Override(
+				BenchmarkResult::from_weight(T::BlockWeights::get().max_block)
+			)


what is this covering for? shouldn't this always succeed or fail loudly?

What about chains who just do not support this particular XCM. it covers that scenario where you can still run benchmarks for everything else.

The resulting number screams pretty loudly.

apopiak · 2021-09-24T08:41:31Z

xcm/pallet-xcm-benchmarks/src/lib.rs

+	(0..HOLDING_FUNGIBLES)
+		.map(|i| {
+			MultiAsset {
+				id: Concrete(GeneralIndex(i as u128).into()),
+				fun: Fungible(fungibles_amount * i as u128),
+			}
+			.into()
+		})
+		.chain(core::iter::once(MultiAsset { id: Concrete(Here.into()), fun: Fungible(u128::MAX) }))


This means the worst case holding amount of fungibles is 100, potentially increasing to 101 if the benchmark adds a new asset.
Is that the desired setup?

apopiak · 2021-09-24T08:47:52Z

xcm/pallet-xcm-benchmarks/src/lib.rs

+pub fn new_executor<T: Config>(origin: MultiLocation) -> ExecutorOf<T> {
+	ExecutorOf::<T>::new(origin)
+}


this, (or a differently named util function) might also just populate the executor with a worst-case holding on construction

apopiak · 2021-09-24T08:51:15Z

xcm/pallet-xcm-benchmarks/src/mock.rs

+pub struct AllAssetLocationsPass;
+impl FilterAssetLocation for AllAssetLocationsPass {
+	fn filter_asset_location(_: &MultiAsset, _: &MultiLocation) -> bool {
+		true
+	}
+}


This seems unused?

why? it is a mock function to enable tests to pass

apopiak · 2021-09-24T09:02:18Z

xcm/pallet-xcm-benchmarks/src/fungible/mod.rs

+		type CheckedAccount: Get<Option<Self::AccountId>>;
+
+		/// A trusted location which we allow teleports from, and the asset we allow to teleport.
+		type TrustedTeleporter: Get<Option<(xcm::latest::MultiLocation, xcm::latest::MultiAsset)>>;


why introduce an extra asset here?, you already have get_multi_asset below?

It can be a different asset for teleports versus other xcm messages.

why are you worried about including more information?

Just trying to keep the interface/config tidy.

apopiak · 2021-09-24T09:08:16Z

xcm/pallet-xcm-benchmarks/src/fungible/mock.rs

+
+impl crate::Config for Test {
+	type XcmConfig = XcmConfig;
+	type AccountIdConverter = AccountIdConverter;


Why introduce an extra AccountIdConverter when there is the OriginConverter in the XcmConfig?

apopiak · 2021-09-24T09:13:15Z

runtime/westend/src/weights/xcm/mod.rs

+	fn weigh_multi_assets(&self, balances_weight: Weight) -> Weight;
+}
+
+// TODO wild case


Todo.
I'm guessing you'll address this in a follow-up PR?

apopiak

Seems fine if we iron out any kinks in follow ups.

shawntabrizi · 2021-09-27T04:31:09Z

bot merge

ghost · 2021-09-27T04:31:13Z

Waiting for commit status.

ghost · 2021-09-27T04:34:15Z

Merge aborted: Checks failed for 7b47f5c

shawntabrizi · 2021-09-27T04:38:41Z

https://research.web3.foundation/ is down and causing the CI failure.

* master: (24 commits) Added multisig in Rococo (#3873) Fix bin-substrate toml (#3939) Bump tracing from 0.1.27 to 0.1.28 (#3937) XCM Benchmarks for Asset Transactor w/ Fungible Asset (#3818) Bump libc from 0.2.101 to 0.2.102 (#3933) Do not compile `wasm_api` module when not needed. (#3934) Bump syn from 1.0.76 to 1.0.77 (#3932) Fix spelling (#3845) Bump tokio from 1.11.0 to 1.12.0 (#3910) Ignore `generate-bags` by dependabot (#3930) Update logging in paras_inherent (#3927) Bump libsecp256k1 from 0.6.0 to 0.7.0 (#3909) Remove BEEFY repo dependency (#3923) Substrate Companion for rust 1.54 (#3807) Fix broken links (#3919) update BaseXcmWegiht to match Kusama (#3911) add parachains pallets to Polkadot runtime (#3815) Add a `force_unfreeze` extrinsic to the disputes module (#3906) Add new rococo chainspec (#3905) Ensure all parachain configuration extrinsics are operational (#3912) ...

shawntabrizi added 6 commits September 9, 2021 01:36

benchmarks for fungibles

cdf3e86

add benchmark to westend

7e4fd07

fix hex

419702f

clean up a bit

06c1a9d

update code doc

70beba3

fix warnings

ce4705b

paritytech deleted a comment from parity-benchapp bot Sep 9, 2021

cargo run --quiet --release --features runtime-benchmarks -- benchmar…

99215ad

…k --chain=westend-dev --pallet=pallet_xcm_benchmarks::fungible --extrinsic=* --steps=10 --repeat=10 --template=./xcm/pallet-xcm-benchmarks/template.hbs --output=./ --execution=wasm --wasm-execution=compiled

shawntabrizi added 3 commits September 10, 2021 08:07

Merge branch 'master' into shawntabrizi-fungible-benchmarks

26ece05

use skip

cdf2b9b

fix spelling

9955fad

shawntabrizi added 2 commits September 16, 2021 21:28

Merge branch 'master' into shawntabrizi-fungible-benchmarks

f313d57

Update Cargo.lock

556dccf

shawntabrizi added 3 commits September 16, 2021 22:19

add scale-info

f491adf

Merge branch 'master' into shawntabrizi-fungible-benchmarks

386d3a5

Update Cargo.lock

2381dcb

update bench

dcdc5e8

kianenigma reviewed Sep 22, 2021

View reviewed changes

xcm/pallet-xcm-benchmarks/src/fungible/benchmarking.rs Outdated Show resolved Hide resolved

kianenigma reviewed Sep 22, 2021

View reviewed changes

kianenigma approved these changes Sep 22, 2021

View reviewed changes

shawntabrizi and others added 2 commits September 23, 2021 01:01

Apply suggestions from code review

11e5d7d

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

fix code review

a7519b6

apopiak reviewed Sep 23, 2021

View reviewed changes

apopiak reviewed Sep 24, 2021

View reviewed changes

apopiak approved these changes Sep 24, 2021

View reviewed changes

shawntabrizi added 4 commits September 26, 2021 23:16

Merge branch 'master' into shawntabrizi-fungible-benchmarks

007e446

update weight

161906f

fix report_error

3f394c0

fix spell check

7b47f5c

shawntabrizi merged commit f0b2bf3 into master Sep 27, 2021

shawntabrizi deleted the shawntabrizi-fungible-benchmarks branch September 27, 2021 04:38

mmostafas added D1-audited 👍 PR contains changes to critical logic that has been properly reviewed and externally audited. and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Sep 28, 2021

Dengjianping mentioned this pull request Oct 13, 2021

[Calamari]Bump deps to polkadot-v0.9.11 Manta-Network/Manta#211

Merged

17 tasks

ghzlatarev mentioned this pull request Nov 2, 2021

[Manta-PC] Update dependencies to v0.9.12 Manta-Network/Manta#242

Merged

19 tasks

		const HOLDING_FUNGIBLES: u32 = 99;
		const HOLDING_NON_FUNGIBLES: u32 = 99;

		@@ -0,0 +1,180 @@
		// Copyright 2021 Parity Technologies (UK) Ltd.

XCM Benchmarks for Asset Transactor w/ Fungible Asset #3818

XCM Benchmarks for Asset Transactor w/ Fungible Asset #3818

Conversation

shawntabrizi commented Sep 9, 2021

shawntabrizi commented Sep 9, 2021

parity-benchapp bot commented Sep 9, 2021 • edited Loading

shawntabrizi commented Sep 10, 2021

parity-benchapp bot commented Sep 10, 2021 • edited Loading

shawntabrizi commented Sep 17, 2021

shawntabrizi commented Sep 17, 2021

parity-benchapp bot commented Sep 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apopiak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apopiak left a comment

Choose a reason for hiding this comment

shawntabrizi commented Sep 27, 2021

ghost commented Sep 27, 2021

ghost commented Sep 27, 2021

shawntabrizi commented Sep 27, 2021

parity-benchapp bot commented Sep 9, 2021 •

edited

Loading

parity-benchapp bot commented Sep 10, 2021 •

edited

Loading

parity-benchapp bot commented Sep 17, 2021 •

edited

Loading