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

[benchmarking] Refactor Benchmark #1533

Merged
merged 36 commits into from
Apr 22, 2022
Merged

[benchmarking] Refactor Benchmark #1533

merged 36 commits into from
Apr 22, 2022

Conversation

longbowlu
Copy link
Contributor

@longbowlu longbowlu commented Apr 22, 2022

This PR refactors the benchmark into several pieces:

  1. add ValidatorPreparer that keeps tracks of validator/committee information and more importantly takes care of validator deployment and teardown.
  2. simplify TransactionCreator by moving some validator logic to ValidatorPreparer. After this diff it will only focuses on transaction generation

Also introduce the ability to run validator locally as a process. Add args 'local-single-validator-process' and 'local-single-validator-thread' to distinguish the running mode. More modes to be added for distributed benchmarking,

Also Add "How-to" in bench.rs.

In the long term, there are more abstraction work can be done. e.g. fully split out benchmark coordinator, clients (load generators), validator, and more roles e.g. gateway in the future. Separating load generator from coordinator is beneficial in having distributed load generators to saturate network.

Copy link
Collaborator

@gdanezis gdanezis left a comment

Choose a reason for hiding this comment

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

Great stuff - a few little nits here and there that you can address as you see fit.

network_utils/src/network.rs Outdated Show resolved Hide resolved
sui/src/benchmark.rs Outdated Show resolved Hide resolved
/// Size of the Sui committee. Minimum size is 4 to tolerate one fault
#[clap(long, default_value = "10", global = true)]
/// Size of the Sui committee.
#[clap(long, default_value = "1", global = true)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we setting this to default = 1? Then someone who runs the bench with default values will get no sense of the overhead of running even with the smallest meaningful committee of 4 (in terms of sig verification etc).

Copy link
Contributor Author

@longbowlu longbowlu Apr 22, 2022

Choose a reason for hiding this comment

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

In setup, we spin out {committee_size} validators. However currently the benchmarking only supports one validator, hence it continuously looks for peers and gets connection errors. Soon we should be able to add support for multiple validators though.

For the default value, I'm changing it to 1 so we don't have the issue above. I can keep it at 10 or 4 but we need to specify --committee-size=1 for now. I would expect multi-validator approach come along as a fast follow so either way works for me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gdanezis I'm keeping it @ 1 for now. Happy to update it any time!

gdanezis and others added 4 commits April 22, 2022 08:29
…ading the transactions information (#1446)

Co-authored-by: George Danezis <[email protected]>
There is no undefined address in the [addresses] section (e.g. AddressToBeFilledIn = "_") so having a [dev-addresses] section is not necessary.
- Made init rules static
- The existence of the function is optional
- The function must be named 'init'
- The function must be private
- The function can have a single parameter, &mut TxContext
- Alternatively, the function can have zero parameters
@oxade
Copy link
Contributor

oxade commented Apr 22, 2022

@longbowlu is there any functional change or is it just a refactor?

@longbowlu
Copy link
Contributor Author

@longbowlu is there any functional change or is it just a refactor?

as we discussed, the PR also adds the ability to run a single validator as a process

* Refactor Build index by common user workflow

* Update index.md

Fix paths missing Markdown file extension
Removing tutorial series until ready
Remove version from API link
Capitalize REST
Copy link
Contributor

@Clay-Mysten Clay-Mysten left a comment

Choose a reason for hiding this comment

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

Made a small fix to the API link directly
Markdown LGTM

Lu, please make sure this affects none of our other docs. Thanks!

@longbowlu longbowlu merged commit 64357d7 into MystenLabs:main Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants