benchmarking: fix timing leak from bulk setup operations#10802
Merged
benchmarking: fix timing leak from bulk setup operations#10802
Conversation
…leaks Call `commit_db()` inside the setup_stmts repetition block in the benchmark procedural macro. This ensures each setup statement's storage changes are committed immediately, rather than batching all commits in the following on_before_start triggered by recording.start(). Fixes timing leaks in benchmarks with large setup operations (e.g., `clear_validators_and_nominators()` which removes ~27k entries). Without this fix, such benchmarks show regression (milliseconds instead of microseconds for extrinsics requiring very few storage operations) even though timing starts after commit.
sigurpol
added
R0-no-crate-publish-required
Contributor
Author
|
To be noted (@ggwpez , @bkchr ) that this small change probably doesn't hurt but doesn't explain why the following commit_db triggered by recording.start() is not enough (see #10798 (comment)). |
Contributor
Author
|
/cmd prdoc --audience runtime_dev --bump patch |
Contributor
Differential Tests Results (PolkaVM)Specified Tests
Counts
FailuresThe test specifiers seen in this section have the format 'path::case_idx::compilation_mode' and they're compatible with the revive differential tests framework and can be specified to it directly in the same way that they're provided through the The failures are provided in an expandable section to ensure that the PR does not get polluted with information. Please click on the section below for more information Detailed Differential Tests Failure Information
|
Contributor
Author
|
/cmd bench --pallet pallet_staking_async --runtime asset-hub-westend |
Contributor
|
Command "bench --pallet pallet_staking_async --runtime asset-hub-westend" has started 🚀 See logs here |
…t_staking_async --runtime asset-hub-westend'
Contributor
|
Command "bench --pallet pallet_staking_async --runtime asset-hub-westend" has finished ✅ See logs here DetailsSubweight results:
Command output:✅ Successful benchmarks of runtimes/pallets: |
Move commit_db() calls from macro-generated code into Recording trait. BenchmarkRecording commits after each setup stmt (fixes timing leaks), TestRecording skips commits (test backend doesn't support proper commit).
…ights-fix-benchmark # Conflicts: # cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_staking_async.rs
Contributor
Author
|
/cmd bench --pallet pallet_staking_async --runtime asset-hub-westend |
Contributor
|
Command "bench --pallet pallet_staking_async --runtime asset-hub-westend" has started 🚀 See logs here |
Contributor
Author
|
/cmd bench --pallet pallet_staking_async --runtime asset-hub-westend |
Contributor
|
Command "bench --pallet pallet_staking_async --runtime asset-hub-westend" has started 🚀 See logs here |
kianenigma
approved these changes
Jan 15, 2026
Contributor
kianenigma
left a comment
kianenigma
left a comment
There was a problem hiding this comment.
Any regression on the total execution time of benchmarks of e.g. staking-async as a whole? wondering because we do this committing more often. I wouldn't expect one.
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-10802-to-stable2512
git worktree add --checkout .worktree/backport-10802-to-stable2512 backport-10802-to-stable2512
cd .worktree/backport-10802-to-stable2512
git reset --hard HEAD^
git cherry-pick -x 64dc02179ba69303191bfeb8df2b00583fc77658
git push --force-with-lease |
sigurpol
added
A4-backport-unstable2507
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-10802-to-unstable2507
git worktree add --checkout .worktree/backport-10802-to-unstable2507 backport-10802-to-unstable2507
cd .worktree/backport-10802-to-unstable2507
git reset --hard HEAD^
git cherry-pick -x 64dc02179ba69303191bfeb8df2b00583fc77658
git push --force-with-lease |
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-10802-to-stable2509
git worktree add --checkout .worktree/backport-10802-to-stable2509 backport-10802-to-stable2509
cd .worktree/backport-10802-to-stable2509
git reset --hard HEAD^
git cherry-pick -x 64dc02179ba69303191bfeb8df2b00583fc77658
git push --force-with-lease |
sigurpol
added a commit
that referenced
this pull request
Jan 19, 2026
Backport #10802 into `stable2509` from sigurpol. 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: Paolo La Camera <paolo@parity.io>
sigurpol
added a commit
that referenced
this pull request
Jan 19, 2026
Backport #10802 into `unstable2507` from sigurpol. 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: Paolo La Camera <paolo@parity.io> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
sigurpol
added a commit
that referenced
this pull request
Jan 19, 2026
Backport #10802 into `stable2512` from sigurpol. 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: Paolo La Camera <paolo@parity.io> Co-authored-by: Egor_P <egor@parity.io>
ggwpez
reviewed
Jan 30, 2026
| Weight::from_parts(16_508_868_000, 0) | ||
| .saturating_add(Weight::from_parts(0, 6696)) | ||
| .saturating_add(T::DbWeight::get().reads(17)) | ||
| .saturating_add(T::DbWeight::get().writes(11)) |
Contributor
Author
There was a problem hiding this comment.
fix is coming in 5 min, wait for it and sorry for that
sigurpol
added a commit
that referenced
this pull request
Jan 30, 2026
PR #10802 added `reset_read_write_count()` at the end of commit_db() to prevent warmup operations from appearing in benchmarking results. However, commit_db is called twice: one on_before_start() closure before benchmark, and one after benchmark execution after benchmark. This PR moves read_write_count() call before the post-benchmark commit_db() call.
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Feb 3, 2026
PR #10802 added `reset_read_write_count()` at the end of commit_db() to prevent warmup operations from appearing in benchmarking results. However, commit_db is called twice: one on `on_before_start()` closure before benchmark, and one after benchmark execution after benchmark. This PR whitelists the warmup key used in commit_db so that it doesn't appear in the read/write count. We also regenerated staking-async weights (wrongly benchmarked in #10802) and conviction-voting to check both v1 and v2 benchmarking. Driven-by: update `try-runtime-cli` to v0.10.1 as an attempt to fix the issue for which CI regularly fails in the check-migration (WAH) job in ./try-runtime create-snapshot --uri wss://westend-asset-hub-rpc.polkadot.io:443 snapshot.raw` --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This was referenced Feb 11, 2026
github-project-automation
Bot
moved this from Todo
to Done
in [preview] release tracker
github-project-automation
Bot
moved this from Todo
to Done
in [preview] release tracker
arturgontijo
pushed a commit
to moonbeam-foundation/polkadot-sdk
that referenced
this pull request
Apr 1, 2026
Backport paritytech#10802 into `stable2512` from sigurpol. 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: Paolo La Camera <paolo@parity.io> Co-authored-by: Egor_P <egor@parity.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes timing leaks in benchmarks with large setup operations (e.g., clearing 27k staking entries). After bulk deletions are committed, the first new allocation can trigger memory allocator overhead that leaks into benchmark timing.
The fix adds a memory allocator warmup step in
commit_db()that performs a dummy write/clear cycle to absorb this overhead before timing starts.Fix #10798.
Another related issue: #10813 (rework of staking benchmarks to avoid massive bulk deletion if not needed. An example showing the validity of the approach here where we just remove a clear_validators_and_nominators() from one benchmark and that's enough to go down from ms to microsec)