-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Test benchmarks with storage info for bags PR #9468
Test benchmarks with storage info for bags PR #9468
Conversation
…path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_balances --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/balances/src/weights.rs --template=./.maintain/frame-weight-template.hbs
…path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_balances --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/balances/src/weights.rs --template=./.maintain/frame-weight-template.hbs
…path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_staking --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/staking/src/weights.rs --template=./.maintain/frame-weight-template.hbs
… shawntabrizi-refactor-benchmarks
…path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_balances --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/balances/src/weights.rs --template=./.maintain/frame-weight-template.hbs
…path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_staking --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/staking/src/weights.rs --template=./.maintain/frame-weight-template.hbs
…us/api (#9319) * moved client code out of primitives * bump ci * Fixup from merge. * Removed unused deps thanks to review feedback * Removing unneeded deps * updating lock file * note about rustfmt * fixed typo to bump ci * Move lonely CacheKeyId to parent * cargo fmt * updating import style * Update docs/STYLE_GUIDE.md Co-authored-by: André Silva <[email protected]> Co-authored-by: André Silva <[email protected]>
… shawntabrizi-refactor-benchmarks
…path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_balances --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/balances/src/weights.rs --template=./.maintain/frame-weight-template.hbs
…path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_balances --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/balances/src/weights.rs --template=./.maintain/frame-weight-template.hbs
/benchmark runtime pallet pallet_staking |
Benchmark Runtime Pallet for branch "zeke-bags-bench-storage-info" with command cargo run --quiet --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_staking --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/staking/src/weights.rs --template=./.maintain/frame-weight-template.hbs Results
|
…path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_staking --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/staking/src/weights.rs --template=./.maintain/frame-weight-template.hbs
.saturating_add(T::DbWeight::get().reads(5 as Weight)) | ||
.saturating_add(T::DbWeight::get().writes(4 as Weight)) | ||
} | ||
// Storage: Staking Bonded (r:1 w:0) | ||
// Storage: Staking Ledger (r:1 w:1) | ||
// Storage: Staking VoterNodes (r:4 w:4) | ||
// Storage: Staking VoterBagFor (r:1 w:1) |
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.
thinking aloud - maybe we can eliminate this read if we store the current bag of the voter in the node
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.
yes let's merge them.
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.
sounds good - just to state the obvious, this will add a u64 (8 bytes) to each node. Storage space wise this should be less than the map since we don't need to store the key (accountId), but I think it means each node will take longer to encode/decode
// Storage: Staking Ledger (r:1 w:0) | ||
// Storage: Staking Bonded (r:1 w:0) | ||
// Storage: Staking VoterBagFor (r:1 w:1) | ||
// Storage: Staking VoterBags (r:2 w:2) |
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 are we touching two voter bags here? is it because the worse case is to assume that the person being rebagged is the head? in that case it makes sense.
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 think its because we always do bag.put()
on the destination bag as well as the source bag. Maybe remove_node
could return bool indicating if the head or tail of the bag was updated. If it is updated then we can do bag.put
, otherwise we can skip the write. Regardless though, we need the 2 reads (unless we modify the nodes to know if they are a head or a tail)
// clear the old bag head/tail pointers as necessary
if let Some(mut bag) = Bag::<T>::get(node.bag_upper) {
bag.remove_node(&node);
bag.put();
} else {
debug_assert!(false, "every node must have an extant bag associated with it");
crate::log!(
error,
"Node for staker {:?} did not have a bag; VoterBags is in an inconsistent state",
node.voter.id,
);
}
// put the voter into the appropriate new bag
let new_idx = notional_bag_for::<T>(weight_of(&node.voter.id));
node.bag_upper = new_idx;
let mut bag = Bag::<T>::get_or_make(node.bag_upper);
bag.insert_node(node);
bag.put();
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.
indeed, we can totally prevent read/write to bags. If you rebag from non-head-tail to non-head-tail, which is arguably the majority of the cases, you do not need to touch any Bag
.
Not important to get done now or here, but would be great if you put a TODO in the code for it.
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.
If you rebag from non-head-tail to non-head-tail, which is arguably the majority of the cases, you do not need to touch any Bag.
I think the insert still needs a read of the bag unless we know ahead of the time the two nodes we are inserting in between.
Also, in the current state we always insert a node as the tail
Since #9373 was merged we can now get the storage read/write data in master, so this is no longer neccesary |
Do not merge; running benchmarks with storage info