Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add xcm benchmarks #1129

Merged
merged 16 commits into from
Jan 15, 2024
Merged

feat: add xcm benchmarks #1129

merged 16 commits into from
Jan 15, 2024

Conversation

ashutoshvarma
Copy link
Member

@ashutoshvarma ashutoshvarma commented Jan 9, 2024

Supersedes - #1012

Pull Request Summary
This PR adds the XCM Benchmarks (for both generic and fungible instructions). This will replace the fixed unit cost we charge currently.

XCM Benchmarks

  • The benchmarks for each instructions are implemented in pallet-xcm-benchmarks pallet which is separated into fungibles (instr that deals with asset transfer, etc) and generic.
  • Some of the benchmarks are needed to be modified to properly benchmark xcm in our runtimes, for this a new crate astar-xcm-benchmarks is introduced which is a wrapper over pallet-xcm-benchmarks with some benchmarks modified.
  • The WrappedBenchmark struct is used to merge benchmarks.

astar-xcm-benchmarks structure

├── fungible 
│   ├── benchmarking.rs        # `pallet_xcm_benchmakrs::fungible` modified benchs 
│   └── mod.rs
├── generic
│   ├── benchmarking.rs        # `pallet_xcm_benchmakrs::generic` modified benchs 
│   └── mod.rs
├── lib.rs
├── mock.rs                    # common mock runtime for bench tests

Notes

  • We only benchmark the fungible instructions with pallet-assets as assets since they involve additional DB read in XcAssetConfig for converting location to asset-id. Therefore they will be more expensive then native asset (pallet_balances)
  • The fungible's reserve_asset_deposited benchmarks was not available till v1.x.x release therefore is manually copy-pasted for now.

TODO

  • re-run benchmarks after final review

Check list

  • added or updated unit tests
  • updated Astar official documentation
  • added OnRuntimeUpgrade hook for precompile revert code registration
  • added benchmarks & weights for any modified runtime logics.

@ashutoshvarma ashutoshvarma added shibuya related to shibuya runtime This PR/Issue is related to the topic “runtime”. labels Jan 9, 2024
@ashutoshvarma ashutoshvarma marked this pull request as ready for review January 10, 2024 09:04
@ashutoshvarma
Copy link
Member Author

/bench shibuya-dev xcm_benchmarks_generic,xcm_benchmarks_fungible

Copy link

Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/7476342157.
Please wait for a while.
Branch: feat/xcm-benchmarks
SHA: 0a6acaa

@ashutoshvarma
Copy link
Member Author

/bench shibuya-dev xcm_benchmarks_generic,xcm_benchmarks_fungible

Copy link

Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/7477119023.
Please wait for a while.
Branch: feat/xcm-benchmarks
SHA: 0a6acaa

@shaunxw
Copy link
Member

shaunxw commented Jan 10, 2024

/bench shibuya-dev xcm_benchmarks_generic,xcm_benchmarks_fungible

Copy link

Benchmarks have been finished.
You can download artifacts if exists https://github.com/AstarNetwork/Astar/actions/runs/7477119023.

@AstarNetwork AstarNetwork deleted a comment from github-actions bot Jan 10, 2024
@shaunxw
Copy link
Member

shaunxw commented Jan 10, 2024

/bench shibuya-dev xcm_benchmarks_generic,xcm_benchmarks_fungible

Copy link

Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/7478071828.
Please wait for a while.
Branch: feat/xcm-benchmarks
SHA: 0a6acaa

@shaunxw
Copy link
Member

shaunxw commented Jan 10, 2024

/bench shibuya-dev xcm_benchmarks_generic,xcm_benchmarks_fungible

Copy link

Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/7478138231.
Please wait for a while.
Branch: feat/xcm-benchmarks
SHA: 0a6acaa

Copy link
Member

@Dinonard Dinonard left a comment

Choose a reason for hiding this comment

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

I checked some of the instructions, just to see why certain weights were measured. But I'll probably do more later before the merge. All looks good to me so far.

Just one question about the approach to reuse.

Comment on lines +31 to +34
/// We need re-write buy_execution benchmark becuase our runtime
/// needs 1 additional DB read (XcAssetConfig) for fetching unit per sec
/// for a fungible asset. The upstream benchmark use native assets thus
/// won't accout for it.
Copy link
Member

Choose a reason for hiding this comment

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

One general question about this approach - how viable is to reuse code from the official repo if we have to account for different actions for some calls?

This can change in the future, upstream, and it means we should be re-checking all of the benchmarks each time we do an uplift?

It would seem easier to just have all the benchmarks in our repo since that way we have full control over it. If I understood the current approach correctly, it's highly likely we miss something in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

The upstream benchmarks are quite generic themselves, but not generic enough maybe because not many parachains are using them.
The benchmarks we had to re-write were only due the use of hardcoded native asset inside some benchmarks.

  • buy_execution: hardcoded native asset used for buying execution
  • transfer_*: does not take ED into account, they use native asset (pallet_balances) which defaults to allow death unlike fungibles (pallet_assets).
  • expect_pallet: hardcoded pallet index

Other than that all benchmarks were configurable through the pallet's Config. As long as we we used same release upstream pallet (XcmConfig is not changed) it should be fine.

For the hardcoded cases, we can make a upstream PR to make it more generic for other parachains to use as well.

I'm fine either ways, if we copy-paste all benchmarks then we need to keep them updated during uplift if there are major changes like precompile-utils and if we don't copy-paste then we still need to check them during uplifts

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation!

Let's keep it like this, as you suggested.
For the upstream PR, let's start with an issue first perhaps?

Comment on lines 207 to 213
fn reserve_asset_deposited() -> Weight {
// Proof Size summary in bytes:
// Measured: `0`
// Estimated: `0`
// Minimum execution time: 3_440_000 picoseconds.
Weight::from_parts(3_603_000, 0)
}
Copy link
Member

Choose a reason for hiding this comment

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

Added manually?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I meant added (copy-pasted) the benchmark itself from pallet-xcm-benchmark v1.x.x release not the weights

Copy link
Member

Choose a reason for hiding this comment

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

I know, but I asked because there are no docs above the function like the others 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh that is because ReserveAssetDeposited does not use touch storage and just put asset into holding thus purely ref_time based computation therefore no storage access logs above

PierreOssun
PierreOssun previously approved these changes Jan 12, 2024
Copy link
Member

@PierreOssun PierreOssun left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -53,6 +53,7 @@ use pallet_xc_asset_config::{ExecutionPaymentRate, XcAssetLocation};
mod tests;

pub const XCM_SIZE_LIMIT: u32 = 2u32.pow(16);
pub const MAX_ASSETS: u32 = 64;
Copy link
Member

Choose a reason for hiding this comment

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

Is this const not already set somewhere ? Or it's an arbitrary value

Copy link
Member Author

Choose a reason for hiding this comment

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

It was already set in XcmConfig::MaxAssetsIntoHolding as ConstU64<64>, I just refactor it to be primitives.

Copy link
Member

Choose a reason for hiding this comment

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

It's more of a const than a primitive though 🙂

Might be worth putting all consts into a separate module later on.

@ashutoshvarma
Copy link
Member Author

/bench shibuya-dev xcm_benchmarks_generic,xcm_benchmarks_fungible

Copy link

Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/7503066669.
Please wait for a while.
Branch: feat/xcm-benchmarks
SHA: 121a505

Copy link

Benchmarks have been finished.
You can download artifacts if exists https://github.com/AstarNetwork/Astar/actions/runs/7503066669.

@ashutoshvarma
Copy link
Member Author

/bench shibuya-dev xcm_benchmarks_generic,xcm_benchmarks_fungible

Copy link

Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/7511661871.
Please wait for a while.
Branch: feat/xcm-benchmarks
SHA: d258dd8

Copy link

Benchmarks have been finished.
You can download artifacts if exists https://github.com/AstarNetwork/Astar/actions/runs/7511661871.

Dinonard
Dinonard previously approved these changes Jan 15, 2024
Copy link
Member

@Dinonard Dinonard left a comment

Choose a reason for hiding this comment

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

LGTM

I'd suggest to apply the 2nd comment, but the 1st one is more of a question so can be freely ignored.

Comment on lines +89 to +100
/// Storage: ParachainInfo ParachainId (r:1 w:0)
/// Proof: ParachainInfo ParachainId (max_values: Some(1), max_size: Some(4), added: 499, mode: MaxEncodedLen)
/// Storage: PolkadotXcm SupportedVersion (r:1 w:0)
/// Proof Skipped: PolkadotXcm SupportedVersion (max_values: None, max_size: None, mode: Measured)
/// Storage: PolkadotXcm VersionDiscoveryQueue (r:1 w:1)
/// Proof Skipped: PolkadotXcm VersionDiscoveryQueue (max_values: Some(1), max_size: None, mode: Measured)
/// Storage: PolkadotXcm SafeXcmVersion (r:1 w:0)
/// Proof Skipped: PolkadotXcm SafeXcmVersion (max_values: Some(1), max_size: None, mode: Measured)
/// Storage: ParachainSystem HostConfiguration (r:1 w:0)
/// Proof Skipped: ParachainSystem HostConfiguration (max_values: Some(1), max_size: None, mode: Measured)
/// Storage: ParachainSystem PendingUpwardMessages (r:1 w:1)
/// Proof Skipped: ParachainSystem PendingUpwardMessages (max_values: Some(1), max_size: None, mode: Measured)
Copy link
Member

Choose a reason for hiding this comment

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

I find this part a bit weird - shouldn't these be covered already by the XCM-related hooks?

Copy link
Member Author

Choose a reason for hiding this comment

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

}
}

// For backwards compatibility and tests
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed given it's Shibuya-specific benchmark?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I'll remove them

Copy link

Code Coverage

Package Line Rate Branch Rate Health
pallets/dapp-staking-migration/src 39% 0%
pallets/ethereum-checked/src 48% 0%
pallets/dapp-staking-v3/src 76% 0%
pallets/astar-xcm-benchmarks/src 0% 0%
pallets/dapps-staking/src/pallet 85% 0%
primitives/src 57% 0%
pallets/astar-xcm-benchmarks/src/fungible 0% 0%
pallets/unified-accounts/src 84% 0%
pallets/collator-selection/src 69% 0%
pallets/xvm/src 40% 0%
pallets/xc-asset-config/src 53% 0%
pallets/dapp-staking-v3/src/benchmarking 0% 0%
precompiles/sr25519/src 64% 0%
precompiles/unified-accounts/src 100% 0%
primitives/src/xcm 66% 0%
precompiles/dapp-staking-v3/src/test 0% 0%
precompiles/dapps-staking/src 94% 0%
pallets/dynamic-evm-base-fee/src 81% 0%
chain-extensions/pallet-assets/src 56% 0%
chain-extensions/unified-accounts/src 0% 0%
precompiles/xcm/src 72% 0%
chain-extensions/types/assets/src 0% 0%
pallets/block-rewards-hybrid/src 87% 0%
pallets/dapp-staking-v3/src/test 0% 0%
precompiles/dapp-staking-v3/src 90% 0%
pallets/astar-xcm-benchmarks/src/generic 0% 0%
precompiles/xvm/src 74% 0%
pallets/dapp-staking-v3/rpc/runtime-api/src 0% 0%
pallets/static-price-provider/src 61% 0%
chain-extensions/types/xvm/src 0% 0%
chain-extensions/types/unified-accounts/src 0% 0%
pallets/inflation/src 70% 0%
chain-extensions/xvm/src 0% 0%
pallets/dapps-staking/src 81% 0%
precompiles/assets-erc20/src 81% 0%
precompiles/substrate-ecdsa/src 74% 0%
Summary 70% (3254 / 4679) 0% (0 / 0)

Minimum allowed line rate is 50%

@ashutoshvarma ashutoshvarma merged commit ffe8628 into master Jan 15, 2024
8 checks passed
@ashutoshvarma ashutoshvarma deleted the feat/xcm-benchmarks branch January 15, 2024 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
runtime This PR/Issue is related to the topic “runtime”. shibuya related to shibuya
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants