-
Notifications
You must be signed in to change notification settings - Fork 857
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
Clamp benchmark ranks to respect MaxRank #7720
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The general logic looks good, just some nit improvements to make this better :)
let r = r.try_into().expect("r is too large"); | ||
// Get target rank for promotion. | ||
let max_rank = T::MaxRank::get(); | ||
let target_rank = r.min(max_rank).try_into().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let target_rank = r.min(max_rank).try_into().unwrap(); | |
let target_rank = r.min(max_rank); |
r
and max_rank
are already a u32
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should change MaxRank
to u16
. I don't get why ranks are u16
and MaxRank
is u32
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub struct ConvertU16ToU32<Inner>(PhantomData<Inner>);
for BoundedVec
let member = make_member::<T, I>(0)?; | ||
log::info!("Benchmark promote_fast: r={} MaxRank={}", r, T::MaxRank::get()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log::info!("Benchmark promote_fast: r={} MaxRank={}", r, T::MaxRank::get()); |
let max_rank: RankOf<T, I> = T::MaxRank::get().try_into().unwrap(); | ||
let initial_rank = max_rank.saturated_into::<u16>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we can remove all the conversion code.
@@ -523,7 +533,7 @@ pub mod pallet { | |||
/// This is useful for out-of-band promotions, hence it has its own `FastPromoteOrigin` to | |||
/// be (possibly) more restrictive than `PromoteOrigin`. Note that the member must already | |||
/// be inducted. | |||
#[pallet::weight(T::WeightInfo::promote_fast(*to_rank as u32))] | |||
#[pallet::weight(T::WeightInfo::promote_fast(*to_rank))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would need to run benchmark command for this 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Generally we have now these weight updates once a week, that should be fine to be done as part of them.
@@ -523,7 +533,7 @@ pub mod pallet { | |||
/// This is useful for out-of-band promotions, hence it has its own `FastPromoteOrigin` to | |||
/// be (possibly) more restrictive than `PromoteOrigin`. Note that the member must already | |||
/// be inducted. | |||
#[pallet::weight(T::WeightInfo::promote_fast(*to_rank as u32))] | |||
#[pallet::weight(T::WeightInfo::promote_fast(*to_rank))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Generally we have now these weight updates once a week, that should be fine to be done as part of them.
@@ -163,6 +163,13 @@ impl< | |||
} | |||
} | |||
|
|||
pub struct ConvertU16ToU32<Inner>(PhantomData<Inner>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine for now. After the pr got merged, could you do a commit to the upstream crate that has the Get
trait to introduce some sort of GetInto
converter type:
impl<T: Get<I>, I: Into<R>, R> Get<R> GetInto<I> {
fn get() -> R {
T::get().into()
}
}
resolves #7517
Issues
1. Compile-Time vs Runtime Rank Mismatch
promote_fast
Benchmark uses mock runtime'sMaxRank
during benchmark generation while allowing runtime overrides. Creating potential parameter mismatch between benchmark metadata(i.e. generatedr
values) and runtime configuration(i.e.T::MaxRank
).2. Static Rank Assumptions
bump_demote
Benchmark initialized members at rank 2, making it incompatible withMaxRank=1
configurationspromote
Benchmark contained hardcoded rank values (1 → 2) which fails forMaxRank=1
Changes
Dynamic Rank Clamping