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

Changed T::BlockWeights::get().max_block -> Weight::MAX #7546

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

bkontur
Copy link
Contributor

@bkontur bkontur commented Jul 25, 2023

Based on discussion bellow: paritytech/cumulus#2934 (comment)

@bkontur bkontur added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jul 25, 2023
@bkontur
Copy link
Contributor Author

bkontur commented Jul 25, 2023

bot bench $ xcm rococo pallet_xcm_benchmarks::fungible
bot bench $ xcm westend pallet_xcm_benchmarks::fungible
bot bench $ xcm kusama pallet_xcm_benchmarks::fungible
bot bench $ xcm polkadot pallet_xcm_benchmarks::fungible

@command-bot
Copy link

command-bot bot commented Jul 25, 2023

@bkontur Positional arguments are not supported anymore. I guess you meant bot bench polkadot-pallet --subcommand=xcm --runtime=rococo --pallet=pallet_xcm_benchmarks::fungible, but I could be wrong.
Read docs to find out how to run your command.

@bkontur
Copy link
Contributor Author

bkontur commented Jul 25, 2023

bot bench polkadot-pallet --subcommand=xcm --runtime=rococo --pallet=pallet_xcm_benchmarks::fungible
bot bench polkadot-pallet --subcommand=xcm --runtime=westend --pallet=pallet_xcm_benchmarks::fungible
bot bench polkadot-pallet --subcommand=xcm --runtime=kusama --pallet=pallet_xcm_benchmarks::fungible
bot bench polkadot-pallet --subcommand=xcm --runtime=polkadot --pallet=pallet_xcm_benchmarks::fungible

@command-bot
Copy link

command-bot bot commented Jul 25, 2023

@bkontur https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3256526 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=xcm --runtime=rococo --target_dir=polkadot --pallet=pallet_xcm_benchmarks::fungible. Check out https://gitlab.parity.io/parity/mirrors/polkadot/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 13-a6626e24-1309-4a8c-8d03-1016879fec28 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Jul 25, 2023

@bkontur https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3256527 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=xcm --runtime=westend --target_dir=polkadot --pallet=pallet_xcm_benchmarks::fungible. Check out https://gitlab.parity.io/parity/mirrors/polkadot/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 14-bcf34a79-e19a-49aa-81dc-c0ba4982f407 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Jul 25, 2023

@bkontur https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3256528 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=xcm --runtime=kusama --target_dir=polkadot --pallet=pallet_xcm_benchmarks::fungible. Check out https://gitlab.parity.io/parity/mirrors/polkadot/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 15-eb8171a8-5640-4174-b12a-7a8eeea85477 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Jul 25, 2023

@bkontur https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3256529 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=xcm --runtime=polkadot --target_dir=polkadot --pallet=pallet_xcm_benchmarks::fungible. Check out https://gitlab.parity.io/parity/mirrors/polkadot/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 16-fbd86393-77d4-4f6b-bae9-ea1dc3447896 to cancel this command or bot cancel to cancel all commands in this pull request.

…sama --target_dir=polkadot --pallet=pallet_xcm_benchmarks::fungible
@command-bot
Copy link

command-bot bot commented Jul 25, 2023

@bkontur Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=xcm --runtime=kusama --target_dir=polkadot --pallet=pallet_xcm_benchmarks::fungible has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3256528 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3256528/artifacts/download.

@command-bot
Copy link

command-bot bot commented Jul 25, 2023

@bkontur Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=xcm --runtime=rococo --target_dir=polkadot --pallet=pallet_xcm_benchmarks::fungible has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3256526 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3256526/artifacts/download.

@command-bot
Copy link

command-bot bot commented Jul 25, 2023

@bkontur Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=xcm --runtime=westend --target_dir=polkadot --pallet=pallet_xcm_benchmarks::fungible has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3256527 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3256527/artifacts/download.

@command-bot
Copy link

command-bot bot commented Jul 25, 2023

@bkontur Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=xcm --runtime=polkadot --target_dir=polkadot --pallet=pallet_xcm_benchmarks::fungible has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3256529 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3256529/artifacts/download.

@bkontur bkontur added D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. and removed D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Jul 25, 2023
@bkontur
Copy link
Contributor Author

bkontur commented Jul 25, 2023

@franciscoaguirre @ggwpez @gilescope @KiChjang
now I see, why there was BenchmarkResult::from_weight(T::BlockWeights::get().max_block) and why it worked probably just by accident, https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3257664

because polkadot/kusama runtime do not support IsReserve, which means they do not process ReserveAssetDeposited instruction,
but pallet_xcm::reserve_transfer_assets estimates remote weight according to local weights and here is problem because neither Kusama nor Polkadot does not support ReserveAssetDeposited (kind of chicken and egg problem)

so how to fix this now?

  1. revert back to use BenchmarkResult::from_weight(T::BlockWeights::get().max_block) ?
  2. find some proper/correct fix?

there is also opened fix for remote weight estimation: #7424

@bkontur
Copy link
Contributor Author

bkontur commented Jul 26, 2023

@franciscoaguirre @ggwpez @gilescope @KiChjang now I see, why there was BenchmarkResult::from_weight(T::BlockWeights::get().max_block) and why it worked probably just by accident, https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3257664

because polkadot/kusama runtime do not support IsReserve, which means they do not process ReserveAssetDeposited instruction, but pallet_xcm::reserve_transfer_assets estimates remote weight according to local weights and here is problem because neither Kusama nor Polkadot does not support ReserveAssetDeposited (kind of chicken and egg problem)

so how to fix this now?

1. revert back to use `BenchmarkResult::from_weight(T::BlockWeights::get().max_block)` ?

2. find some proper/correct fix?

there is also opened fix for remote weight estimation: #7424

the only solution here is to split:

  • runtime real generate weights (e.g. used for xcm execution procession)
  • weights used for remote weight estimation for pallet_xcm::reserve_transfer_assets / teleport_assets

follow-up: #7424

@KiChjang
Copy link
Contributor

KiChjang commented Aug 2, 2023

Benchmark tests are failing and needs to be fixed. Perhaps it's a reason why we use max_block instead of Weight::MAX?

@KiChjang
Copy link
Contributor

so how to fix this now?

1. revert back to use `BenchmarkResult::from_weight(T::BlockWeights::get().max_block)` ?

2. find some proper/correct fix?

I think we should just look at what the average weight is for the instruction on the system chains, and reuse them here since we are likely going to be sending these instructions to them. I hope the weight is high enough on the system chains so that it acts as a ceiling on the instruction weights.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants