Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

Withdraw-teleport-benchmark #296

Merged
merged 19 commits into from
Oct 2, 2023
Merged

Withdraw-teleport-benchmark #296

merged 19 commits into from
Oct 2, 2023

Conversation

metricaez
Copy link
Collaborator

No description provided.

stiiifff
stiiifff previously approved these changes Sep 29, 2023
@stiiifff stiiifff requested a review from hbulgarini September 29, 2023 09:32
WithdrawAsset(assets.clone()),
BurnAsset(assets),
]);
T::Weigher::weight(&mut message).map_or(Weight::MAX, |w| <T as pallet::Config>::WeightInfo::withdraw_and_teleport().saturating_add(w))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% sure about this but here is my concern:

You are computing the weight for the XCM messages and once you have the result you sum that weight to the benchmarking for the extrinsic weight. Aren't these XCM messages charged already on the XCM Executor when the messages are processed? In other terms , are you not charging twice the XCM messages?

Copy link
Collaborator Author

@metricaez metricaez Oct 2, 2023

Choose a reason for hiding this comment

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

I stuck to the pattern used on pallet-xcm and traced relevant PR even though I agree it's not fully clear to me either. Already discussed this with xcm team and agreed on keeping it until they address it further just to be sure on not being undercharging.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What I do also realize is that if I want to fully stick to the pattern I should also add pallet-xcm send weight as it is also called inside the extrinsic, this is opaque on the case of teleport and reserve as it is done inside the instructions.

runtime/stout/src/xcm_config.rs Show resolved Hide resolved
// Measured: `177`
// Estimated: `3642`
// Minimum execution time: 87_000_000 picoseconds.
Weight::from_parts(88_000_000, 3642)
Copy link
Contributor

Choose a reason for hiding this comment

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

for some reason the ref_time is different from the benchmark result:

https://github.com/paritytech/trappist/pull/296/files#diff-220649b2a41e8072e94f45ddbe66886b3fbea765f3546d9aef12367a0681461cR50

Even if this will not be final results, because they have to be properly benchmarked with the bot i'm curious about this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because they are two different runs.

This one is only used at mock Runtime.

The one that will be used on the runtime itself is the one of the bot benchmark and I had to also run it locally for the trait to be generated not the value itself as it will be overwritten.

templates/frame-weight-template.hbs Show resolved Hide resolved
@stiiifff stiiifff merged commit 8aa876d into main Oct 2, 2023
4 checks passed
@stiiifff stiiifff deleted the emi-withdraw-teleport-benchmark branch October 2, 2023 15:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants