Add more banking stage benchmarks #24248

ckamm · 2022-04-11T16:54:11Z

These cover different batch sizes and account lock contention scenarios.

Some initial measurements for a different number of non-vote banking stage threads:

                     batch sz    1 threads   2 threads   3 threads
all same account       192         136ms       269ms       216ms
independent batches    192         136ms       109ms       106ms
two contentions        192           8ms         6ms         6ms
no same accounts       192           7ms         6ms         5ms
all same account        12          45ms       436ms (??)  142ms
independent batches     12          45ms        37ms        26ms
two contentions         12          14ms        11ms         8ms
no same accounts        12          11ms         9ms         6ms

After review feedback the PR was adjusted to improve banking-bench instead:

Add write-lock-contention option, replacing same_payer
write-lock-contention also has a same-batch-only value, where
contention happens only inside batches, not between them
Rename num-threads to batches-per-iteration, which is closer to what
it is actually doing.
Add num-banking-threads as a new option
Rename packets-per-chunk to packets-per-batch, because this is closer
to what's happening; and it was previously confusing that num-chunks
had little to do with packets-per-chunk.

Example output for a iterations=100 and a permutation of inputs:

contention,threads,batchsize,batchcount,tps
none,           3,192, 4,65290.30
none,           4,192, 4,77358.06
none,           5,192, 4,86436.65
none,           3, 12,64,43944.57
none,           4, 12,64,65852.15
none,           5, 12,64,70674.37
same-batch-only,3,192, 4,3928.21
same-batch-only,4,192, 4,6460.15
same-batch-only,5,192, 4,7242.85
same-batch-only,3, 12,64,11377.58
same-batch-only,4, 12,64,19582.79
same-batch-only,5, 12,64,24648.45
full,           3,192, 4,3914.26
full,           4,192, 4,2102.99
full,           5,192, 4,3041.87
full,           3, 12,64,11316.17
full,           4, 12,64,2224.99
full,           5, 12,64,5240.32

Problem

Banking stage benchmarks cover only cases without account lock contention and one batch size.

Summary of Changes

Add benchmarks.

ckamm · 2022-04-11T16:55:39Z

@taozhu-chicago Is this something you're interested in reviewing?

The main issue I see is that the new benchmarks can take a good while to execute. test::Bencher seems to run each function at least 300 times, meaning something that takes 400ms per iteration takes 120s to benchmark.

tao-stones · 2022-04-11T17:04:20Z

@taozhu-chicago Is this something you're interested in reviewing?

The main issue I see is that the new benchmarks can take a good while to execute. test::Bencher seems to run each function at least 300 times, meaning something that takes 400ms per iteration takes 120s to benchmark.

Thanks for adding these benchmarks, happy to look at it, maybe later today. Just a thought (without looking into details), would these scenarios fit better in banking_bench?

ckamm · 2022-04-11T17:24:20Z

Thanks for adding these benchmarks, happy to look at it, maybe later today. Just a thought (without looking into details), would these scenarios fit better in banking_bench?

Hah, I didn't know about banking_bench, will take a look tomorrow.

core/benches/banking_stage.rs

tao-stones · 2022-04-11T20:54:10Z

Actually, it looks better to add benches here as is

ckamm · 2022-04-12T12:07:29Z

Actually, it looks better to add benches here as is

Are you sure? I was about to port things to banking-bench because that seemed more flexible and I wouldn't need to put all the permutations of options into the code. Do you disagree and would prefer to have it stay like this?

sakridge · 2022-04-12T12:13:11Z

Actually, it looks better to add benches here as is

Are you sure? I was about to port things to banking-bench because that seemed more flexible and I wouldn't need to put all the permutations of options into the code. Do you disagree and would prefer to have it stay like this?

I prefer banking-bench as well actually.

ckamm · 2022-04-14T14:05:08Z

@sakridge @taozhu-chicago I've redone this PR to target banking-bench instead. Fresh reviews will be appreciated (note that I'm on vacation the next week)

- Add write-lock-contention option, replacing same_payer - write-lock-contention also has a same-batch-only value, where contention happens only inside batches, not between them - Rename num-threads to batches-per-iteration, which is closer to what it is actually doing. - Add num-banking-threads as a new option - Rename packets-per-chunk to packets-per-batch, because this is closer to what's happening; and it was previously confusing that num-chunks had little to do with packets-per-chunk. Example output for a iterations=100 and a permutation of inputs: contention,threads,batchsize,batchcount,tps none, 3,192, 4,65290.30 none, 4,192, 4,77358.06 none, 5,192, 4,86436.65 none, 3, 12,64,43944.57 none, 4, 12,64,65852.15 none, 5, 12,64,70674.37 same-batch-only,3,192, 4,3928.21 same-batch-only,4,192, 4,6460.15 same-batch-only,5,192, 4,7242.85 same-batch-only,3, 12,64,11377.58 same-batch-only,4, 12,64,19582.79 same-batch-only,5, 12,64,24648.45 full, 3,192, 4,3914.26 full, 4,192, 4,2102.99 full, 5,192, 4,3041.87 full, 3, 12,64,11316.17 full, 4, 12,64,2224.99 full, 5, 12,64,5240.32

tao-stones

lgtm

tao-stones · 2022-04-14T17:45:21Z

banking-bench/src/main.rs

-            if !same_payer {
-                new.message.account_keys[0] = solana_sdk::pubkey::new_rand();
-            }
-            new.message.account_keys[1] = solana_sdk::pubkey::new_rand();


+1 for replacing same_payer with contention on to account

codecov · 2022-04-14T19:22:46Z

Codecov Report

Merging #24248 (d3744d1) into master (2da4e3e) will increase coverage by 0.1%.
The diff coverage is 78.4%.

@@            Coverage Diff            @@
##           master   #24248     +/-   ##
=========================================
+ Coverage    81.8%    82.0%   +0.1%     
=========================================
  Files         581      580      -1     
  Lines      158312   161287   +2975     
=========================================
+ Hits       129518   132271   +2753     
- Misses      28794    29016    +222

* banking-bench: Migrate to clap 3.1.8 (cherry picked from commit 2c7699e) # Conflicts: # Cargo.lock * banking-bench: Add and rearrange options - Add write-lock-contention option, replacing same_payer - write-lock-contention also has a same-batch-only value, where contention happens only inside batches, not between them - Rename num-threads to batches-per-iteration, which is closer to what it is actually doing. - Add num-banking-threads as a new option - Rename packets-per-chunk to packets-per-batch, because this is closer to what's happening; and it was previously confusing that num-chunks had little to do with packets-per-chunk. Example output for a iterations=100 and a permutation of inputs: contention,threads,batchsize,batchcount,tps none, 3,192, 4,65290.30 none, 4,192, 4,77358.06 none, 5,192, 4,86436.65 none, 3, 12,64,43944.57 none, 4, 12,64,65852.15 none, 5, 12,64,70674.37 same-batch-only,3,192, 4,3928.21 same-batch-only,4,192, 4,6460.15 same-batch-only,5,192, 4,7242.85 same-batch-only,3, 12,64,11377.58 same-batch-only,4, 12,64,19582.79 same-batch-only,5, 12,64,24648.45 full, 3,192, 4,3914.26 full, 4,192, 4,2102.99 full, 5,192, 4,3041.87 full, 3, 12,64,11316.17 full, 4, 12,64,2224.99 full, 5, 12,64,5240.32 (cherry picked from commit d2c6c04) * attempt to fix `banking_bench` build, make it same as `dos` Co-authored-by: Christian Kamm <[email protected]> Co-authored-by: Tao Zhu <[email protected]>

mergify bot added the community Community contribution label Apr 11, 2022

mergify bot requested a review from a team April 11, 2022 16:54

tao-stones reviewed Apr 11, 2022

View reviewed changes

core/benches/banking_stage.rs Outdated Show resolved Hide resolved

core/benches/banking_stage.rs Outdated Show resolved Hide resolved

banking-bench: Migrate to clap 3.1.8

9604f20

ckamm force-pushed the ckamm/bench-banking-stage-contention branch from 0338c9a to 4e687dc Compare April 14, 2022 14:02

ckamm force-pushed the ckamm/bench-banking-stage-contention branch from 4e687dc to d3744d1 Compare April 14, 2022 16:23

tao-stones approved these changes Apr 14, 2022

View reviewed changes

tao-stones mentioned this pull request Apr 18, 2022

add option to send transfer to same account #24214

Closed

tao-stones merged commit d2c6c04 into solana-labs:master Apr 18, 2022

tao-stones mentioned this pull request Apr 25, 2022

Bench test with account contention to mimic transaction congestions #24646

Closed

carllin added the v1.10 label May 2, 2022

mergify bot mentioned this pull request May 2, 2022

Add more banking stage benchmarks (backport #24248) #24919

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more banking stage benchmarks #24248

Add more banking stage benchmarks #24248

ckamm commented Apr 11, 2022 •

edited

Loading

ckamm commented Apr 11, 2022

tao-stones commented Apr 11, 2022

ckamm commented Apr 11, 2022

tao-stones commented Apr 11, 2022

ckamm commented Apr 12, 2022

sakridge commented Apr 12, 2022

ckamm commented Apr 14, 2022

tao-stones left a comment

tao-stones Apr 14, 2022

codecov bot commented Apr 14, 2022

Add more banking stage benchmarks #24248

Add more banking stage benchmarks #24248

Conversation

ckamm commented Apr 11, 2022 • edited Loading

Problem

Summary of Changes

ckamm commented Apr 11, 2022

tao-stones commented Apr 11, 2022

ckamm commented Apr 11, 2022

tao-stones commented Apr 11, 2022

ckamm commented Apr 12, 2022

sakridge commented Apr 12, 2022

ckamm commented Apr 14, 2022

tao-stones left a comment

Choose a reason for hiding this comment

tao-stones Apr 14, 2022

Choose a reason for hiding this comment

codecov bot commented Apr 14, 2022

Codecov Report

ckamm commented Apr 11, 2022 •

edited

Loading