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

Conversation

@georgesdib
Copy link
Contributor

@georgesdib georgesdib commented Mar 27, 2022

Fixes #11111

Moving the previous onchain.rs into a pallet in order to be able to benchmark it.

Adding benchmarks for it.

I have change quite drastically the API for that, this trait will always need to be bounded now.

Some thoughts:

DataProvider is part of the pallet. This means that only one such provider can be given to that pallet for the full blockchain (unless the pallet is instantiable).
Given that this pallet is meant to be used in a variety of cases, including staking but also governance. Don’t we need multiple DataProvider to cater for that?
In other words, should the pallet be instantiable (or take DataProvider out of the pallet similar to NposSolver)?

Polkadot companion: paritytech/polkadot#5211
skip check-dependent-cumulus

Polkadot address: 131dPecTmpTC1p1ofemufqFBJo9vNbV7dkgN7vWwKnaSMkC4

Georges Dib and others added 30 commits February 23, 2022 21:03
Removing a seemingly outdated comment
And to follow the general guidelines

Co-authored-by: Kian Paimani <[email protected]>
Removing a seemingly outdated comment
And to follow the general guidelines

Co-authored-by: Kian Paimani <[email protected]>
Renaming it to `UnboundedSequentialPhragmen` which should only be used
at genesis and for testing.
Use preferrably `BoundedOnChainSequentialPhragmen`
Copy link
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.

I really like the overall direction of this, except one minor problem that I want to raise as soon as possible:

I don't think we should mix the benchmarking pallet into the existing code. It should, instead, but a new crate. The reason for this is that now, if anyone wants to use BoundedExecution, they need to import a whole fake pallet into their runtime. This is more bytes of code in the runtime, which is a problem for both parachians and the relay chain (wasm size does matter!).

Whereas if you make it a separate pallet, that new pallet is only compiled when you run the runtime-bechmkarks command, and it spits out just a single file that contains the weight amounts, which is used in the runtime.

Other than this, I don't see any major issues with this and hopefully next week I will do a good review.

Thanks for your contributions!

@kianenigma kianenigma added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Mar 29, 2022
@georgesdib
Copy link
Contributor Author

I really like the overall direction of this, except one minor problem that I want to raise as soon as possible:

I don't think we should mix the benchmarking pallet into the existing code. It should, instead, but a new crate. The reason for this is that now, if anyone wants to use BoundedExecution, they need to import a whole fake pallet into their runtime. This is more bytes of code in the runtime, which is a problem for both parachians and the relay chain (wasm size does matter!).

Whereas if you make it a separate pallet, that new pallet is only compiled when you run the runtime-bechmkarks command, and it spits out just a single file that contains the weight amounts, which is used in the runtime.

Other than this, I don't see any major issues with this and hopefully next week I will do a good review.

Thanks for your contributions!

Can you please check #11149 and let me know if this is a better direction of travel?

@georgesdib
Copy link
Contributor Author

Closing this one as superseded by #11149

@georgesdib georgesdib closed this Apr 1, 2022
@georgesdib georgesdib deleted the npos-election-benchmarking branch April 16, 2022 07:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Benchmark different implementations of Solver

2 participants