-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Fix weights for backed candidates #6929
base: master
Are you sure you want to change the base?
Conversation
bot bench $ runtime kusama runtime_parachains::paras_inherent |
@eskimor |
bot cancel |
@eskimor Command |
bot bench-all $ polkadot-all kusama |
@eskimor https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/2567452 was started for your command Comment |
@@ -116,7 +123,6 @@ impl<T: paras_inherent::Config> BenchBuilder<T> { | |||
dispute_statements: BTreeMap::new(), | |||
dispute_sessions: Default::default(), | |||
backed_and_concluding_cores: Default::default(), | |||
code_upgrade: None, |
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.
Is now part of the candidates themselves.
@@ -221,8 +220,9 @@ impl<T: paras_inherent::Config> BenchBuilder<T> { | |||
|
|||
/// Get the minimum number of validity votes in order for a backed candidate to be included. | |||
#[cfg(feature = "runtime-benchmarks")] | |||
pub(crate) fn fallback_min_validity_votes() -> u32 { | |||
(Self::fallback_max_validators() / 2) + 1 | |||
pub(crate) fn fallback_min_backing_votes() -> 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.
Changed name, because backing votes is less ambiguous than validity_votes.
@@ -895,7 +895,7 @@ fn apply_weight_limit<T: Config + inclusion::Config>( | |||
let (acc_candidate_weight, indices) = | |||
random_sel::<BackedCandidate<<T as frame_system::Config>::Hash>, _>( | |||
rng, | |||
candidates.clone(), | |||
&candidates, |
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.
Not sure why this was cloned. Can be quite expensive for candidates.
@eskimor Command |
ef02aab
to
44b7162
Compare
bot bench $ runtime kusama runtime_parachains::paras_inherent |
"$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" runtime kusama runtime_parachains::paras_inherent was queued. Comment |
@eskimor Command
|
…ains::paras_inherent
@eskimor Command |
bot bench $ runtime westend runtime_parachains::paras_inherent |
@eskimor Exception caught in webhook handler |
bot bench $ runtime westend runtime_parachains::paras_inherent |
@eskimor Exception caught in webhook handler |
bot bench $ runtime westend runtime_parachains::paras_inherent |
@eskimor Exception caught in webhook handler |
bot bench $ runtime westend runtime_parachains::paras_inherent |
@eskimor Exception caught in webhook handler |
bot bench $ runtime westend runtime_parachains::paras_inherent |
@mordamax https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/2737646 was started for your command Comment |
@mordamax https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/2737647 was started for your command Comment |
@mordamax https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/2737648 was started for your command Comment |
@mordamax Command |
@mordamax Command |
…ains::paras_inherent
@mordamax Command |
bot bench $ runtime polkadot runtime_parachains::paras_inherent |
@eskimor https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/2741604 was started for your command Comment |
@eskimor https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/2741605 was started for your command Comment |
@eskimor Command |
@eskimor Command |
In particular take into account ump and hrmp messages.
Update:
This does not work. Benchmarks are all over the place. This PR would result in a massive overestimate of the actual weight. In reality there is not much computation done based on these parameters, it is mostly block size that is affected - which is not measured by weight.
In particular the constant term gets massively over estimated and the linear terms also fluctuate a lot from run to run.
Anyhow, given that the constant term changes by a factor of 41 based on the upper bound of those linear parameters, some significant effect on weight seems to be there regardless - it likely is not linear though and the current benchmarking system has trouble providing meaningful/useful results.
For now limiting candidates based on size (to be done) will need to suffice.
Can hopefully be revived once paritytech/polkadot-sdk#194 is done.
With this in place as well we can also properly track size via v2 weights.
Another Update and Plan forward
Benchmarks are still bonkers, but I managed to get acceptable values (not too crazy high for the base case). Constant factors are mostly ignored now for CPU time, this should be acceptable if we limit by size.
Plan: Get size limiting, then merge this.