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

benchmark pallet asset manager #991

Merged
merged 14 commits into from
Nov 18, 2021

Conversation

girazoki
Copy link
Collaborator

@girazoki girazoki commented Nov 15, 2021

What does it do?

Benchmark pallet-asset-manager draft. A new moonbase-runtime-benchmarks flag was added since this pallet is only present on moonbase

What important points reviewers should know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

@girazoki girazoki changed the title Girazoki benchmark pallet asset manager benchmark pallet asset manager Nov 15, 2021
@girazoki girazoki added the B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes label Nov 15, 2021
]
moonbase-runtime-benchmarks = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this imply runtime-benchmarks so you don't need multiple feature flags to use it? You would probably need to do that in a lot of pallets or create an alias in .cargo/config.toml, so maybe not...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can try

Copy link
Collaborator Author

@girazoki girazoki Nov 17, 2021

Choose a reason for hiding this comment

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

The main issue here is that I need to propagate the flag at least till runtime-common, which is the common place where we put our benchmark apis. And since runtime-common is common for all runtimes, I need to have a special flag to incorportate the benchmarks for pallets belonging to moonbase only.

The way I designed it currently is that if you build with moonbase-runtime-bechmarks, it will build with runtime-benchmarks & moonbase-runtime-benchmarks just the moonbase runtime. From here there are two possibilities:

  • Leave it as is, where building with features moonbase-runtime-bechmarks compile the moonbase runtime with both runtime-benchmarks and moonbase-runtime-benchmarks
  • Make it so that we only have one flag in moonbase called moonbase-runtime-benchmarks, and do something like moonbase-runtime-benchmarks = ["frame-support/runtime-benchmarks", "pallet-society/runtime-benchmarks", etc..]"

Quite honestly I'd rather not be too invasive with this PR, and not change much how things worked. I think this approach is the lesser invasive as things worked as before, but we have a new flag with which we can run the just moonbase benchmarks

@girazoki girazoki marked this pull request as ready for review November 17, 2021 10:19
@girazoki girazoki merged commit 681ba34 into master Nov 18, 2021
@girazoki girazoki deleted the girazoki-benchmark-pallet-asset-manager branch November 18, 2021 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants