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

Repair and extend some benchmarks #5648

Merged
merged 28 commits into from
Apr 24, 2020
Merged

Conversation

shawntabrizi
Copy link
Member

@shawntabrizi shawntabrizi commented Apr 15, 2020

This PR does a deep review with validation logic on some benchmarks to ensure that we are fully testing everything needed to sensibly weight these extrinsics and pallets.

Changes:

  • Handle panics/divide by zero errors when benchmark cli gets passed 0 values
  • Return the pallet name when using "*" syntax

Update Benchmarks for

  • Collective
  • Democracy
  • Timestamp
  • Vesting

Will update Offenses in a separate PR

@shawntabrizi shawntabrizi added the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 15, 2020
@shawntabrizi shawntabrizi added this to the 2.0 milestone Apr 15, 2020
@shawntabrizi shawntabrizi requested a review from gavofyork April 19, 2020 18:52
@shawntabrizi shawntabrizi marked this pull request as ready for review April 19, 2020 18:52
@shawntabrizi shawntabrizi added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Apr 19, 2020
@@ -65,7 +65,7 @@ pub trait Trait<I: Instance=DefaultInstance>: frame_system::Trait {
type Origin: From<RawOrigin<Self::AccountId, I>>;

/// The outer call dispatch type.
type Proposal: Parameter + Dispatchable<Origin=<Self as Trait<I>>::Origin> + From<Call<Self, I>>;
type Proposal: Parameter + Dispatchable<Origin=<Self as Trait<I>>::Origin> + From<frame_system::Call<Self>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

can you elaborate?

Copy link
Member Author

Choose a reason for hiding this comment

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

The original statement allowed us to pass extrinsics from the module as a Proposal. In the benchmark, I opted to use System::remark instead as my generic proposal, so I placed the requirement that I can convert from frame_system::Call to proposal. This trait should always be satisfied for all pallets afaik

Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

Everything looks good,

But for democracy, I didn't looked carefully if the benchmarked actually take into account all possible execution path.

frame/democracy/src/benchmarking.rs Outdated Show resolved Hide resolved
@shawntabrizi
Copy link
Member Author

@thiolliere If things look good, would appreciate a green checkmark here

Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

I approve, about democracy benchmarks I think results will be double check when writing the final weight formula

@shawntabrizi shawntabrizi added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Apr 24, 2020
@gnunicorn gnunicorn merged commit a9c1b75 into master Apr 24, 2020
@shawntabrizi shawntabrizi deleted the shawntabrizi-fix-benchmarks branch May 11, 2020 10:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants