Add analysis for bench-tps transactions by KirillLykov · Pull Request #92 · anza-xyz/agave

KirillLykov · 2024-03-05T16:58:22Z

Problem

There is no analysis of what happens with transactions submitted by bench-tps.
It doesn't submit metrics because it looks like we need first rework metrics in bench-tps and later add new if we need them. Hence, I decided to save everything in csv files first which has value by itself.

Summary of Changes

This PR adds optional analysis of the transactions submitted by bench-tps.
To do that, it creates a thread that requests all the blocks created during bench-tps run and filters the transactions.
This threads may optionally create two csv files: one for all the transactions and the other only for the blocks data.
The first is useful for debugging problems but too big to be generated for each run, while the second is generally good to have to understand how many transactions has been confirmed and some other stats.
Similar analysis was previously implemented in mango market making simulation client.

KirillLykov · 2024-03-06T14:37:54Z

Updated PR to address the comments of @ilya-bobyr

KirillLykov · 2024-03-06T16:43:26Z

+    client: &Arc<Client>,
+    block_data_file: Option<&str>,
+    transaction_data_file: Option<&str>,
+) -> (Option<LogTransactionService>, Option<SignatureBatchSender>)


I return pair of options instead of option of pair because I will need to pass these values independently to different functions.

codecov-commenter · 2024-03-08T16:18:41Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.9%. Comparing base (6024712) to head (7cd0d67).
⚠️ Report is 7056 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master      #92     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         840      840             
  Lines      228068   228068             
=========================================
- Hits       186828   186795     -33     
- Misses      41240    41273     +33

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

bw-solana · 2024-03-11T23:44:15Z

            let mut old_transactions = false;
-            let mut transactions = Vec::<_>::new();
            let mut min_timestamp = u64::MAX;
+            let mut transactions = Vec::<_>::with_capacity(txs0.len());


could use the local tx_len variable for these instead

bw-solana · 2024-03-12T00:01:52Z

+}
+
+// How often process blocks.
+const PROCESS_BLOCKS_EVERY_MS: u64 = 16 * DEFAULT_MS_PER_SLOT;


Why 16? Would be nice to drop a comment for why we chose this

bw-solana · 2024-03-12T00:02:34Z

+
+// How often process blocks.
+const PROCESS_BLOCKS_EVERY_MS: u64 = 16 * DEFAULT_MS_PER_SLOT;
+// Max age for transaction in the transaction map.


age here means time we will wait for a tx before giving up on it?

yeah, probably better to be more explicit in the comment

bw-solana · 2024-03-12T00:08:23Z

+const NUM_RETRY: u64 = 5;
+const RETRY_EVERY_MS: u64 = 4 * DEFAULT_MS_PER_SLOT;
+
+fn call_rpc_with_retry<Func, Data>(f: Func, retry_warning: &str) -> Result<Data>


This seems a little funky to have buried in log_transaction_service. Is there anywhere else this function might make sense to live?

bw-solana · 2024-03-12T00:09:53Z

+    }
+}
+
+fn verify_data_files(block_data_file: Option<&str>, transaction_data_file: Option<&str>) -> bool {


verify here is a little misleading. Maybe data_file_provided ?

bw-solana

Looks pretty good to me - left a few minor comments.

Do you have a sense for how large a block/transaction file will be generated for a given run time (assuming a standard transaction load)?

KirillLykov · 2024-03-12T09:01:04Z

@bw-solana transactions file might be as big as 100MB for 10min run of bench-tps, so I want to use it only for debug purposes. The block file is quite small, like 1.4MB for one hour of simulation.

bw-solana · 2024-03-12T15:46:30Z

@bw-solana transactions file might be as big as 100MB for 10min run of bench-tps, so I want to use it only for debug purposes. The block file is quite small, like 1.4MB for one hour of simulation.

Nice, that's actually smaller than I was expecting

gregcusack

looks pretty good to me. just some small nits

gregcusack · 2024-03-12T04:30:52Z

+    fn new(transaction_data_file: Option<&str>) -> Self {
+        let transaction_log_writer = transaction_data_file.map(|transaction_data_file| {
+            CsvFileWriter::from_writer(
+                File::create(transaction_data_file).expect("File can be created."),


nit: File cannot be created.

I found in docs We recommend that expect messages are used to describe the reason you expect the Result should be Ok.. Following this logic, maybe something like "application should be able to create a file"?

oh this is good to know! and ya that sounds good to me!

gregcusack · 2024-03-12T04:32:10Z

+impl BlockLogWriter {
+    fn new(block_data_file: Option<&str>) -> Self {
+        let block_log_writer = block_data_file.map(|block_data_file| {
+            CsvFileWriter::from_writer(File::create(block_data_file).expect("File can be created."))


nit: File cannot be created.

gregcusack · 2024-03-12T04:35:03Z

+        let commitment: CommitmentConfig = CommitmentConfig {
+            commitment: CommitmentLevel::Confirmed,
+        };


worth pulling this in from the cli using --commitment-config?

it is used to get blocks data and from the discussion in the previous PR it seems that it should work only with confirmed. Probably, makes sense to add corresponding comment there.

gregcusack · 2024-03-12T04:41:11Z

+            .spawn(move || {
+                Self::run(client, signature_receiver, tx_log_writer, block_log_writer);
+            })
+            .expect("LogTransactionService is up.");


ok general comment: doesn't the .expect(<message>) trigger when the unwrap on the object fails? So shouldn't the <message> reflect why/what failed rather than the success path? or am i confused?

looks like I misunderstood the expect phrasing, I will redo it everywhere as described in #92 (comment)

gregcusack · 2024-03-12T16:54:35Z

+            let is_not_timeout_tx =
+                duration_since_past_time.num_seconds() < REMOVE_TIMEOUT_TX_EVERY_SEC;
+            if !is_not_timeout_tx {


i'd maybe change the logic. having !is_not_timeout_tx {...} is a double negative and can be weird to read. I'd change to using something like:

let is_timeout_tx = duration_since_past_time.num_seconds() >= REMOVE_TIMEOUT_TX_EVERY_SEC;

and then return !is_timeout_tx.

KirillLykov · 2024-03-14T10:45:18Z

There is a bug in the loop with select: the processing of blocks takes longer time than specified so when sender has been dropped, we will always go to receiver for timer processing blocks which are irrelevant and never stopping the service. The solution could be:

create two threads: one for transactions handling and the other for blocks. This way we can wait all the incoming signatures has been handled and block stats has been updated. This would require to make communication between these two thread more complicated (instead of HashMap use DashMap).
implement this part with tokio. Probably, performance-wise would be the best but I don't want to introduce tokio as part of this already complicated PR and this also will lead to more sophisticated communication between threads.
check the size of the receiver, if not 0, skip writing blocks information for now. Probably in combination with limited number of blocks to download, might work.

CriesofCarrots · 2024-03-15T01:25:46Z

I'll defer to the other reviewers you have on this one.

gregcusack · 2024-03-15T16:24:44Z

There is a bug in the loop with select: the processing of blocks takes longer time than specified so when sender has been dropped, we will always go to receiver for timer processing blocks which are irrelevant and never stopping the service.

So, when bench-tps stops sending transactions, the receiver continues to process blocks and write them to the CSV, but we don't actually care about these "empty" blocks, right? And we need a way to basically stop the receiver once the sender stops. Is that right?

gregcusack · 2024-03-15T16:43:27Z

create two threads: one for transactions handling and the other for blocks. This way we can wait all the incoming signatures has been handled and block stats has been updated. This would require to make communication between these two thread more complicated (instead of HashMap use DashMap).

implement this part with tokio. Probably, performance-wise would be the best but I don't want to introduce tokio as part of this already complicated PR and this also will lead to more sophisticated communication between threads.

check the size of the receiver, if not 0, skip writing blocks information for now. Probably in combination with limited number of blocks to download, might work.

Simpler is better. so would vote for (3) if you can get it to work. Seems doable.

KirillLykov · 2024-03-18T14:02:01Z

create two threads: one for transactions handling and the other for blocks. This way we can wait all the incoming signatures has been handled and block stats has been updated. This would require to make communication between these two thread more complicated (instead of HashMap use DashMap).

implement this part with tokio. Probably, performance-wise would be the best but I don't want to introduce tokio as part of this already complicated PR and this also will lead to more sophisticated communication between threads.

check the size of the receiver, if not 0, skip writing blocks information for now. Probably in combination with limited number of blocks to download, might work.

Simpler is better. so would vote for (3) if you can get it to work. Seems doable.

i've first implemented 1 but the code is cumbersome. I'm trying with (3), I use it vs invalidator and tesnet, will see how good it is.

KirillLykov · 2024-03-25T13:18:00Z

@gregcusack please let me know if this change 0f93cd3 seems to be clear

gregcusack · 2024-03-25T17:55:13Z

@gregcusack please let me know if this change 0f93cd3 seems to be clear

much more clear! i appreciate the change

gregcusack

lgtm! i like how you dealt with endless block processing bug. seems simple yet effective. appreciate the clarity on is_timeout_tx

KirillLykov force-pushed the klykov/bench-tps-txs-analysis branch from 3e977b0 to 81bbd37 Compare March 5, 2024 17:00

KirillLykov requested a review from illia-bobyr March 5, 2024 17:10

KirillLykov mentioned this pull request Mar 5, 2024

Add get_blocks and get_slot methods to bench-tps-client #94

Merged

KirillLykov commented Mar 6, 2024

View reviewed changes

KirillLykov force-pushed the klykov/bench-tps-txs-analysis branch 2 times, most recently from 65ebb59 to 0787a57 Compare March 8, 2024 13:48

KirillLykov marked this pull request as ready for review March 8, 2024 13:48

KirillLykov requested review from CriesofCarrots, bw-solana and gregcusack March 8, 2024 13:48

bw-solana reviewed Mar 11, 2024

View reviewed changes

bw-solana reviewed Mar 12, 2024

View reviewed changes

gregcusack reviewed Mar 12, 2024

View reviewed changes

CriesofCarrots removed their request for review March 15, 2024 01:25

KirillLykov force-pushed the klykov/bench-tps-txs-analysis branch from 0787a57 to 5742a4c Compare March 22, 2024 17:16

KirillLykov added 2 commits March 24, 2024 13:53

save progress

91e9d92

rename threads handler

99312e7

KirillLykov added 18 commits March 24, 2024 13:53

extract LogWriter

7cac96c

Replace pair TimestampedTransaction with struct

43e538c

add compute_unit_price to TimestampedTransaction

4e295a0

add cu_price to LogWriter

afcfb24

add block time to the logs

c2ac294

Fix warnings

e0f5515

add comments and restructure code

71a360d

some small improvements

d67f7b4

Renamed conformation_processing.rs to log_transaction_service.rs

15b44af

address numerous PR comments

f9740e7

split LogWriter into two structs

c4124ca

simplify code of LogWriters

e5aa95f

extract process_blocks

1606b33

specify commitment in LogTransactionService

9a43ceb

break thread loop if receiver happens to be dropped

2d7699a

update start_slot when processing blocks

03b3315

address pr comments

304fcf2

fix clippy error

4374f5a

KirillLykov force-pushed the klykov/bench-tps-txs-analysis branch from 29a466f to 4374f5a Compare March 24, 2024 13:54

minor changes

57db349

KirillLykov requested review from bw-solana and gregcusack March 24, 2024 14:16

fix ms problem

54adc0c

KirillLykov commented Mar 24, 2024

View reviewed changes

Comment thread bench-tps/src/log_transaction_service.rs Outdated

fix bug with time in clear transaction map

7cd0d67

KirillLykov force-pushed the klykov/bench-tps-txs-analysis branch from 0f93cd3 to 7cd0d67 Compare March 25, 2024 13:58

gregcusack approved these changes Mar 25, 2024

View reviewed changes

KirillLykov merged commit 1261f1f into anza-xyz:master Mar 26, 2024

Conversation

KirillLykov commented Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

KirillLykov commented Mar 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bw-solana left a comment

Choose a reason for hiding this comment

Uh oh!

KirillLykov commented Mar 12, 2024

Uh oh!

bw-solana commented Mar 12, 2024

Uh oh!

gregcusack 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!

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!

KirillLykov Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

KirillLykov commented Mar 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CriesofCarrots commented Mar 15, 2024

Uh oh!

gregcusack commented Mar 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gregcusack commented Mar 15, 2024

Uh oh!

KirillLykov commented Mar 18, 2024

Uh oh!

Uh oh!

KirillLykov commented Mar 25, 2024

Uh oh!

gregcusack commented Mar 25, 2024

Uh oh!

gregcusack left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

KirillLykov commented Mar 5, 2024 •

edited

Loading

codecov-commenter commented Mar 8, 2024 •

edited

Loading

KirillLykov Mar 13, 2024 •

edited

Loading

KirillLykov commented Mar 14, 2024 •

edited

Loading

gregcusack commented Mar 15, 2024 •

edited

Loading