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.

Sign up for GitHub

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

Jump to bottom

fix: update benchmarks #790

Merged
merged 15 commits into from
Nov 18, 2024
Merged

fix: update benchmarks #790

merged 15 commits into from
Nov 18, 2024

Conversation

ntn-x2
Copy link
Member

@ntn-x2 ntn-x2 commented Nov 6, 2024 •
edited
Loading

Part of https://github.com/KILTprotocol/ticket/issues/3650, built on top of #787.

What started as a simple fix for the web3name pallet benchmarks, turned out to be a bigger changeset due to a couple of bugs found in the Substrate code, which are mentioned in the self-review I gave myself below.

In a gist, what I did is:

  1. Update the benchmarking logic for the web3name pallet to delegate the name creation to a BenchmarkHelper type.
  2. Implement the trait above for both web3names (which uses the old implementation) and the dotnames
  3. Work around the Substrate bug for our pallets as well as the pallet_collective and pallet_membership pallets which we deploy multiple times
  4. Add the benchmarking of the pallet_membership deployments of the TipsMembership pallet

@ntn-x2 ntn-x2 self-assigned this Nov 6, 2024
@ntn-x2 ntn-x2 force-pushed the aa/update-benchmarks branch 2 times, most recently from f123833 to 63bf5eb Compare November 7, 2024 12:17
ntn-x2
ntn-x2 commented Nov 11, 2024
View reviewed changes
runtimes/peregrine/src/lib.rs Show resolved Hide resolved
runtimes/peregrine/src/lib.rs

frame_support::parameter_types! {
pub const MaxBalance: crate::Balance = crate::Balance::max_value();
}

/// Workaround for a bug in the benchmarking code around instances.
/// Upstream fix PR: https://github.com/paritytech/polkadot-sdk/pull/6435
#[allow(unused_imports)]
Copy link
Member Author

Choose a reason for hiding this comment

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

See paritytech/polkadot-sdk#6435 for the issue and the fix.

runtimes/peregrine/src/lib.rs
[pallet_collective, Council]
[pallet_technical_committee_collective, TechnicalCommittee]
// pallet_membership instances
[pallet_membership, TipsMembership]
Copy link
Member Author

Choose a reason for hiding this comment

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

We didn't run benchmarks for this instance at all before.

runtimes/peregrine/src/weights/pallet_dot_names.rs Show resolved Hide resolved
@ntn-x2 ntn-x2 marked this pull request as ready for review November 11, 2024 13:54
@ntn-x2 ntn-x2 requested a review from Ad96el November 11, 2024 13:54
This was referenced Nov 11, 2024
Closed
Merged
@Ad96el
Copy link
Member

Ad96el commented Nov 15, 2024

I looked into the benchmark error Pr on the polkadot-sdk, but I am struggling to understand what the underlying issue is. For pallets with multiple instances in the runtime, the benchmarking was executed for each instance with all different config configuration?

Ad96el
Ad96el reviewed Nov 15, 2024
View reviewed changes
Copy link
Member

@Ad96el Ad96el left a comment

Choose a reason for hiding this comment

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

two minior questions

runtimes/peregrine/src/lib.rs Show resolved Hide resolved
runtimes/peregrine/src/weights/pallet_dot_names.rs Show resolved Hide resolved
@ntn-x2
Copy link
Member Author

ntn-x2 commented Nov 15, 2024

I looked into the benchmark error Pr on the polkadot-sdk, but I am struggling to understand what the underlying issue is. For pallets with multiple instances in the runtime, the benchmarking was executed for each instance with all different config configuration?

That is exactly the issue. Plus, a related error and a somewhat consequence of this is that the calculated weights are actually the interpolation of the results combined, for each instance, that is why we had to rename the pallet of each instance, so that one instance run would not interfere with the others. This is a pretty serious bug in Substrate, and it should have been caught way way earlier.

@Ad96el
Copy link
Member

Ad96el commented Nov 15, 2024

That is exactly the issue. Plus, a related error and a somewhat consequence of this is that the calculated weights are actually the interpolation of the results combined, for each instance, that is why we had to rename the pallet of each instance, so that one instance run would not interfere with the others. This is a pretty serious bug in Substrate, and it should have been caught way way earlier.

I see thanks for the conformation

@ntn-x2 ntn-x2 force-pushed the aa/update-benchmarks branch from bc7c458 to 56d14e0 Compare November 15, 2024 13:09
@ntn-x2 ntn-x2 merged commit af07f5a into develop Nov 18, 2024
15 of 16 checks passed
@ntn-x2 ntn-x2 deleted the aa/update-benchmarks branch November 18, 2024 07:50
ntn-x2 added a commit that referenced this pull request Nov 20, 2024
@ntn-x2
chore: refactor runtimes code (#797)
b1039bb 
WIP. Fixes KILTprotocol/ticket#3672. Built on
top of #790.

## Checklist
- [x] Review Peregrine code
- [x] Ask for review
- [x] Apply to Spiritnet code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ad96el Ad96el Ad96el approved these changes

Assignees

@ntn-x2 ntn-x2

@Ad96el Ad96el

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ntn-x2 @Ad96el