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

bench get_retransmit_peers #23292

Merged
merged 9 commits into from
Mar 2, 2022

Conversation

jbiseda
Copy link
Contributor

@jbiseda jbiseda commented Feb 23, 2022

Problem

No bench for baseline get_retransmit_peers

Summary of Changes

Refactor to add benches for get_retransmit_peers_compat and get_retransmit_peers_deterministic

Fixes #

@codecov
Copy link

codecov bot commented Feb 24, 2022

Codecov Report

Merging #23292 (e558d27) into master (eff085f) will increase coverage by 0.0%.
The diff coverage is 89.3%.

❗ Current head e558d27 differs from pull request most recent head d011540. Consider uploading reports for the commit d011540 to get more accurate results

@@           Coverage Diff            @@
##           master   #23292    +/-   ##
========================================
  Coverage    81.3%    81.3%            
========================================
  Files         572      572            
  Lines      155876   156085   +209     
========================================
+ Hits       126815   127025   +210     
+ Misses      29061    29060     -1     

@jbiseda jbiseda marked this pull request as ready for review February 24, 2022 22:40
let mut rng = rand::thread_rng();
let (nodes, cluster_nodes) = make_cluster_nodes(&mut rng);

let mut shred_seed = [0u8; 32];
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to generate a different seed on every iteration to test different behaviors?

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've changed this slightly to more accurately represent the seed generation for each case and to include multiple shreds in the calculation. I didn't want to include random generation of the inputs to get_retransmit_peers_* measurements.

Comment on lines 499 to 503
if rng.gen_ratio(1, 7) {
None // No stake for some of the nodes.
} else {
Some((node.id, rng.gen_range(0, 20)))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, how does this ratio of unstaked:staked nodes of 1:6 affect the performance of the shuffle? Does more or less unstaked/staked nodes make perf faster?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some tests for comparison:

running 4 tests
test bench_get_retransmit_peers_compat_unstaked_ratio_1_2         ... bench:   1,679,677 ns/iter (+/- 2,628)
test bench_get_retransmit_peers_compat_unstaked_ratio_1_32        ... bench:   1,690,616 ns/iter (+/- 3,048)
test bench_get_retransmit_peers_deterministic_unstaked_ratio_1_2  ... bench:   1,478,714 ns/iter (+/- 13,556)
test bench_get_retransmit_peers_deterministic_unstaked_ratio_1_32 ... bench:   2,476,904 ns/iter (+/- 39,032)

test result: ok. 0 passed; 0 failed; 0 ignored; 4 measured; 0 filtered out; finished in 5.22s

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for the insights!

behzadnouri
behzadnouri previously approved these changes Mar 1, 2022
Copy link
Contributor

@behzadnouri behzadnouri left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -39,7 +43,8 @@ enum NodeId {
Pubkey(Pubkey),
}

struct Node {
// pub for bench
Copy link
Contributor

Choose a reason for hiding this comment

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

If someone removes pub, the code will not compile; so the comment will be redundant.
Similarly other places below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

Comment on lines 482 to 483
// pub for bench
pub fn make_cluster<R: Rng>(
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe rename this to make_rand_cluster to indicate this is a test/simulation only function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to make_test_cluster

carllin
carllin previously approved these changes Mar 1, 2022
@mergify mergify bot dismissed stale reviews from behzadnouri and carllin March 2, 2022 00:44

Pull request has been modified.

@jbiseda jbiseda merged commit c69e3b7 into solana-labs:master Mar 2, 2022
@jbiseda jbiseda deleted the deterministic-turbine-bench branch March 2, 2022 03:11
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Mar 4, 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.

3 participants