Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Add statistics to benchmarking-cli output #5250

Closed
pscott wants to merge 42 commits into from
Closed

Add statistics to benchmarking-cli output #5250

pscott wants to merge 42 commits into from

Conversation

@pscott
Copy link
Contributor

@pscott pscott commented Mar 13, 2020 •
edited
Loading

The goal of this PR is to add some useful information to the benchmarking-cli output.

  • Accurately track db reads / writes
  • Send the statistics to the benchmarking-cli and output it in CSV format

Before merging:

  • Remove the code that exposes the benchmarking pallet so that it doesn't get added to master.

In order to pass the UsageInfo data from /frame/benchmarking crate to the /utils/frame/benchmarking-cli crate, and use the UsageInfo struct in sp-state-machine and sp-externalities, I had to create a new primitive called sp-stats for this very purpose. Let me know if there's any other way of doing so.

I also merged in Cheme's PR , and then reverted the merge using git revert -m 1 COMMIT. Hope this is the proper way to do it.

Scott Piriou and others added 28 commits March 5, 2020 19:27
Try adding benchmark to runtime and exposit runtime-benchmarks
18b0170
@cheme
Registering state from overlay.
e2286bc
@cheme
fix
660ed3d
@cheme
fix2
89520b9
Merge branch 'master' of https://github.com/paritytech/substrate into…
1dd381f 
… scott_add_usage_info
Merge branch 'state-metrics' of https://github.com/cheme/substrate in…
07da93a 
…to scott_add_usage_info
WIP: add usage info stub functions; add InstantWithDefault
ae340b5
Add new method to InstantWithDefault
fc784f3
Add sp-state-machine as optional in frame/benchmarking
5873f57
Rename methods
2199777
add sp_stats crate
5100dc3
Update comments and typo
711a161
Add default-features = false for runtime-interface
9490cb7
Replace instant with Duration; Make code compilable
fe22050
Add corret fn signatures
71d2a7c
Impl PassBy in runtime-interface
a6dd88d
Remove empty return of db_usage_info
169da7a
Remove unimplemented code
850062b
call usage_info in benchmarking-cli
003325b
Add RefCell<UsageInfo> to BenchmarkingState
ae5c499
Merge branch 'scott_add_usage_info' of github.com:pscott/substrate in…
04371db 
…to scott_add_usage_info
Revert "call usage_info in benchmarking-cli"
a1516be 
This reverts commit 003325b.
Update try_borrow_mut's error logic
a60a5da
Add mut keyword to usage_info
43b81b6
Add anti-pattern let _ to silence clippy's warning
bd96591
Clean code and update comments
c9db643
Read bytes as u64 instead of usize
987027b
Add bytes as u64 instead of usize
c412117
@pscott pscott added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes labels Mar 13, 2020
@pscott pscott added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Mar 18, 2020
@pscott pscott requested review from NikVolf and bkchr March 18, 2020 17:31
bkchr
bkchr previously requested changes Mar 19, 2020
View reviewed changes
@shawntabrizi shawntabrizi mentioned this pull request Mar 20, 2020
Merged
12 tasks
@pscott pscott changed the title Add statistics to bencharking-cli output Add statistics to benchmarking-cli output Mar 26, 2020
@pscott pscott requested a review from bkchr March 29, 2020 16:56
@pscott pscott dismissed bkchr’s stale review March 31, 2020 13:42

Review has been addressed

@pscott
Copy link
Contributor Author

pscott commented Mar 31, 2020

This PR is ready for review (and merge). I've removed the code that exposed the Benchmark pallet. @bkchr @NikVolf :D

@NikVolf
Copy link
Contributor

NikVolf commented Mar 31, 2020

can you rebase without any @cheme code in the diff?

NikVolf
NikVolf reviewed Mar 31, 2020
View reviewed changes
client/db/src/stats.rs
}

/// Merge state machine usage info.
pub fn merge_sm(&self, info: sp_state_machine::UsageInfo) {
Copy link
Contributor

@NikVolf NikVolf Mar 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this something you changed @pscott
or it was part of @cheme merged pr?

@NikVolf
Copy link
Contributor

NikVolf commented Mar 31, 2020

Can we have at least simple test which shows that stats are being collected?

@pscott pscott mentioned this pull request Apr 1, 2020
Closed
@pscott
Copy link
Contributor Author

pscott commented Apr 1, 2020

Closed in favor of #5483 :)

@pscott pscott closed this Apr 1, 2020
@bkchr bkchr deleted the scott_add_usage_info branch April 1, 2020 10:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@bkchr bkchr Awaiting requested review from bkchr

1 more reviewer

@NikVolf NikVolf NikVolf left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@pscott pscott

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@pscott @parity-cla-bot @NikVolf @bkchr @paritytech-ci @cheme @shawntabrizi