Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Frame-omni-bencher weights are worse than old bench #6797

Closed
mordamax opened this issue Dec 9, 2024 · 7 comments · Fixed by #7299 or paritytech-stg/polkadot-sdk#78
Closed

Frame-omni-bencher weights are worse than old bench #6797

mordamax opened this issue Dec 9, 2024 · 7 comments · Fixed by #7299 or paritytech-stg/polkadot-sdk#78
Assignees
Labels
T12-benchmarks This PR/Issue is related to benchmarking and weights.

Comments

@mordamax
Copy link
Contributor

mordamax commented Dec 9, 2024

a6ce732
This diff shows

  • red (frame-omni-bencher)
  • green (old bench)

the environment is the same (Github runner)

@mordamax mordamax added the T12-benchmarks This PR/Issue is related to benchmarking and weights. label Dec 10, 2024
@mordamax
Copy link
Contributor Author

@kianenigma @ggwpez ^^ pinging here
because of this issue we are still defaulted to old benchmarks

@iulianbarbu
Copy link
Contributor

I'll look into this

@bkchr
Copy link
Member

bkchr commented Jan 22, 2025

@iulianbarbu try to change the following line:

cargo install --path substrate/utils/frame/omni-bencher --locked

to:

cargo install --path substrate/utils/frame/omni-bencher --locked  --profile production

@mordamax
Copy link
Contributor Author

@iulianbarbu note that to test this workflow change can be done through paritytech-stg, as this event trigger takes always workflow from master, so it's easier to test from there

@ggwpez
Copy link
Member

ggwpez commented Jan 23, 2025

Was this closed by accident or do we know that this was the issue?

@mordamax mordamax reopened this Jan 23, 2025
@mordamax
Copy link
Contributor Author

@ggwpez yes, by accident as PR was merged, it auto-closed issue, thanks

@iulianbarbu
Copy link
Contributor

iulianbarbu commented Jan 24, 2025

Latest test here, which used @bkchr 's suggested fix, ran against corresponding 9edaef0 on paritytech/polkadot-sdk/master, resulted in a smaller difference than what was shown initially in the issue description.

My previous test which resulted in similar numbers as @mordamax showed in the issue description was done against: 350a6c4, which is not much different from 9edaef0 (as in nothing happened in between based on my understanding, that would impact pallet_balances weights on westend).

I would conclude that @bkchr 's fix resolves the issue. Will follow up with the same change on master.

github-merge-queue bot pushed a commit that referenced this issue Jan 24, 2025
# Description

This PR builds frame-omni-bencher with `production` profile when calling
`/cmd bench-omni` to compute benchmarks for pallets.
Fix proposed by @bkchr , thanks!

Closes #6797.

## Integration

N/A

## Review Notes

More info on #6797, and related to how the fix was tested:
#6797 (comment).

---------

Signed-off-by: Iulian Barbu <[email protected]>
Co-authored-by: command-bot <>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T12-benchmarks This PR/Issue is related to benchmarking and weights.
Projects
None yet
4 participants