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 31, 2022

Fixes #11111

Created a benchmarking pallet for frame-election-provider-support, namely SequentialPhragmen and PhragMMS.
Few changes to onchain.rs as well and making the trait always bounded, and adding WeightInfo.

Refactoring #11126

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

Polkadot address: 131dPecTmpTC1p1ofemufqFBJo9vNbV7dkgN7vWwKnaSMkC4

@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 Apr 4, 2022
@kianenigma
Copy link
Contributor

I have some minor feedback as well, but I am going to save that until this major flaw is solved: the purpose of this pallet is to benchmark these election algorithms alone and isolated. The reason for this is that currently the ElectionDataProvider trait specifies that its implementors need to be self-weighing and register their own weight. With that out of the way, we need to only benchmark T::Solver::solve() to have some kind of a realistic measure.

@kianenigma
Copy link
Contributor

Here's some random feedback and how I think this should work out, but this does not compile yet and has some TODOs. If you agree with the overall direction, the branch can be a good guideline:

https://github.com/georgesdib/substrate/pull/new/kiz-onchain-election-benchmarking

Lastly:

For now, we have VotersBound and TargetsBound as being part of ConfigParams which is normally implemented for Runtime. However, ideally these 2 types should be part of the ElectionDataProvider trait as they are really a property of the data provider. This could be worked on in a separate PR with a big ensuing refactoring.

A VoterBound on ElectionDataProvider has the meaning of: what is the maximum number of voters that this data provider can provide (which I don't think we ever need to bound)

A VoterBound on onchain::Config has the meaning of: how many voters should this implementation of onchain::Config ask from its ElectionDataProvider?

TLDR; I think VotersBound on onchain::Config makes good sense :)

@kianenigma
Copy link
Contributor

\tip large

1 similar comment
@kianenigma
Copy link
Contributor

\tip large

Copy link
Contributor

@emostov emostov left a comment

Choose a reason for hiding this comment

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

LGTM - thanks

@kianenigma
Copy link
Contributor

/tip large

@kianenigma
Copy link
Contributor

/tip large

@kianenigma
Copy link
Contributor

\tip large

@kianenigma
Copy link
Contributor

Okay, I will open a manual tip for you @georgesdib.

@kianenigma
Copy link
Contributor

bot merge

@paritytech-processbot
Copy link

Error: pr-custom-review is not passing for paritytech/polkadot#5241

@kianenigma
Copy link
Contributor

bot merge

@paritytech-processbot
Copy link

Error: pr-custom-review is not passing for paritytech/polkadot#5241

@kianenigma
Copy link
Contributor

bot merge

@paritytech-processbot
Copy link

Error: pr-custom-review is not passing for paritytech/polkadot#5241

@kianenigma
Copy link
Contributor

bot merge

@paritytech-processbot
Copy link

Error: pr-custom-review is not passing for paritytech/polkadot#5241

@kianenigma
Copy link
Contributor

bot merge

@paritytech-processbot paritytech-processbot bot merged commit c9af3c6 into paritytech:master Apr 15, 2022
@georgesdib georgesdib deleted the onchain-election-benchmarking branch April 15, 2022 10:22
@ggwpez
Copy link
Member

ggwpez commented Apr 15, 2022

You skipped the cumulus check, somehow my cumulus CI errors with:

error[E0432]: unresolved import `frame_election_provider_support::onchain::ExecutionConfig`
  --> /usr/local/cargo/git/checkouts/polkadot-4038f27d5e4ea2e8/3ff2b83/runtime/common/src/elections.rs:20:12
   |
20 |     onchain::{ExecutionConfig, UnboundedExecution},
   |               ^^^^^^^^^^^^^^^ no `ExecutionConfig` in `onchain`

Could this be related @georgesdib ?

@georgesdib
Copy link
Contributor Author

You skipped the cumulus check, somehow my cumulus CI errors with:

error[E0432]: unresolved import `frame_election_provider_support::onchain::ExecutionConfig`
  --> /usr/local/cargo/git/checkouts/polkadot-4038f27d5e4ea2e8/3ff2b83/runtime/common/src/elections.rs:20:12
   |
20 |     onchain::{ExecutionConfig, UnboundedExecution},
   |               ^^^^^^^^^^^^^^^ no `ExecutionConfig` in `onchain`

Could this be related @georgesdib ?

Isn't this in from the polkadot codebase? The companion PR is now merged, perhaps the above was run after the substrate PR was merged but not the polkadot PR?

@ggwpez
Copy link
Member

ggwpez commented Apr 15, 2022

Yes sorry, I think the dependencies just got messed up. Thanks for you contributions!

girazoki pushed a commit to moonbeam-foundation/substrate that referenced this pull request May 3, 2022
…tech#11149)

* First stab at adding benchmarking for
`election-provider-support` onchain

* Adding `BoundedPhragMMS` and fixing stuff

* Fixing node runtime

* Fixing tests

* Finalising all benchmarking stuff

* better comments

* Better benchmarking config

* Better `WeightInfo` and benchmarking

* Fixing tests

* Adding some documentation

* Fixing some typos

* Incorporating review feedback

* cleanup of rustdocs

* rustdoc changes

* changes after code review

* Fixing some errors.

* Fixing dependencies post merge

* Bringing back `UnboundedExecution`

* Better rustdoc and naming

* Cargo.toml formatting
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
…tech#11149)

* First stab at adding benchmarking for
`election-provider-support` onchain

* Adding `BoundedPhragMMS` and fixing stuff

* Fixing node runtime

* Fixing tests

* Finalising all benchmarking stuff

* better comments

* Better benchmarking config

* Better `WeightInfo` and benchmarking

* Fixing tests

* Adding some documentation

* Fixing some typos

* Incorporating review feedback

* cleanup of rustdocs

* rustdoc changes

* changes after code review

* Fixing some errors.

* Fixing dependencies post merge

* Bringing back `UnboundedExecution`

* Better rustdoc and naming

* Cargo.toml formatting
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
…tech#11149)

* First stab at adding benchmarking for
`election-provider-support` onchain

* Adding `BoundedPhragMMS` and fixing stuff

* Fixing node runtime

* Fixing tests

* Finalising all benchmarking stuff

* better comments

* Better benchmarking config

* Better `WeightInfo` and benchmarking

* Fixing tests

* Adding some documentation

* Fixing some typos

* Incorporating review feedback

* cleanup of rustdocs

* rustdoc changes

* changes after code review

* Fixing some errors.

* Fixing dependencies post merge

* Bringing back `UnboundedExecution`

* Better rustdoc and naming

* Cargo.toml formatting
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

4 participants