-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Make election benchmarks more *memory-aware* #9286
Conversation
/benchmark pallet pallet_election_provider_multi_phase |
Starting benchmark for branch: kiz-mem-aware-benchmarks (vs master) Comment will be updated. |
/benchmark pallet pallet_election_provider_multi_phase |
Starting benchmark for branch: kiz-mem-aware-benchmarks (vs master) Comment will be updated. |
/benchmark runtime pallet pallet_election_provider_multi_phase |
Starting benchmark for branch: kiz-mem-aware-benchmarks (vs master) Comment will be updated. |
/benchmark runtime pallet pallet_election_provider_multi_phase |
Benchmark Runtime Pallet for branch "kiz-mem-aware-benchmarks" with command cargo run --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_election_provider_multi_phase --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/election-provider-multi-phase/src/weights.rs --template=./.maintain/frame-weight-template.hbs Results
|
… kiz-mem-aware-benchmarks
…/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_election_provider_multi_phase --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/election-provider-multi-phase/src/weights.rs --template=./.maintain/frame-weight-template.hbs
.saturating_add((1_957_000 as Weight).saturating_mul(a as Weight)) | ||
// Standard Error: 18_000 | ||
.saturating_add((588_000 as Weight).saturating_mul(d as Weight)) | ||
.saturating_add(T::DbWeight::get().reads(6 as Weight)) |
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.
extra read is expected, we need to read snapshot_metadata
now as well.
/// The numbers configured here should always be more than the the maximum limits of staking pallet | ||
/// to ensure election snapshot will not run out of memory. |
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.
I dont quite see how we can ensure this is always the case given the variability on chain.
I get if this is a temp solution, but i think we need to be more careful than to just add this code and forget about it later.
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.
Actually, this comment no longer holds exactly. Given #9285, I had to #[ignore]
these benchmarks again, so they are not always executed (which was my intention). My intention was that we always run them as we grow these parameters and it would automatically prevent us from making them too big.
Currently I am benchmarking only create_snapshot
. If we do the same with on_initialize
(which is only a smidgen different), and if it works here, I think then we can be pretty sure that the same success will hold on-chain as well.
how we can ensure this is always the case given the variability on chain.
Which part of it doesn't add up, and what vulnerability you mean?
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 word was variability, that being that this 25_000
value can be changed on-chain, and nothing is really updating the value here in that case.
If we have some hardcoded value here, maybe we need to pass that hard-coded value through to the pallet and make sure the on-chain value is not set higher than this hard-coded value.
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.
since staking's 25_000 is a storage item and it can be changed on the fly, this is not possible. This will be some manual process that we run to ensure some new value that we might want to set in staking's storage is safe.
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.
My thought is that we can also place a limit to the value set in storage, so it does not reach past any limit we have not prepared benchmarks for
// ONLY run this benchmark in isolation, and pass the `--extra` flag to enable it. | ||
#[extra] | ||
create_snapshot_memory { |
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.
do we have some process that will execute this when things change? or just internal knowledge
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 yet, internal knowledge.
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 changes look okay, but i worry that all of this will be forgotten until it bites us again
given that the onchain limits are storage item and this is not, relying on this benchmark to set the boundaries of heap-safety will have to be manual. Before setting any value to polkadot, we should re-run these benchmarks with the new numbers to get insight. I think this, next to westend being populated more than polkadot, is as good as it gets for now. |
bot merge |
Waiting for commit status. |
Merge aborted: Checks failed for 06982f6 |
bot merge |
Waiting for commit status. |
Merge aborted: Checks failed for 1d2d10e |
bot merge |
Waiting for commit status. |
Merge failed: "Required status check "continuous-integration/gitlab-check-polkadot-companion-build" is failing." |
bot merge |
Trying merge. |
Related to #9285
For now I added two new benchmarks. They can only be executed in isolation, and if they work, then it is good news.
polkadot companion: paritytech/polkadot#3443