try making bench numbers make sense #5526
Conversation
ordian
added
the
T12-benchmarks
|
bot bench polkadot-pallet --pallet=polkadot_runtime_parachains::inclusion --runtime=westend |
|
/cmd bench --help |
|
/cmd bench --runtime westend rococo --pallet=polkadot_runtime_parachains::inclusion |
…=westend --target_dir=polkadot --pallet=polkadot_runtime_parachains::inclusion
…=rococo --target_dir=polkadot --pallet=polkadot_runtime_parachains::inclusion
|
bot bench polkadot-pallet --pallet=polkadot_runtime_parachains::inclusion --runtime=westend |
…=westend --target_dir=polkadot --pallet=polkadot_runtime_parachains::inclusion
|
bot bench polkadot-pallet --pallet=polkadot_runtime_parachains::inclusion --runtime=westend |
|
/cmd bench --runtime westend rococo --pallet=polkadot_runtime_parachains::inclusion |
|
Command "bench --runtime westend rococo --pallet=polkadot_runtime_parachains::inclusion" has started 🚀 See logs here |
…=westend --target_dir=polkadot --pallet=polkadot_runtime_parachains::inclusion
|
@ordian Command |
|
Command "bench --runtime westend rococo --pallet=polkadot_runtime_parachains::inclusion" has failed ❌! See logs here |
| doc: | ||
| - audience: Runtime Dev | ||
| description: | | ||
| This PR works around an issue in multivariate linear regression of weight generation. |
There was a problem hiding this comment.
Is there an issue for this and shall we reference it from the benchmarks, so the values can be changed back once fixed?
There was a problem hiding this comment.
I'll need to dig a bit deeper to provide meaningful feedback for the issue, but I don't think there's a need to change those values back once fixed. The purpose of linear regression is to find the right correlation and if it can be achieved with just a points, there is no need to add more.
bkontur
added
the
A4-needs-backport
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-5526-to-stable2407
git worktree add --checkout .worktree/backport-5526-to-stable2407 backport-5526-to-stable2407
cd .worktree/backport-5526-to-stable2407
git reset --hard HEAD^
git cherry-pick -x de0b6f25725325db0e97379f2d49a41dc8a13c44
git push --force-with-lease |
|
Successfully created backport PR for |
|
Successfully created backport PR for |
Backport #5526 into `stable2409` from ordian. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> Co-authored-by: ordian <write@reusable.software>
Backport #5526 into `stable2412` from ordian. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> Co-authored-by: ordian <write@reusable.software>
## Description Addressing @ordian's [comment](#522 (review)): > Previously, the weight of `enact_candidate` was included in the weight of availability bitfields, but this was a bug, because the number of bitfields depends on number of validators, whereas number of enacted candidates depends on the number of cores. This was later (accidentally) fixed, but the `enact_candidate` weight was unaccounted for. [paritytech/polkadot-sdk#5270](paritytech/polkadot-sdk#5270) was supposed to fix that. > > The base weight of `enact_candidate` still looks excessive and it was fixed in [paritytech/polkadot-sdk#5526](paritytech/polkadot-sdk#5526), but looks like it didn't make into the same release. > > That being said, even with 1.5ms of enactment weight per candidate, this shouldn't be a blocker to scale to 200 parachains weight-wise. So I'm fine with having these excessive weights for now. Addressing @bkontur's [comment](#522 (comment)): > @ordian The next patch, **stable2409-4**, is planned for this week. I have backported your fix here: [paritytech/polkadot-sdk#7145](paritytech/polkadot-sdk#7145). Once it is released, I will re-run the benchmarks for this within another PR. ## Weights diff ### Kusama `--method asymptotic` does not work: ``` subweight compare commits --path-pattern "./relay/kusama/**/weights/**/*.rs" --format markdown --no-color --change added changed --method asymptotic --ignore-errors --strip-path-prefix="relay/kusama/src/weights/" remotes/polkadot-fellows/main origin/bko-patch-parachains-inclusion ``` | File | Extrinsic | Old | New | Change [%] | |---------------------------------|-----------------|-----|-----|------------| | runtime_parachains_inclusion.rs | enact_candidate | - | - | ERROR | but `--method base` works: ``` subweight compare commits --path-pattern "./relay/kusama/**/weights/**/*.rs" --format markdown --no-color --change added changed --method base --ignore-errors --strip-path-prefix="relay/kusama/src/weights/" remotes/polkadot-fellows/main origin/bko-patch-parachains-inclusion ``` | File | Extrinsic | Old | New | Change [%] | |---------------------------------|-----------------|--------|----------|------------| | runtime_parachains_inclusion.rs | enact_candidate | 2.97ms | 939.10us | -68.35 | ### Polkadot `--method asymptotic` does not work: ``` subweight compare commits --path-pattern "./relay/polkadot/**/weights/**/*.rs" --format markdown --no-color --change added changed --method asymptotic --ignore-errors --strip-path-prefix="relay/polkadot/src/weights/" remotes/polkadot-fellows/main origin/bko-patch-parachains-inclusion ``` | File | Extrinsic | Old | New | Change [%] | |---------------------------------|-----------------|-----|-----|------------| | runtime_parachains_inclusion.rs | enact_candidate | - | - | ERROR | but `--method base` works: ``` subweight compare commits --path-pattern "./relay/polkadot/**/weights/**/*.rs" --format markdown --no-color --change added changed --method base --ignore-errors --strip-path-prefix="relay/polkadot/src/weights/" remotes/polkadot-fellows/main origin/bko-patch-parachains-inclusion ``` | File | Extrinsic | Old | New | Change [%] | |---------------------------------|-----------------|--------|----------|------------| | runtime_parachains_inclusion.rs | enact_candidate | 2.92ms | 974.24us | -66.60 | <!-- Remember that you can run `/merge` to enable auto-merge in the PR --> <!-- Remember to modify the changelog. If you don't need to modify it, you can check the following box. Instead, if you have already modified it, simply delete the following line. --> - [X] Does not require a CHANGELOG entry --------- Co-authored-by: fellowship-merge-bot[bot] <151052383+fellowship-merge-bot[bot]@users.noreply.github.com>
Follow-up to #5270. The baseline numbers for Westend were too high to be representative of the reality as it seemed to do with how multi-variate linear regression is calculated.