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

Add overhead benchmark to parachain-template #2858

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

Conversation

kalaninja
Copy link

Adds benchmark overhead that was missing in the previous PR #1156

@kalaninja kalaninja requested review from ggwpez and bkchr July 12, 2023 11:44
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Is there a test to check that the new command works?
Fot the storage benchmark there seems to be one for the asset hubs:

.args(["benchmark", "storage", "--chain", runtime])

parachain-template/node/src/benchmarking.rs Show resolved Hide resolved
@kalaninja
Copy link
Author

kalaninja commented Jul 12, 2023

Fot the storage benchmark there seems to be one for the asset hubs:

This test is from polkadot-parachain. I decided to split this into 2 PRs. So the overhead benchmark and the test for collator will come in a different PR.

Is there a test to check that the new command works?

I tested this one manually with cargo run -p parachain-template-node -- benchmark overhead

@kalaninja kalaninja requested a review from ggwpez July 12, 2023 13:25
@kalaninja
Copy link
Author

@ggwpez the test was added in #2900

@kalaninja kalaninja requested a review from bkontur July 19, 2023 11:57
@bkontur
Copy link
Contributor

bkontur commented Jul 19, 2023

@kalaninja could we run benchbot here to see that it works? maybe for parachain-template it is not configured

@ggwpez
Copy link
Member

ggwpez commented Jul 19, 2023

@kalaninja could we run benchbot here to see that it works? maybe for parachain-template it is not configured

We can run it for the asset hubs over here once its merged and the bench bot is updated: #2900.
The Bot does not care about templates, since they dont need correct weights.

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

LGTM (tested it locally).

We could still add an integration test as good measure to ensure that we dont break it in the future, but its also fine like this 😄

You can enable it with `--features runtime-benchmarks`."
.into())
Copy link
Member

Choose a reason for hiding this comment

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

Also here the Question; why not use the cfg! to make it look more compact instead of the cfg(not...?

Copy link
Author

@kalaninja kalaninja Jul 19, 2023

Choose a reason for hiding this comment

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

the only reason is that it is done with attributes in all other places. So, just didn't want to mix multiple approaches.

@ggwpez ggwpez added B0-silent Changes should not be mentioned in any release notes 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 labels Jul 19, 2023
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.

4 participants