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

Use XCM benchmarks weights results instead of fixed estimations #1056

Closed
NachoPal opened this issue Mar 1, 2022 · 5 comments · Fixed by #1454
Closed

Use XCM benchmarks weights results instead of fixed estimations #1056

NachoPal opened this issue Mar 1, 2022 · 5 comments · Fixed by #1454
Assignees
Labels
T7-system_parachains This PR/Issue is related to System Parachains.

Comments

@NachoPal
Copy link
Contributor

NachoPal commented Mar 1, 2022

Currently, XCM weights are fixed and taken from here: https://github.com/paritytech/cumulus/blob/master/polkadot-parachains/statemine/src/xcm_config.rs#L126

Weights from the benchmarks results are only used in Westend:
https://github.com/paritytech/polkadot/blob/master/runtime/westend/src/xcm_config.rs#L111
https://github.com/paritytech/polkadot/blob/master/runtime/westend/src/xcm_config.rs#L137

I am wondering if this is intentional and if benchmarks weights can be applied also at least for Westmint, Statemine and Statemint.

cc @shawntabrizi @KiChjang

@NachoPal NachoPal added the T7-system_parachains This PR/Issue is related to System Parachains. label Mar 1, 2022
@NachoPal NachoPal added this to the statemint-v8.0.0 milestone Mar 1, 2022
@shawntabrizi
Copy link
Member

XCM benchmarks are basically in beta. I would be happy for you to check if they run here and integrate them if they do.

If they don't, would be good to see why.

@hbulgarini hbulgarini removed this from the statemint-v8.0.0 milestone Mar 2, 2022
@NachoPal
Copy link
Contributor Author

NachoPal commented Mar 2, 2022

XCM benchmarks are basically in beta. I would be happy for you to check if they run here and integrate them if they do.

If they don't, would be good to see why.

Why exactly the XCM benchmarks are still in beta. What are your concerns about them and how do you think they might negatively impact Statemine/mint runtimes?

Is it just about inaccurate weight values?

@shawntabrizi
Copy link
Member

No, its just that XCM is very generic and configurable, and that the benchmarks is not proven to work 100% of the time. You may run into errors, and we just need to catch those and fix them.

The only reason they have not been integrated is that it takes someone to do it, and I have been busy with other stuff.

@KiChjang has been tasked with this, but he is also very busy. So, if you feel you can jump in, would be very happy to have you try to integrate them.

@KiChjang
Copy link
Contributor

KiChjang commented Mar 2, 2022

I'm actually trying to fix the remaining XCM benchmarks here, but the compile-test-code cycle takes a really long time when it comes to benchmarks, but once it's done, I'll be taking over Shawn's XCM benchmark PR for Kusama and getting it to the finish line. By then I may be able to look at how we do the same benchmark with the asset parachains, if nobody else has gotten to it before me.

@gilescope
Copy link
Contributor

We should be able to do this for statemine/t now that paritytech/polkadot#4442 has landed for the relay chain.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T7-system_parachains This PR/Issue is related to System Parachains.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants