-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat: pop bench pallet
logic implementation
#407
base: chungquantin/feat-pop_bench_command
Are you sure you want to change the base?
feat: pop bench pallet
logic implementation
#407
Conversation
use frame_benchmarking_cli::PalletCmd; | ||
use sp_runtime::traits::BlakeTwo256; | ||
|
||
type HostFunctions = ( |
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.
These are set based on the implementation of FRAME Omni Bencher. These are necessary to benchmark parachain runtime.
5c891b5
to
8f82fde
Compare
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## chungquantin/feat-pop_bench_command #407 +/- ##
=======================================================================
- Coverage 75.11% 75.09% -0.03%
=======================================================================
Files 63 66 +3
Lines 13842 13946 +104
Branches 13842 13946 +104
=======================================================================
+ Hits 10398 10473 +75
- Misses 2118 2139 +21
- Partials 1326 1334 +8
|
@AlexD10S CI fails but the PR is good for a review. |
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.
Great work! The implementation is clean, and I love seeing how you're already handling the UI testing.
I’ve left some comments on areas for improvement, along with a couple of additional tests that could help improve coverage.
One refactor I want to see: In the rest of the commands, we typically separate the core logic (handled in pop_contracts
or pop_parachain
) from the "frontend logic" (pop-cli
crate). In this PR, the logic is just cmd.run_with_spec
, but it might be worth following the same approach by adding a generate_benchmarks(cmd)
function in pop_parachain
crate to handle this logic.
use anyhow::Result; | ||
|
||
// Displays a message to the user, with formatting based on the success status. | ||
pub(crate) fn display_message(message: &str, success: bool, cli: &mut impl Cli) -> Result<()> { |
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.
Nice! I should have done this refactor a while ago
console::Term::stderr().clear_last_lines(1)?; | ||
cli.info(format!( | ||
"Weight file is generated to {}", | ||
output_path.as_path().display().to_string() |
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.
Clippy warning
Features
bench
to benchmark pallets & parachain.frame-benchmarking-cli
as a subcommand.<runtime-name>.wasm
or trigger build if needed #411How to test?
Use the below command in your pop-cli project to benchmark the
base_parachain.wasm
.Or run a command
pop bench pallet
Benchmarking completed successfully
To run benchmarking, builds the parachain runtime and specify the runtime path with
--runtime
Failed with error
[sc-2881]