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.

Sign up for GitHub

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

Jump to bottom

WIP: Modularize benches #1321

Merged
merged 5 commits into from
Apr 14, 2022
Merged

WIP: Modularize benches #1321

merged 5 commits into from
Apr 14, 2022

Conversation

oxade
Copy link
Contributor

@oxade oxade commented Apr 12, 2022 •
edited
Loading

  1. Re-organize benchmarks
  2. Use common functions for benchmarks
  3. Change the way we measure latency: now we run a load gen and also send one-shot tracers to measure the latency
mbp> ./bench microbench latency
2022-04-12T07:44:20.665252Z  INFO sui::benchmark::transaction_creator: Open database on path: "/var/folders/3m/hnrdf4892mj0df4tqf55pm_m0000gn/T/DB_3F8D9B91E231F071D73EAEF3C197B7069B05C421"
2022-04-12T07:44:21.147115Z  INFO sui_network::transport: Listening to TCP traffic on 127.0.0.1:9555
Average Latency 3153.63 us @ 100000 tps

mbp> ./bench microbench throughput 
2022-04-12T07:44:50.747650Z  INFO sui::benchmark::transaction_creator: Open database on path: "/var/folders/3m/hnrdf4892mj0df4tqf55pm_m0000gn/T/DB_AEE3599DEC78997B6264DC322ACE9C445B7A3CA0"
2022-04-12T07:44:51.274317Z  INFO sui_network::transport: Listening to TCP traffic on 127.0.0.1:9555
Throughout: 120452.03238714246 tps

@codecov
Copy link

codecov bot commented Apr 12, 2022 •
edited
Loading

Codecov Report

Merging #1321 (b14b4a7) into main (3b31866) will increase coverage by 0.93%.
The diff coverage is 0.29%.

@@            Coverage Diff             @@
##             main    #1321      +/-   ##
==========================================
+ Coverage   81.71%   82.64%   +0.93%     
==========================================
  Files         100      103       +3     
  Lines       20966    20743     -223     
==========================================
+ Hits        17133    17144      +11     
+ Misses       3833     3599     -234
Impacted Files Coverage Δ
sui/src/benchmark.rs 0.00% <0.00%> (ø)
sui/src/benchmark/bench_types.rs 0.00% <0.00%> (ø)
sui/src/benchmark/load_generator.rs 0.00% <0.00%> (ø)
sui/src/benchmark/transaction_creator.rs 0.00% <0.00%> (ø)
sui/src/lib.rs 83.33% <ø> (ø)
sui/src/bench.rs 8.33% <8.33%> (ø)
network_utils/src/network.rs 37.41% <100.00%> (ø)
network_utils/src/unit_tests/transport_tests.rs 88.23% <0.00%> (-1.48%) ⬇️
...i_programmability/verifier/src/id_leak_verifier.rs 90.58% <0.00%> (-0.45%) ⬇️
sui/src/rest_server.rs 23.43% <0.00%> (-0.34%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b31866...b14b4a7. Read the comment docs.

asonnino
asonnino reviewed Apr 13, 2022
View reviewed changes
sui/src/bench.rs
use tracing::subscriber::set_global_default;
use tracing_subscriber::EnvFilter;

fn main() {
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the input parameters that this function is expecting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It uses CLI args. Am I supposed to specify something here?

Copy link
Contributor

Choose a reason for hiding this comment

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

where are the cli arguments defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asonnino https://github.com/MystenLabs/sui/blob/b14b4a7c4c74675ca539ddd7fb27f2ba00007af8/sui/src/benchmark/bench_types.rs

sui/src/benchmark.rs Outdated Show resolved Hide resolved
sui/src/benchmark.rs Outdated Show resolved Hide resolved
sui/src/benchmark.rs Outdated Show resolved Hide resolved
sui/src/benchmark.rs Outdated Show resolved Hide resolved
sui/src/benchmark.rs Outdated Show resolved Hide resolved
sui/src/benchmark.rs Outdated
let receiver = authority_client
.handle_batch_streaming_as_stream(BatchInfoRequest {
start,
end: start + 10_000,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the 10_000?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was added by @gdanezis
I simply ported it over.
In reality we want to benchmark this follower logic more thoroughly but thats still in the works

sui/src/benchmark/bench_types.rs Outdated Show resolved Hide resolved
mystenmark
mystenmark reviewed Apr 13, 2022
View reviewed changes
sui/src/benchmark.rs Outdated
let runtime = Builder::new_multi_thread()
.enable_all()
.thread_stack_size(32 * 1024 * 1024)
.worker_threads(usize::min(num_cpus::get(), 24))
Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on where this will be run, we might get more reproducible results if we use, say, half the CPUs.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Also the authority should probably use only half as well, looks like its using all cores right now)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried with half and still got same level of variance

@oxade oxade requested a review from gdanezis April 14, 2022 05:27
@oxade oxade merged commit 85ab554 into main Apr 14, 2022
@oxade oxade deleted the bench_reorg branch April 14, 2022 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mystenmark mystenmark mystenmark left review comments

@asonnino asonnino asonnino approved these changes

@lxfind lxfind Awaiting requested review from lxfind

@patrickkuo patrickkuo Awaiting requested review from patrickkuo

@bmwill bmwill Awaiting requested review from bmwill

@velvia velvia Awaiting requested review from velvia

@gdanezis gdanezis Awaiting requested review from gdanezis

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@oxade @asonnino @mystenmark