feat: benchmark foreign investments worst case #1565

wischli · 2023-09-22T14:17:44Z

Description

Adds ForeignInvestmentBenchmarkHelper which provides the setup required for benchmarking the heaviest foreign investment call
Adds benchmarking mod to pallet-liquidity-pools which benches an incoming CollectRedeem message by executing handle_collect_redemption
- NOTE: Skipped the InboundQueue since this complicates the investor setup: Right now, the investor account is created during the pool creation of pool_system::PoolBenchmarkHelper and thus just some Substrate account id. However, for the InboundQueue it would have to be a AccountConverter<Runtime, LocationToAccountId>::convert((sender_domain, investor_bytes)).
Feature creep: Renames pallet_foreign_investments::<T>::Config::Rate to BalanceRatio

TODO

Discuss how to move forward: Do we want to defensively stick to the hardcoded weights?
- If so, we should not add LiquidityPools to the runtimes' benchmark list
- Else, I would continue writing benchmarks for existing extrinsics such that we have everything covered properly: an inbound messages by the FI worst case (handle_collect_redemption) and extrinsics by their corresponding benchmarks
Maybe: Right now, the pallet-liquidity-pools benchmarks cannot be unit tested, because the existing mock is insufficient. If running unit tests is desired, I propose to enable this in a subsequent PR.

Justification about heaviest FI call

In the context of pallet-foreign-investments, the heaviest action (measured by amount of reads and writes) is collecting a redemption when there exists an InvestmentState which swaps from foreign to pool
By collecting the redemption, a swap from pool to foreign emerges which needs to be resolved in handle_concurrent_swap_orders by comparing with the existing swap from the investment

About the weight

For presentation purposes, I temporarily added the generated weight file. This must be removed before merging as otherwise executing benchmarks for pallet-liquidity-pools would remove all hardcoded values.

When looking at the results of executing the benchmark on my local machine, the total weight is ~55% of the current hardcoded one. The PoV size is ~80% of the hardcoded one. For details, see the WIP Weights Sheet. I suppose the locally generated weights are close enough to our benchmark runner, since the results are similar.

		Weight::from_parts(236_000_000, 71940)
			.saturating_add(T::DbWeight::get().reads(20))
			.saturating_add(T::DbWeight::get().writes(17))

Checklist:

I have added Rust doc comments to structs, enums, traits and functions
I have made corresponding changes to the documentation
I have performed a self-review of my code
I have added tests that prove my fix is effective or that my feature works

…o bench/foreign-investments

…ents

wischli · 2023-09-22T14:40:33Z

pallets/foreign-investments/src/lib.rs

@@ -152,7 +152,7 @@ pub mod pallet {

 		/// Type for price ratio for cost of incoming currency relative to
 		/// outgoing
-		type Rate: Parameter
+		type BalanceRatio: Parameter


Sorry for feature creeping. This change does not have any functional impact as it's just renaming.

wischli · 2023-09-22T14:41:08Z

pallets/liquidity-pools/src/weights_generated.rs

@@ -0,0 +1,78 @@
+
+//! Autogenerated weights for `pallet_liquidity_pools`


NOTE: Committed solely for demonstration purpose, will be removed before merging!

lemunozm

Love how clean are you getting this given that is quite difficult benchmark. 💯

lemunozm · 2023-09-22T16:16:19Z

libs/traits/src/benchmarking.rs

+	/// Returns
+	///  * The substrate investor address
+	///  * The investment id
+	///  * The pool currency id
+	///  * The foreign currency id
+	///  * A trading account which can bidirectionally fulfill swap orders for
+	///    the (foreign, pool) currency pair
+	fn bench_prepare_foreign_investments_setup() -> (
+		Self::AccountId,
+		Self::InvestmentId,
+		Self::CurrencyId,
+		Self::CurrencyId,
+		Self::AccountId,
+	);


Maybe a plain struct along with this trait can avoid errors when reading from these values.

lemunozm · 2023-09-25T07:13:11Z

pallets/foreign-investments/src/impls/benchmark_utils.rs

+	) {
+		let pool_id = Default::default();
+		let pool_admin: T::AccountId = frame_benchmarking::account("pool_admin", 0, 0);
+		<T::PoolInspect as PoolBenchmarkHelper>::bench_create_pool(pool_id, &pool_admin);


Comment: I really like how the bench_* methods are called inside bench_* methods, leaving a clean API to use from the benchmark side with 0 knowledge about what's happening under the hood. i.e. the pool_id used is totally transparent here ❤️

lemunozm · 2023-09-25T07:15:18Z

pallets/pool-system/src/impls.rs

+			let pool_account = PoolLocator { pool_id }.into_account_truncating();
+			frame_support::assert_ok!(T::Tokens::mint_into(
+				POOL_CURRENCY,
+				&pool_account,
+				POOL_ACCOUNT_BALANCE.into()
+			));


Just curious, why is this minting now needed?

This was taken from the integration tests when I was debugging BalanceTooLow errors. So it might not be necessary but I believe it still is.

When we process a redemption of tranche tokens, the pool account needs to have native balance which is exchanged for these tranche tokens.

In contrast, we don't need to fund the pool account with tranche tokens as these are minted when processing an investment.

mustermeiszer

Sick! Whats your final feeling regarding the adaption of the process_msg weight?

I am personally leaning torwards keeping it as is an heavier. But might be paranoia...

@wischli could you include the changes from #1562, then we do not need to wait for CI 2 times.

… bench/foreign-investments

libs/traits/src/benchmarking.rs

…ents

lemunozm

LGTM!

wischli and others added 18 commits September 19, 2023 17:27

feat: orderbook bench helper

3189ed3

feat: add bench_fill_order_full

b7227b4

refactor: bench_create_pool

bb563a0

refactor: orderbook bench helper

80f4c2f

feat: add InvestmentIdBenchmarkHelper

ac3ae0c

Merge branch 'main' into feat/orderbook-bench-helper

6681346

feat: support single pallet check benchmark

32e28bd

fix: use fixed weight for fn process_msg()

d1b80a5

Merge branch 'main' into feat/orderbook-bench-helper

2131eda

fix: mock_bench_default_investment_id

ff04c51

fix: order book bench

6adb198

refactor: use Ratio in FI

daddfb6

feat: ForeignInvestmentBenchmarkHelper

0af85e5

bench: fund pool account in create setup

1d6a5b1

wip: bench lp inbound msg

9cd2f50

Merge remote-tracking branch 'origin/feat/orderbook-bench-helper' int…

67aef5b

…o bench/foreign-investments

feat: bench inbound collect redeem

4fd7682

tmp: present generated weights

99e7087

wischli self-assigned this Sep 22, 2023

wischli added the I8-enhancement An additional feature. label Sep 22, 2023

Merge remote-tracking branch 'origin/main' into bench/foreign-investm…

a9f5b3f

…ents

wischli requested review from lemunozm and mustermeiszer September 22, 2023 14:39

wischli commented Sep 22, 2023

View reviewed changes

lemunozm reviewed Sep 25, 2023

View reviewed changes

mustermeiszer previously approved these changes Sep 25, 2023

View reviewed changes

wischli added 3 commits September 25, 2023 09:52

fix: Quantity remnant

d34c7ce

refactor: defensive weights

a22054e

refactor: add FI bench setup return type

239fa89

Merge remote-tracking branch 'origin/fix/unweighted-process-msg' into…

0d1c6f3

… bench/foreign-investments

wischli dismissed mustermeiszer’s stale review via 0d1c6f3 September 25, 2023 08:25

wischli marked this pull request as ready for review September 25, 2023 08:25

wischli requested review from mustermeiszer and lemunozm September 25, 2023 08:25

lemunozm reviewed Sep 25, 2023

View reviewed changes

libs/traits/src/benchmarking.rs Outdated Show resolved Hide resolved

wischli added 2 commits September 25, 2023 10:56

refactor: explicit return type

030c3a0

Merge remote-tracking branch 'origin/main' into bench/foreign-investm…

1936e32

…ents

wischli requested a review from lemunozm September 25, 2023 09:27

mustermeiszer mentioned this pull request Sep 25, 2023

Fix/unweighted process msg #1562

Closed

4 tasks

Merge branch 'main' into bench/foreign-investments

04ea57a

wischli enabled auto-merge (squash) September 25, 2023 12:39

mustermeiszer approved these changes Sep 25, 2023

View reviewed changes

lemunozm approved these changes Sep 25, 2023

View reviewed changes

Merge branch 'main' into bench/foreign-investments

90eb02e

wischli merged commit 255f8fd into main Sep 25, 2023
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: benchmark foreign investments worst case #1565

feat: benchmark foreign investments worst case #1565

wischli commented Sep 22, 2023 •

edited

wischli Sep 22, 2023 •

edited

wischli Sep 22, 2023

lemunozm left a comment

lemunozm Sep 22, 2023

wischli Sep 25, 2023

lemunozm Sep 25, 2023

lemunozm Sep 25, 2023

wischli Sep 25, 2023

mustermeiszer left a comment

lemunozm left a comment

		@@ -0,0 +1,78 @@

		//! Autogenerated weights for `pallet_liquidity_pools`

feat: benchmark foreign investments worst case #1565

feat: benchmark foreign investments worst case #1565

Conversation

wischli commented Sep 22, 2023 • edited

Description

TODO

Justification about heaviest FI call

About the weight

Checklist:

wischli Sep 22, 2023 • edited

Choose a reason for hiding this comment

wischli Sep 22, 2023

Choose a reason for hiding this comment

lemunozm left a comment

Choose a reason for hiding this comment

lemunozm Sep 22, 2023

Choose a reason for hiding this comment

wischli Sep 25, 2023

Choose a reason for hiding this comment

lemunozm Sep 25, 2023

Choose a reason for hiding this comment

lemunozm Sep 25, 2023

Choose a reason for hiding this comment

wischli Sep 25, 2023

Choose a reason for hiding this comment

mustermeiszer left a comment

Choose a reason for hiding this comment

lemunozm left a comment

Choose a reason for hiding this comment

wischli commented Sep 22, 2023 •

edited

wischli Sep 22, 2023 •

edited