[fastx benchmarking] Removed unnecessary TX order sending #93

oxade · 2021-12-26T20:24:36Z

Reasoning:
Current benchmark measures B-C-D and F-G-H and returns the sum.
In lines 160-164 we already certify the TX (which eliminates steps B-D), so there is no need to send the orders to the authorities.
However in measuring the throughput, should we include this roundtrip just for completeness sake? @gdanezis

By removing B-D, throughput is 34% higher => 35000tx/s vs 26000 tx/s

Solution
This PR provides the option to measure B-C-D and F-G-H separately, or both via CLI selector.
Will enhance this in future to optionally measure end-to-end (B-H and B-L)

In lines 160-164 we already certify the TX, so there is no need to send the orders to the authorities. However in measuring the throughput, should we include these just for completeness sake?

sblackshear

I think if we get rid of this, we also want to eliminate the / 2 here--otherwise, our throughput # will be off.
Perhaps it also makes sense to get rid of

// Serialize order
let bufx = serialize_order(&order);
assert!(!bufx.is_empty());

above?

oxade · 2021-12-26T23:35:22Z

I think if we get rid of this, we also want to eliminate the / 2 here--otherwise, our throughput # will be off.

Perhaps it also makes sense to get rid of
// Serialize order
let bufx = serialize_order(&order);
assert!(!bufx.is_empty());
above?

Yes and yes. Lots to cleanup in this code. Forgot to mark it as draft PR
But ultimately do you think we should count both roundtrips in our throughput measurement?

We can also measure both separately for internal purposes but choose one consistent method to use for marketing/publicity.

sblackshear · 2021-12-27T00:18:16Z

But ultimately do you think we should count both roundtrips in our throughput measurement?

I think counting F + G + H for throughput measurements (as this PR is doing) is simple + a good start. The old code (IIUC) was doing something a bit funky--it was including B + C + D + F + G + H (but not E), and it wasn't implemented in a way that forced D to finish before F begins (which might or might not skew the #'s).

oxade · 2021-12-27T00:41:44Z

But ultimately do you think we should count both roundtrips in our throughput measurement?

I think counting F + G + H for throughput measurements (as this PR is doing) is simple + a good start. The old code (IIUC) was doing something a bit funky--it was including B + C + D + F + G + H (but not E), and it wasn't implemented in a way that forced D to finish before F begins (which might or might not skew the #'s).

Precisely. Wanted to make sure I wasn't missing some reasoning behind the previous (funky) formulation.

Will make the changes to only count F+G+H. Thank you.

sblackshear · 2021-12-27T17:16:56Z

fastpay/src/bench.rs

@@ -105,12 +105,12 @@ impl ClientServerBenchmark {
        };

        // Pick an authority and create one state per shard.
-        let (public_auth0, secret_auth0) = keys.pop().unwrap();
+        let (public_auth0, secret_auth0) = &keys[0];


This will use the same key for every authority, which I don't think we want.

huitseeker · 2021-12-28T16:48:30Z

fastpay/src/bench.rs

@@ -34,16 +34,16 @@ struct ClientServerBenchmark {
    #[structopt(long, default_value = "9555")]
    port: u32,
    /// Size of the FastPay committee
-    #[structopt(long, default_value = "10")]
+    #[structopt(name = "committee-size", long, default_value = "10")]


OK, I think what you're looking for here, rather than those names, is a parameter rename-all = "kebab_case" on the enclosing structopt attribute of ClientServerBenchmark (doc)

gdanezis · 2021-12-28T19:22:43Z

In lines 160-164 we already certify the TX (which eliminates steps B-D), so there is no need to send the orders to the authorities.

The goal of this benchmark is to estimate the throughput that one authority can support. This is why we process both an order and a certificate: when an authority operates in the wild it will need to process (about) one order for each certificate. So here we simulate a load that is close to what we expect in the wild.

oxade · 2021-12-29T06:06:58Z

In lines 160-164 we already certify the TX (which eliminates steps B-D), so there is no need to send the orders to the authorities.

The goal of this benchmark is to estimate the throughput that one authority can support. This is why we process both an order and a certificate: when an authority operates in the wild it will need to process (about) one order for each certificate. So here we simulate a load that is close to what we expect in the wild.

@gdanezis so you wanna leave it as is?

gdanezis · 2021-12-29T15:26:41Z

In lines 160-164 we already certify the TX (which eliminates steps B-D), so there is no need to send the orders to the authorities.

The goal of this benchmark is to estimate the throughput that one authority can support. This is why we process both an order and a certificate: when an authority operates in the wild it will need to process (about) one order for each certificate. So here we simulate a load that is close to what we expect in the wild.

@gdanezis so you wanna leave it as is?

Right now the bench is measuring a reasonable proxy of cost of order + cost of cert processing. It might be worthwhile to have a flag that separate these two, and only measure one or the other. But just dropping the cost of processing orders just creates a blind spot I think.

sblackshear · 2021-12-29T17:29:58Z

But just dropping the cost of processing orders just creates a blind spot I think.

Makes sense to me. To suggest a path forward: let's leave the throughput benchmark as-is, and work on adding a new benchmark for latency that measures B -> L?

oxade · 2021-12-30T16:17:57Z

But just dropping the cost of processing orders just creates a blind spot I think.

Makes sense to me. To suggest a path forward: let's leave the throughput benchmark as-is, and work on adding a new benchmark for latency that measures B -> L?

@sblackshear @gdanezis

In lines 160-164 we already certify the TX (which eliminates steps B-D), so there is no need to send the orders to the authorities.

The goal of this benchmark is to estimate the throughput that one authority can support. This is why we process both an order and a certificate: when an authority operates in the wild it will need to process (about) one order for each certificate. So here we simulate a load that is close to what we expect in the wild.

@gdanezis so you wanna leave it as is?

Right now the bench is measuring a reasonable proxy of cost of order + cost of cert processing. It might be worthwhile to have a flag that separate these two, and only measure one or the other. But just dropping the cost of processing orders just creates a blind spot I think.

Created PR to selectively measure Orders, Certs or both (default as is now)
#106

oxade · 2021-12-30T16:19:21Z

But just dropping the cost of processing orders just creates a blind spot I think.

Makes sense to me. To suggest a path forward: let's leave the throughput benchmark as-is, and work on adding a new benchmark for latency that measures B -> L?

@sblackshear got it. Have this now: #106
If this approach is acceptable, will extend it to measure B-L

Removed unnecessary TX order sending

649639c

In lines 160-164 we already certify the TX, so there is no need to send the orders to the authorities. However in measuring the throughput, should we include these just for completeness sake?

oxade mentioned this pull request Dec 26, 2021

[Performance testing] Create a reference end-to-end test to measure performance #40

Closed

oxade requested review from gdanezis, huitseeker, patrickkuo and sblackshear December 26, 2021 20:45

sblackshear reviewed Dec 26, 2021

View reviewed changes

oxade marked this pull request as draft December 27, 2021 00:15

oxade and others added 2 commits December 26, 2021 19:58

Proper order counting

e63c56d

Added arg labels to benchmark command parser

ea4e5fd

oxade self-assigned this Dec 27, 2021

oxade marked this pull request as ready for review December 27, 2021 04:58

sblackshear reviewed Dec 27, 2021

View reviewed changes

huitseeker reviewed Dec 28, 2021

View reviewed changes

oxade changed the title ~~Removed unnecessary TX order sending~~ [fastx benchmarking] Removed unnecessary TX order sending Dec 29, 2021

oxade marked this pull request as draft December 29, 2021 05:34

oxade changed the title ~~[fastx benchmarking] Removed unnecessary TX order sending~~ [fastx benchmarking] Selectively Measure Different Code Paths Dec 30, 2021

oxade changed the title ~~[fastx benchmarking] Selectively Measure Different Code Paths~~ [fastx benchmarking] Selectively Benchmark Different Code Paths Dec 30, 2021

oxade changed the title ~~[fastx benchmarking] Selectively Benchmark Different Code Paths~~ [fastx benchmarking] Removed unnecessary TX order sending Dec 30, 2021

oxade mentioned this pull request Dec 30, 2021

Added options to benchmark orders vs certs #106

Merged

oxade closed this Dec 30, 2021

oxade deleted the bugfix/benchmark_throughput_excess_counting branch January 22, 2022 23:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fastx benchmarking] Removed unnecessary TX order sending #93

[fastx benchmarking] Removed unnecessary TX order sending #93

oxade commented Dec 26, 2021 •

edited

Loading

sblackshear left a comment

oxade commented Dec 26, 2021 •

edited

Loading

sblackshear commented Dec 27, 2021 •

edited

Loading

oxade commented Dec 27, 2021

sblackshear Dec 27, 2021

huitseeker Dec 28, 2021

gdanezis commented Dec 28, 2021

oxade commented Dec 29, 2021

gdanezis commented Dec 29, 2021

sblackshear commented Dec 29, 2021

oxade commented Dec 30, 2021

oxade commented Dec 30, 2021 •

edited

Loading

[fastx benchmarking] Removed unnecessary TX order sending #93

[fastx benchmarking] Removed unnecessary TX order sending #93

Conversation

oxade commented Dec 26, 2021 • edited Loading

sblackshear left a comment

Choose a reason for hiding this comment

oxade commented Dec 26, 2021 • edited Loading

sblackshear commented Dec 27, 2021 • edited Loading

oxade commented Dec 27, 2021

sblackshear Dec 27, 2021

Choose a reason for hiding this comment

huitseeker Dec 28, 2021

Choose a reason for hiding this comment

gdanezis commented Dec 28, 2021

oxade commented Dec 29, 2021

gdanezis commented Dec 29, 2021

sblackshear commented Dec 29, 2021

oxade commented Dec 30, 2021

oxade commented Dec 30, 2021 • edited Loading

oxade commented Dec 26, 2021 •

edited

Loading

oxade commented Dec 26, 2021 •

edited

Loading

sblackshear commented Dec 27, 2021 •

edited

Loading

oxade commented Dec 30, 2021 •

edited

Loading