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

frame-benchmarking-cli: Remove native dispatch requirement #14474

Merged
merged 4 commits into from
Jun 29, 2023

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Jun 28, 2023

No need for this, we can just use the WasmExecutor directly.

Changes

Removes the requirement to pass the ExecDispatch generic parameter and instead to pass the host functions directly. If you don't want to add any extra host functions, you can just pass () to PalletCmd::run.

polkadot companion: paritytech/polkadot#7434
cumulus companion: paritytech/cumulus#2792

No need for this, we can just use the `WasmExecutor` directly.
@bkchr bkchr added A0-please_review Pull request needs code review. 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 B1-note_worthy Changes should be noted in the release notes T0-node This PR/Issue is related to the topic “node”. labels Jun 28, 2023
@bkchr bkchr requested a review from ggwpez June 28, 2023 08:01
@bkchr bkchr requested review from a team June 28, 2023 08:24
@bkchr bkchr requested a review from a team June 28, 2023 08:24
@bkchr bkchr requested a review from koute as a code owner June 28, 2023 08:24
Copy link
Contributor

@sam0x17 sam0x17 left a comment

Choose a reason for hiding this comment

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

LGTM, few minor doc tweaks

/// execution engine as a preprocessing step and therefore don't yield a meaningful benchmark
/// result. However, in contrast to the instructions mentioned in 2. they can be spammed. We
/// price them with the same weight as the "default" instruction (i64.const): Block, Loop, Nop
/// 4. We price both i64.const and drop as InstructionWeights.i64const / 2. The reason for that is
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// 4. We price both i64.const and drop as InstructionWeights.i64const / 2. The reason for that is
/// 4. We price both `i64.const` and drop as `InstructionWeights.i64.const` / 2. The reason for that is

/// 3. The following instructions cannot be benchmarked because they are removed by any real world
/// execution engine as a preprocessing step and therefore don't yield a meaningful benchmark
/// result. However, in contrast to the instructions mentioned in 2. they can be spammed. We
/// price them with the same weight as the "default" instruction (i64.const): Block, Loop, Nop
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// price them with the same weight as the "default" instruction (i64.const): Block, Loop, Nop
/// price them with the same weight as the "default" instruction (`i64.const`): Block, Loop, Nop

@bkchr bkchr merged commit 444bc4f into master Jun 29, 2023
@bkchr bkchr deleted the bkchr-benchmarking-cli-remove-native-dispatch branch June 29, 2023 15:56
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
…h#14474)

* frame-benchmarking-cli: Remove native dispatch requirement

No need for this, we can just use the `WasmExecutor` directly.

* Fixes

* Pass benchmarking host functions

* Ensure we can pass custom host functions
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. B1-note_worthy Changes should be noted in the 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 T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants