Improve bench-tps keypair generation by jstarry · Pull Request #7723 · solana-labs/solana

jstarry · 2020-01-09T16:48:38Z

Problem

Account generation at the beginning of bench-tps can sometimes be really slow. This could be due to a number of reasons:

If a large portion of a batch of transfers fails, generation has no way to bail out of attempting to verify each transfer.
RPC requests are slow due to geographic latency

Summary of Changes

Parallelize fund verification so that RPC requests are less of a bottleneck
Short circuit fund verification if more than half of the tx's failed
Short circuit verification of a tx if any of the "to" keypairs is funded or not funded
Transfer direction changes more frequently so that source/dest keypair funds stay balanced
Made the initial funds check much more lenient so that we can avoid re-running fund generation
General code cleanup

Fixes: #7597

codecov · 2020-01-10T13:52:32Z

Codecov Report

Merging #7723 into master will decrease coverage by <.1%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master   #7723     +/-   ##
========================================
- Coverage    81.7%   81.7%   -0.1%     
========================================
  Files         241     241             
  Lines       50731   50731             
========================================
- Hits        41491   41486      -5     
- Misses       9240    9245      +5

jstarry · 2020-01-10T15:13:40Z

@sakridge @mvines this is ready for review now

mvines · 2020-01-13T15:55:14Z

@sakridge has a better eye than I do in for this part of the code, I defer review to him

sakridge · 2020-01-13T23:41:30Z

    let keypair_chunks = source_keypair_chunks.len() as u64;
    let mut reclaim_lamports_back_to_source_account = false;
-    let mut i = keypair0_balance;
+    let mut i = 0;


why change this?

So that mechanism was in place to quickly restart bench-tps with the same parameters as a previous run. We would detect the balance of the last keypair to check how many lamports had been transferred from one half of they keypairs to the other half. But I found that the assumptions were not valid for running with ramp-tps.

In this PR, I removed the quick-start mechanism but I think that was a mistake. I'll think of another approach that works well for repeated bench-tps runs as well as incremental ramp-tps runs

sakridge · 2020-01-14T00:01:06Z

-                        if timer.elapsed() >= Duration::from_secs(5) {
-                            if failed_verify > 0 {
-                                debug!("total txs failed verify: {}", failed_verify);
+                    let failed_verify = Arc::new(AtomicUsize::new(0));


what do you think about breaking this up into another function. The complexity was bad before, now it's even worse.

Totally in favour, I'll clean this up

sakridge · 2020-01-14T00:02:19Z

Overall question, why are the funding transactions failing in the original?

jstarry

Overall question, why are the funding transactions failing in the original?

To be honest, not exactly sure but it's likely either:

Chosen node for funding txs goes down or is unresponsive.
Another bench-tps client is already running and funding txs get dropped by the network

This PR doesn't aim to fix the root cause for failed funding txs but it does aim to make funding faster and it will bail on verifying the fund txs if it comes across too many failures. In that case, the funding txs would be sent again (helping with 2.) and the multi client might pick a new RPC node (helping with 1.)

jstarry · 2020-01-14T07:02:26Z

-                        if timer.elapsed() >= Duration::from_secs(5) {
-                            if failed_verify > 0 {
-                                debug!("total txs failed verify: {}", failed_verify);
+                    let failed_verify = Arc::new(AtomicUsize::new(0));


Totally in favour, I'll clean this up

jstarry · 2020-01-14T07:03:51Z

    let keypair_chunks = source_keypair_chunks.len() as u64;
    let mut reclaim_lamports_back_to_source_account = false;
-    let mut i = keypair0_balance;
+    let mut i = 0;


So that mechanism was in place to quickly restart bench-tps with the same parameters as a previous run. We would detect the balance of the last keypair to check how many lamports had been transferred from one half of they keypairs to the other half. But I found that the assumptions were not valid for running with ramp-tps.

In this PR, I removed the quick-start mechanism but I think that was a mistake. I'll think of another approach that works well for repeated bench-tps runs as well as incremental ramp-tps runs

jstarry · 2020-01-14T15:25:45Z

@sakridge I updated the PR and it's ready for another pass. I updated the logic around transfer direction to be a lot simpler. I tested the changes briefly on a cpu testnet and behaviour looks good. I'll run with gpus later today to make sure tps isn't affected.

sakridge · 2020-01-14T19:48:53Z

@sakridge I updated the PR and it's ready for another pass. I updated the logic around transfer direction to be a lot simpler. I tested the changes briefly on a cpu testnet and behaviour looks good. I'll run with gpus later today to make sure tps isn't affected.

ok cool, i'll take a look

sakridge · 2020-01-14T20:03:24Z

+    // 100 lamports should give enough wiggle room to handle source / dest keypair sets getting unbalanced
+    let minimum_balance = 100;
+
+    if first_keypair_balance < minimum_balance || last_keypair_balance < minimum_balance {


I'm not sure I get this minimum balance check fixed at 100. The user could have specified 1000 or 10,000 lamports per account, then if they have only 100 they would not be funded, is that correct? The cluster fees can be in the 1000 lamport range also, so I'm not sure a fixed low value here will work in all cases.

@sakridge you're right, this was oversimplified and didn't take tx fees into account. I updated the PR with a new approach: 78516ca

jstarry · 2020-01-15T02:20:47Z

I wasn't able to get a gpu testnet working but ran a gce cpu testnet with 25k avg tps and tx errors were pretty quiet so I feel pretty confident in this new approach of switching transfer directions more frequently.

EDIT: Metrics link: https://metrics.solana.com:3000/d/testnet-edge/testnet-monitor-edge?orgId=2&from=1579052950236&to=1579053426330&var-datasource=Solana%20Metrics%20(read-only)&var-testnet=testnet-dev-jstarry&var-hostid=All

jstarry · 2020-01-16T07:29:09Z

@sakridge can you take another look? Thanks!

sakridge

lgtm, nice cleanup!

* Improve bench-tps keypair generation * Fix tests * Fix move test * cargo fmt * Split up funding function into smaller functions * Support restarting bench-tps without re-funding * Change quick start logic and remove noisy log (cherry picked from commit b78b1bb)

automerge

jstarry requested review from mvines and sakridge January 9, 2020 16:48

jstarry force-pushed the improve-bench-tps-generation branch 2 times, most recently from 6735635 to ad668c0 Compare January 9, 2020 17:03

jstarry added the v0.22 label Jan 9, 2020

jstarry added 2 commits January 10, 2020 09:06

Improve bench-tps keypair generation

12e5d6a

Fix tests

d5f22ba

jstarry force-pushed the improve-bench-tps-generation branch from ad668c0 to d5f22ba Compare January 10, 2020 01:06

Fix move test

e5c9718

cargo fmt

df774b2

mvines removed their request for review January 13, 2020 15:55

sakridge reviewed Jan 13, 2020

View reviewed changes

sakridge reviewed Jan 14, 2020

View reviewed changes

jstarry commented Jan 14, 2020

View reviewed changes

jstarry added 2 commits January 14, 2020 21:10

Split up funding function into smaller functions

4bc3ef4

Support restarting bench-tps without re-funding

69cbb82

jstarry force-pushed the improve-bench-tps-generation branch from 0f187f5 to 69cbb82 Compare January 14, 2020 14:27

sakridge reviewed Jan 14, 2020

View reviewed changes

Change quick start logic and remove noisy log

78516ca

sakridge approved these changes Jan 16, 2020

View reviewed changes

jstarry merged commit b78b1bb into solana-labs:master Jan 17, 2020

jstarry deleted the improve-bench-tps-generation branch January 17, 2020 02:35

mergify Bot mentioned this pull request Jan 17, 2020

Improve bench-tps keypair generation (bp #7723) #7853

Merged

solana-grimes pushed a commit that referenced this pull request Jan 17, 2020

Improve bench-tps keypair generation (#7723) (#7853)

ee1300a

automerge

Conversation

jstarry commented Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

codecov Bot commented Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jstarry commented Jan 10, 2020

Uh oh!

mvines commented Jan 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sakridge commented Jan 14, 2020

Uh oh!

jstarry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jstarry commented Jan 14, 2020

Uh oh!

sakridge commented Jan 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jstarry commented Jan 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jstarry commented Jan 16, 2020

Uh oh!

sakridge left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jstarry commented Jan 9, 2020 •

edited

Loading

codecov Bot commented Jan 10, 2020 •

edited

Loading

jstarry commented Jan 15, 2020 •

edited

Loading