Skip to content

Adding migration instruction from benchmarking v1 to v2#6093

Merged
ggwpez merged 18 commits intomasterfrom
re-gius/benchmarking-docs-migrate-v2
Oct 18, 2024
Merged

Adding migration instruction from benchmarking v1 to v2#6093
ggwpez merged 18 commits intomasterfrom
re-gius/benchmarking-docs-migrate-v2

Conversation

@re-gius
Copy link
Copy Markdown
Contributor

@re-gius re-gius commented Oct 16, 2024

Adding instruction to migrate benchmarking from v1 to v2

Even if the documentation for benchmarking v1 and v2 is clear and detailed, I feel that adding a migration guide from v1 to v2 would help doing it quicker.

Integration

This change only affects documentation, so it does not cause integration issues.

Review Notes

I followed the migration procedure I applied in PR #6018 . I added everything from there, but I may be missing some extra steps that are needed in specific case, so in case you notice something please let me know.

@re-gius re-gius added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Oct 16, 2024
@re-gius re-gius self-assigned this Oct 16, 2024
@re-gius re-gius marked this pull request as ready for review October 16, 2024 13:56
@re-gius re-gius requested a review from a team as a code owner October 16, 2024 13:56
Copy link
Copy Markdown
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Thank you very much!

@github-actions github-actions bot requested a review from kianenigma October 16, 2024 14:22
@github-actions
Copy link
Copy Markdown
Contributor

Review required! Latest push from author must always be reviewed

Copy link
Copy Markdown
Contributor

@rockbmb rockbmb left a comment

Choose a reason for hiding this comment

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

Great summary!

There's a construct from v1 that doesn't exist in v2 whose refactoring would be nice to describe here, but I don't know how it can be done in the general case.

In v1 of the benchmarking pallet, there exists a macro named selected_benchmark, which is used to create a SelectedBenchmark enum, which will contain all of the benchmarks defined inside benchmark! as variants.
From this enum we can access a specific benchmark to e.g. run it as a unit test.
Here are examples: https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/benchmarking/src/tests.rs#L256

In #6018 there was no code using it, but in #6025, there was one occurrence - which luckily could just be removed.
Doing a code search on SelectedBenchmark over the entire polkadot-sdk shows it is used in:

  • frame-benchmarking itself (src/tests.rs, src/v1.rs)
  • frame-support
  • frame-benchmarking-cli

I doubt moving the above pallets/crates to v2 is a high-priority task, so describing how to migrate SelectedBenchmark can be done at a later time.

@github-actions github-actions bot requested a review from rockbmb October 17, 2024 08:25
/// 1. Change the import from `frame_benchmarking::v1::` to `frame_benchmarking::v2::*`, or
/// `frame::benchmarking::prelude::*` under the umbrella crate;
/// 2. Move the code inside the v1 `benchmarks! { ... }` block to the v2 benchmarks module `mod
/// benchmarks { ... }` under the benchmarks macro (`#[benchmarks]` or `#[instance_benchmarks]`);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe worth explaining briefly why you'd use one or the other as you did for #[extrinsic_call] vs #[block].

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added in 509a173

re-gius and others added 2 commits October 17, 2024 14:14
Co-authored-by: Dónal Murray <donal.murray@parity.io>
Co-authored-by: Dónal Murray <donal.murray@parity.io>
@github-actions github-actions bot requested review from rockbmb and seadanda October 17, 2024 12:14
@re-gius re-gius added the R0-no-crate-publish-required The change does not require any crates to be re-published. label Oct 17, 2024
@ggwpez ggwpez enabled auto-merge October 18, 2024 14:39
@ggwpez ggwpez added this pull request to the merge queue Oct 18, 2024
Merged via the queue into master with commit a83f0fe Oct 18, 2024
@ggwpez ggwpez deleted the re-gius/benchmarking-docs-migrate-v2 branch October 18, 2024 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

R0-no-crate-publish-required The change does not require any crates to be re-published. T1-FRAME This PR/Issue is related to core FRAME, the framework.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants