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

Add overhead benchmark to frame-omni-bencher #5891

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

skunert
Copy link
Contributor

@skunert skunert commented Oct 1, 2024

Benchmark Overhead Command for Parachains

This implements the benchmark overhead command for parachains. Full context is available at: #5303

Changes Overview

Users are now able to use frame-omni-bencher to generate extrinsic_weight.rs and block_weight.rs files for their runtime. The core logic for generating these remains untouched; this PR provides mostly machinery to make it work for parachains at all.

Similar to the pallet benchmarks, we gain the option to benchmark based on just a runtime:

frame-omni-bencher v1 benchmark overhead --runtime {{runtime}} --genesis-builder runtime;

In this case, the genesis state is generated from the runtime presets. However, it is also possible to use --chain and genesis builder spec to generate the genesis state from the chain spec. Using --generate-num-accounts, we can inject fresh accounts into the genesis state. --account-type can be used to choose between ECDSA and Sr25519 accounts.

Additionally, we use metadata to perform some checks based on the pallets the runtime exposes:

  • If we see the ParaInherent pallet, we assume that we are dealing with a relay chain. This means that we don't need proof recording during import (since there is no storage weight).
  • If we detect the ParachainSystem pallet, we assume that we are dealing with a parachain and take corresponding actions like patching a para id into the genesis state.

On the inherent side, I am currently supplying the standard inherents every parachain needs.

In the current state, frame-omni-bencher supports all system chains. In follow-up PRs, we could add additional inherents to increase compatibility.

Since we are building a block during the benchmark, we also need to build an extrinsic. By default, I am leveraging subxt to build the xt dynamically. If a chain is not compatible with the SubstrateConfig that comes with subxt, it can provide a custom extrinsic builder to benchmarking-cli. This requires either a custom bencher implementation or an integration into the parachains node.

TBD:

  • PRDoc
  • Validate account generation approach

@skunert skunert added T0-node This PR/Issue is related to the topic “node”. T12-benchmarks This PR/Issue is related to benchmarking and weights. labels Oct 1, 2024
log::debug!("{}", input_value);
}

if let Some(num_accounts) = num_accounts {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My first hunch in #5303 was to generate accounts to inject into the genesis state. Happy to hear some opinions about this approach.

Main problem is that the number of accounts we can inject here is limited due to #5419. As an alternative, we could attempt to patch the raw storage or alternatively, not inject anything at all and extrapolate.

Copy link
Member

@ggwpez ggwpez Oct 1, 2024

Choose a reason for hiding this comment

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

You only need to inject one account and can then send transfers to fund other accounts. Not sure how you build transactions, but subxt should also be able to do that?

@ggwpez
Copy link
Member

ggwpez commented Oct 1, 2024

Thanks for picking this up!

In the current state, frame-omni-bencher supports all system chains. In follow-up PRs, we could add additional inherents to increase compatibility.

You can look at the try-runtime CLI if you get stuck with the inherent stuff: https://github.com/paritytech/try-runtime-cli/tree/main/core/src/common/empty_block/inherents, as it is also building blocks.

Using --generate-num-accounts, we can inject fresh accounts into the genesis state

I think the old overhead benchmark ran with an actual client, so that it can also measure disk access time. If you insert the account like this, then they will be held in memory, right? Maybe you can also count the read/writes and we factor them out like we do for the normal weights.
Or you open an actual client directory with ParityDb / RocksDb and read from that.

By default, I am leveraging subxt to build the xt dynamically

Huh nice. I did not think about this at all, and would have created a new runtime API, but that is annoying and needs integration.

One more thing i want to throw in is that it is easy to integrate state snapshots to run it on real data. They are just KV pairs and can be read from disk like this: https://github.com/ggwpez/pdu/blob/5b3556689f4c934c160ca440580f6026f810a156/src/main.rs#L473 . We already have those cached in the SDK CI and use them for the migration checks.

@skunert
Copy link
Contributor Author

skunert commented Oct 1, 2024

I think the old overhead benchmark ran with an actual client, so that it can also measure disk access time. If you insert the account like this, then they will be held in memory, right? Maybe you can also count the read/writes and we factor them out like we do for the normal weights.
Or you open an actual client directory with ParityDb / RocksDb and read from that.

Currently this is already opening a proper client with a RocksDb database (new temporary directory per run). To be independent of the runtimes, I am leveraging the same FakeRuntimeApi strategy that we use in the omni-node.

Huh nice. I did not think about this at all, and would have created a new runtime API, but that is annoying and needs integration.

What would that runtime API do? Return a valid transaction? I was discussing some options with @michalkucharczyk, something like this was also on the table. I wanted to try the subxt approach first, and seems to work reasonably well. What is a bit annoying is that you currently have to specify these configs manually, like the SubstrateConfig here:

impl ExtrinsicBuilder for DynamicRemarkBuilder<SubstrateConfig> {

However, in the future we should be able to derive these configs from Metadata like this PR implements.

One more thing i want to throw in is that it is easy to integrate state snapshots to run it on real data. They are just KV pairs and can be read from disk like this: https://github.com/ggwpez/pdu/blob/5b3556689f4c934c160ca440580f6026f810a156/src/main.rs#L473 . We already have those cached in the SDK CI and use them for the migration checks.

That should not be too hard. We can feed the client with whatever storage we want.

You can look at the try-runtime CLI if you get stuck with the inherent stuff: https://github.com/paritytech/try-runtime-cli/tree/main/core/src/common/empty_block/inherents, as it is also building blocks.

Will take a look at this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”. T12-benchmarks This PR/Issue is related to benchmarking and weights.
Projects
Status: backlog
Development

Successfully merging this pull request may close these issues.

2 participants