Bags-list on_idle: per-item weight consumption via WeightMeter#11594
Bags-list on_idle: per-item weight consumption via WeightMeter#11594
Conversation
… values The benchmark failed with MaxAutoRebagPerBlock=5 because: - pending_count = 5/3 = 1, and the i%7 score skip hit the only pending node - the scoreless node took the cheapest rebag_internal path (early error return) while still consuming budget, leaving fewer successful rebags than expected - the exact-count assertion broke since it also counted pre-existing nodes in higher bags, making the result depend on the budget-to-distribution ratio Fix by placing all regular nodes in the lowest bag and setting scores for all pending nodes. This makes the count deterministic (only on_idle moves nodes into higher bags) and exercises worst-case weight (every node triggers a full rebag). Tested with MaxAutoRebagPerBlock = 3, 5, and 10.
sigurpol
added
the
T12-benchmarks
|
/cmd prdoc --audience runtime_dev --bump none |
|
/cmd bench --pallet pallet_bags_list --runtime asset-hub-westend |
|
Command "bench --pallet pallet_bags_list --runtime asset-hub-westend" has started 🚀 See logs here |
…time_dev --bump none'
Exercises the benchmark setup with MaxAutoRebagPerBlock = 1, 2, 3, 5, 7, 10, 15, 20 to ensure deterministic results regardless of budget value.
…t_bags_list --runtime asset-hub-westend'
|
Command "bench --pallet pallet_bags_list --runtime asset-hub-westend" has finished ✅ See logs here DetailsSubweight results:
Command output:✅ Successful benchmarks of runtimes/pallets: |
| // Ensure we have at least three bags populated before rebag | ||
| assert!(List::<T, _>::get_bags().len() >= 2); | ||
| // All regular nodes are in the lowest bag; pending nodes are not yet in the list. | ||
| assert_eq!(List::<T, _>::get_bags().len(), 1); |
There was a problem hiding this comment.
Seems correct, but wondering why was the old test set up that way? Is having multiple bags worse than having one bag for this benchmark? Same question about simulating the cleanup scenario.
There was a problem hiding this comment.
Starting all nodes in a single (lowest) bag is actually the worst case for rebag_internal when it hits the (true, Some(current_score)) path at https://github.com/paritytech/polkadot-sdk/blob/sigurpol-bag-list-better-on-idle-benchmark/substrate/frame/bags-list/src/lib.rs#L639-L639 and do_rebag has to:
- Nuke the node from the old bag (update prev/next pointers, possibly
update bag head/tail) - Insert it into a new, possibly not-yet-existing destination bag
If nodes were already spread across multiple bags, some would already be in their correct bag and do_rebag would be a no-op (score matches current bag).
The point here is to have a worst case scenario but also something robust meaning not depending by MaxAutoRebagPerBlock value and something where we can exact assertion and not >= as before
There was a problem hiding this comment.
About the former cleanup scenarios, I think it was not an accurate comment either - the benchmark doesn't simulate one or any corruption in the (unchanged) lines of code https://github.com/paritytech/polkadot-sdk/blob/sigurpol-bag-list-better-on-idle-benchmark/substrate/frame/bags-list/src/benchmarks.rs#L380-L382
List::remove fully removes the node from both the bag and ListNodes. After removal, these nodes won't appear in Self::iter() (which iterates the linked list via bags) , in ListNodes storage or in PendingRebag - but it's just a proper cleanup. There is no orphan. I think we can just remove these cleanup lines imho
| let regular_count = rebag_budget + 5; | ||
|
|
||
| // Insert regular nodes with varying scores | ||
| // Insert all regular nodes into the lowest bag. This is worst-case: every node |
There was a problem hiding this comment.
Dumb question: wouldn't it be better if this was benching weight of one worse case rebag, and then on_idle can consume items * weight_one_rebag? So we only need to bench for one item and it does not change based on MaxAutoRebagPerBlock?
There was a problem hiding this comment.
I don't think it's a dumb question at all - I think it's actually a better design 😄
There was a problem hiding this comment.
and I could get rid of rebags_exactly_budget_nodes_regardless_of_budget_size - lemme
|
/cmd bench --pallet pallet_bags_list --runtime asset-hub-westend |
|
/cmd bench --pallet pallet_bags_list --runtime dev |
|
Command "bench --pallet pallet_bags_list --runtime asset-hub-westend" has started 🚀 See logs here |
|
Command "bench --pallet pallet_bags_list --runtime dev" has started 🚀 See logs here |
|
/cmd bench --pallet pallet_bags_list --runtime pallet-staking-async-parachain-runtime |
|
Command "bench --pallet pallet_bags_list --runtime pallet-staking-async-parachain-runtime" has started 🚀 See logs here |
|
/cmd bench --pallet pallet_bags_list --runtime westend |
|
Command "bench --pallet pallet_bags_list --runtime westend" has started 🚀 See logs here |
|
Command "bench --pallet pallet_bags_list --runtime pallet-staking-async-parachain-runtime" has failed ❌! See logs here |
…t_bags_list --runtime asset-hub-westend'
|
Command "bench --pallet pallet_bags_list --runtime asset-hub-westend" has finished ✅ See logs here DetailsSubweight results:
Command output:✅ Successful benchmarks of runtimes/pallets: |
…t_bags_list --runtime westend'
|
Command "bench --pallet pallet_bags_list --runtime westend" has finished ✅ See logs here DetailsSubweight results:
Command output:✅ Successful benchmarks of runtimes/pallets: |
…t_bags_list --runtime dev'
|
Command "bench --pallet pallet_bags_list --runtime dev" has finished ✅ See logs here DetailsSubweight results:
Command output:✅ Successful benchmarks of runtimes/pallets: |
|
I suggest also updating the PR's description to include the fact that you now benchmark only a single worst-case rebag and the weight no longer depends on |
sigurpol
changed the title
Good point, done |
| assert_ok!(List::<T, _>::insert(node.clone(), score)); | ||
| } | ||
| let origin_bag_thresh = T::BagThresholds::get()[0]; | ||
| let dest_bag_thresh = T::BagThresholds::get()[1]; |
There was a problem hiding this comment.
Nit: While I understand that in practice all tested runtimes have at least 2 bags, the documentation from substrate/frame/bags-list/src/lib.rs:220 indicates that it is possible for T::BagThresholds::get() to be empty. I suggest guarding against this scenario here (like asserting the size to be at least 2 etc.).
3 participants
The benchmark failed depending on
MaxAutoRebagPerBlock(e.g. it passes with 10 as configured in Westend, Polkadot and Kusama AH runtime but it failed with 5, as it was configured before, see runtime PR).Replace the bulk
on_idlebenchmark with a per-itemon_idle_rebagbenchmark that measures the worst-case cost of a single rebag.on_idlenow consumes weight per iteration viaWeightMeterinstead of reserving a single bulk weight upfront.This decouples the benchmark from
MaxAutoRebagPerBlock. Changing the config no longer requires re-running benchmarks.