[Auto Benchmarks] Precision adjustment of latency #3538
Conversation
|
Sishan Long seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
| status: tide_disco::StatusCode::BAD_REQUEST, | ||
| message: "Peer's public configs are not ready".to_string(), | ||
| }); | ||
| return Ok(false); |
There was a problem hiding this comment.
If we're no longer throwing an error it seems like this function should just return a boolean instead of a result? How come we've switched this?
| println!("[{node_index}]: throughput: {throughput_bytes_per_sec} bytes/sec, avg_latency: {avg_latency_in_sec} sec."); | ||
| BenchResults { | ||
| partial_results: "Unset".to_string(), | ||
| avg_latency_in_sec, | ||
| num_latency, | ||
| minimum_latency_in_sec: minimum_latency, | ||
| maximum_latency_in_sec: maximum_latency, | ||
| minimum_latency_in_sec: minimum_latency as f64 / 1000.0, |
There was a problem hiding this comment.
Why not just make minimum/maximum_latency f64 to avoid this whole casting business?
| @@ -560,14 +561,14 @@ pub trait RunDa< | |||
| let throughput_bytes_per_sec = total_transactions_committed | |||
| * (transaction_size_in_bytes + 8) | |||
| / total_time_elapsed_sec; | |||
| let avg_latency_in_sec = total_latency / num_latency; | |||
| let avg_latency_in_sec = (total_latency / num_latency) as f64 / 1000.0; | |||
There was a problem hiding this comment.
We could just make total_latency an f64 since it appears to be measuring time, and just cast num_latency
| * (metrics.num_latency as f64) | ||
| + cur_metrics.avg_latency_in_sec * (cur_metrics.num_latency as f64)) | ||
| / (metrics.num_latency + cur_metrics.num_latency) as f64; |
There was a problem hiding this comment.
All this casting is a bit hard to follow. We first cast num_latency to f64, then cur_metrics' num_latency, then the combined operation of the both of them. Could we perhaps just cast them once as variables to make reading this statement easier? Right now a misplaced closing parenthesis could mess this whole statement up in a not-obvious way.
Closes #<ISSUE_NUMBER>
This PR:
wait_for_fn_from_orchestratorThis PR does not:
Key places to review: