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.

Sign up for GitHub

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

Jump to bottom

Refactor Identity to benchmark v2 #1838

Merged
merged 12 commits into from
Oct 11, 2023
Merged

Refactor Identity to benchmark v2 #1838

merged 12 commits into from
Oct 11, 2023

Conversation

0xmovses
Copy link
Contributor

@0xmovses 0xmovses commented Oct 10, 2023 •
edited
Loading

This PR refactors identity/benchmarkings.rs to use benchmarking v2. These changes are needed to improve the readability and maintainability of the benchmarking code. Changes were implemented using this commit as a guide. The logic of the benchmarks remains the same.

No known issue to backlink.

Local Testing

To test the new benchmarks:

  1. cargo build --features runtime-benchmarks
  2. ./target/debug/polkadot benchmark pallet --steps=5 --repeat=2 --pallet=pallet_identity --extrinsic='*'

@0xmovses 0xmovses added R0-silent Changes should not be mentioned in any release notes I4-refactor Code needs refactoring. T2-pallets This PR/Issue is related to a particular pallet. T12-benchmarks This PR/Issue is related to benchmarking and weights. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. labels Oct 10, 2023
@0xmovses 0xmovses requested review from a team October 10, 2023 14:11
@0xmovses 0xmovses marked this pull request as draft October 10, 2023 14:11
joepetrowski
joepetrowski reviewed Oct 10, 2023
View reviewed changes
Copy link
Contributor

@joepetrowski joepetrowski left a comment

Choose a reason for hiding this comment

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

Looks good, also need to run cargo +nightly fmt, looks like that CI is failing.
Or just enter "bot fmt" as a comment on the PR.

substrate/frame/identity/src/benchmarking.rs Outdated Show resolved Hide resolved
substrate/frame/identity/src/benchmarking.rs Outdated Show resolved Hide resolved
@0xmovses 0xmovses marked this pull request as ready for review October 11, 2023 13:36
@paritytech-ci paritytech-ci requested a review from a team October 11, 2023 13:38
@joepetrowski
Copy link
Contributor

bot fmt

@command-bot
Copy link

command-bot bot commented Oct 11, 2023 •
edited
Loading

@joepetrowski https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3935227 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 26-d77c1b18-c0bd-46cd-87d3-d7b10d79b645 to cancel this command or bot cancel to cancel all commands in this pull request.

sam0x17
sam0x17 approved these changes Oct 11, 2023
View reviewed changes
Copy link
Contributor

@sam0x17 sam0x17 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

looks like it properly refactors to the new syntax 👍🏻

just some quick rustfmt fixes and should be good

@command-bot
Copy link

command-bot bot commented Oct 11, 2023

@joepetrowski Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3935227 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3935227/artifacts/download.

@0xmovses
Copy link
Contributor Author

bot fmt

@command-bot
Copy link

command-bot bot commented Oct 11, 2023 •
edited
Loading

@0xmovses https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3935887 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 27-49f3c8ab-ab1a-4bad-bce6-f10304973c67 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Oct 11, 2023

@0xmovses Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3935887 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3935887/artifacts/download.

ggwpez
ggwpez approved these changes Oct 11, 2023
View reviewed changes
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Looks like this will produce conflicts with #1661 (cc @georgepisaltu), hopefully not too painful.

substrate/frame/identity/src/benchmarking.rs Outdated Show resolved Hide resolved
substrate/frame/identity/src/benchmarking.rs Outdated Show resolved Hide resolved
@0xmovses
Copy link
Contributor Author

bot fmt

@command-bot
Copy link

command-bot bot commented Oct 11, 2023 •
edited
Loading

@0xmovses https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3937648 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 29-f2954972-3c45-4897-8832-d67e2bd1e70e to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Oct 11, 2023

@0xmovses Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3937648 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3937648/artifacts/download.

@0xmovses 0xmovses enabled auto-merge (squash) October 11, 2023 14:55
@0xmovses 0xmovses merged commit 1d9ec57 into master Oct 11, 2023
10 checks passed
@0xmovses 0xmovses deleted the 0xmovses-identity-benchmark-v2 branch October 11, 2023 15:21
ordian added a commit that referenced this pull request Oct 12, 2023
@ordian
Merge branch 'master' into ao-disabling-statement-distribution
e333fa8 
* master: (33 commits)
  ci: set CI_IMAGE back to (now updated) .ci-unified (#1854)
  ci: bump ci image to rust 1.73.0 (#1830)
  Refactor Identity to benchmark v2 (#1838)
  PVF worker: bump landlock, update ABI docs (#1850)
  Xcm emulator nits (#1649)
  Fixes path issue in derive-impl (#1823)
  upgrade to macro_magic 0.4.3 (#1832)
  Use safe math when pruning statuses (#1835)
  remote-ext: fix state download stall on slow connections and reduce memory usage (#1295)
  Update testnet bootnode dns name (#1712)
  [FRAME] Warn on unchecked weight witness (#1818)
  [xcm] Use `Weight::MAX` for `reserve_asset_deposited`, `receive_teleported_asset` benchmarks (#1726)
  Update bridges subtree (#1803)
  Check for parent of first ready block being on chain (#1812)
  Make CheckNonce refuse transactions signed by accounts with no providers (#1578)
  Fix Asset Hub collator crashing when starting from genesis (#1788)
  Mixnet integration (#1346)
  [xcm-emulator] Decouple the `AccountId` type from `AccountId32` (#1458)
  Treasury spends various asset kinds (#1333)
  chore: bump zombienter version (#1806)
  ...
@kiltbot kiltbot mentioned this pull request Nov 7, 2023
Closed
@ahmadkaouk ahmadkaouk mentioned this pull request Nov 16, 2023
Closed
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
@0xmovses @joepetrowski
@franciscoaguirre @ggwpez
Refactor Identity to benchmark v2 (paritytech#1838)
a111c83 
This PR refactors `identity/benchmarkings.rs` to use benchmarking v2.
These changes are needed to improve the readability and maintainability
of the benchmarking code. Changes were implemented using
[this](paritytech@9ec8009)
commit as a guide. The logic of the benchmarks remains the same.

No known issue to backlink.

## Local Testing
To test the new benchmarks:
1. `cargo build --features runtime-benchmarks`
2. `./target/debug/polkadot benchmark pallet --steps=5 --repeat=2
--pallet=pallet_identity --extrinsic='*'`

---------

Co-authored-by: Richard Melkonian <[email protected]>
Co-authored-by: joe petrowski <[email protected]>
Co-authored-by: command-bot <>
Co-authored-by: Francisco Aguirre <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
@rockbmb rockbmb mentioned this pull request Oct 11, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Oct 16, 2024
@rockbmb @seadanda @shawntabrizi
Refactor staking pallet benchmarks to v2 (#6025)
9d78c51 
# Description
This PR migrates the staking pallet's benchmarks to the `v2` of pallet
benchmarking tooling provided by
[`frame_benchmarking`](https://github.com/paritytech/polkadot-sdk/tree/master/substrate/frame/benchmarking).

## Integration

N/A

## Review Notes

The PR is straightforward, as were #1676 , #1838 and #1868.

---------

Co-authored-by: Dónal Murray <[email protected]>
Co-authored-by: Shawn Tabrizi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joepetrowski joepetrowski joepetrowski left review comments

@ggwpez ggwpez ggwpez approved these changes

@sam0x17 sam0x17 sam0x17 approved these changes

@franciscoaguirre franciscoaguirre Awaiting requested review from franciscoaguirre

Assignees
No one assigned
Labels
D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. I4-refactor Code needs refactoring. R0-silent Changes should not be mentioned in any release notes T2-pallets This PR/Issue is related to a particular pallet. T12-benchmarks This PR/Issue is related to benchmarking and weights.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@0xmovses @joepetrowski @sam0x17 @franciscoaguirre @ggwpez