Refactor benching harness to separate out client and server connections #4113

tinzh · 2023-07-25T23:40:20Z

Description of changes:

Right now, only pairs of connections of a given TLS library can be benched. This refactor separates out the connection pairs into individual connections that can be mixed and matched into pairs; for example, a Rustls client could be tested with a s2n-tls server. This refactor also allows the reusing of configs between connections, a common behavior when many connections with the same config options need to made.

Call-outs:

The diff is huge because this refactor changes the trait definition, but when comparing the files side-by-side manually, there are a lot more similarities.

harness.rs is now pretty big at ~450 lines. I still think all of the code makes sense to have in one file, however, since harness.rs contains everything common to all of the TLS implementations.

Testing:

To test, run all of the benchmarks with and without AWS-LC (cargo bench, memory/bench-memory.sh, and historical-perf/bench-past.sh), as well as cargo test. To make sure the behavior hasn't changed, run all of the benchmarks before and after the refactor.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

bindings/rust/bench/src/harness.rs

bindings/rust/bench/src/rustls.rs

bindings/rust/bench/certs/generate_certs.sh

bindings/rust/bench/src/harness.rs

jmayclin · 2023-07-26T20:21:57Z

bindings/rust/bench/benches/throughput.rs

-                |harness| {
-                    if let Ok(harness) = harness {
-                        let _ = harness.round_trip_transfer(shared_buf);
+                |conn_pair_res| {


nit: I think I like conn_pair over conn_pair_res

It's a Result, so I want to qualify that with the variable name to make it extra clear; if someone later tried to change this, the type of the variable would hopefully be one less thing to reason through.

jmayclin · 2023-07-26T20:25:14Z

bindings/rust/bench/certs/generate_certs.sh

@@ -62,13 +63,13 @@ cert-gen () {

 if [[ $1 != "clean" ]] 


not in scope for this PR

I kind of want to slap myself for suggesting this, but would a make file make sense here? You could define the targets for the various certificate types, which would allow you to specify the other files that need to exist. And make clean is a well recognized idiom.

There other files that need to exist in order to generate the certs are all deleted afterwards anyways, while (to my understanding) a Makefile is especially useful if you don't need to recompile everything for a small code change, since the intermediate artifacts (like .o files) are cached. With the certs, there's not really a concept for the small code change; sure you can keep the private keys around to be able to regenerate and resign the certs, but there's no need for that for benching. Also, something that might be done in the future is to change how long the cert chain is, which feels more natural as a command line argument than as an option in a Makefile. Overall, Makefiles add more complexity than needed; hopefully the script is fairly readable as-is.

jmayclin · 2023-07-26T22:24:34Z

bindings/rust/bench/src/harness.rs

+impl Debug for HandshakeType {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{}", match self {
+            Self::ServerAuth => "",


It feels like there should be at least something here. Perhaps just "server auth"

Having the debug names be ServerAuth and mTLS or ClientAuth would make the bench group names like handshake-ServerAuth-x25519-ecdsa384 and handshake-ClientAuth-x25519-ecdsa384. Ideally, I'd want the bench group names to be handshake-x25519-ecdsa384 for non-mTLS and handshake-mTLS-x25519-ecdsa384 for mTLS, but there's no good way to do that with debug names alone. (Using the backspace control character \u{8} as the debug string for ServerAuth did do the trick, but it was definitely too hacky.)

At first I was against it, but I think adding a little of matching logic to the handshake benchmark would make the most sense and be the most clear. I'll do that instead of manually implementing Debug for HandshakeType.

bindings/rust/bench/src/harness.rs

jmayclin · 2023-07-26T22:31:36Z

bindings/rust/bench/src/harness.rs

+
+    /// Run handshake on connections
+    pub fn handshake(&mut self) -> Result<(), Box<dyn Error>> {
+        for _ in 0..2 {


I wonder whether it's worth adding a comment about why we still need 2 rtt's for TLS 1.3 (waiting for the server finished message)

Yeah, I think this is a good place to call it out. I'll add it.

bindings/rust/bench/src/s2n_tls.rs

jmayclin · 2023-07-27T20:47:56Z

bindings/rust/bench/src/harness.rs

+    }
+
+    /// Take back ownership of individual connections in the TlsConnPair
+    pub fn unwrap(self) -> (C, S) {


unwrap is confusing naming given that unwrap already has a defined use in the Rust ecosystem. Perhaps consume?

consume makes it sound like that the connections are being used up and turned invalid. Would split() or take_ownership() work? I'm leaning towards split() personally, so I'll commit that change for now.

Alternately, is ownership actually important? if not inner() -> (&C, &S) is a pretty common pattern

This is useful for my memory benching experiments where a connection is handshaked but only the client or only the server is stored and not dropped.

bindings/rust/bench/benches/handshake.rs

commit eafb8a2 Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Mon Jul 31 18:05:50 2023 -0400 shell -> bash commit 12071b8 Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Mon Jul 31 17:59:09 2023 -0400 add ubuntu quickstart back to readme commit 10bf557 Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Mon Jul 31 17:52:06 2023 -0400 fixes commit 74adf8d Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Mon Jul 31 16:46:19 2023 -0400 fixes commit 0548d07 Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Mon Jul 31 16:43:08 2023 -0400 consolidate commit cbe8f2d Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Mon Jul 31 14:55:30 2023 -0400 remove old doc sections commit f194321 Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Mon Jul 31 12:25:28 2023 -0400 more content commit 882eb1d Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Mon Jul 31 09:08:24 2023 -0400 fixes commit ce37d0e Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Mon Jul 31 09:03:45 2023 -0400 fixes commit 011d15f Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Sat Jul 29 22:59:51 2023 -0400 cmake consuming commit 7feadc1 Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Sat Jul 29 22:27:02 2023 -0400 fixes commit 2914950 Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Sat Jul 29 21:34:24 2023 -0400 traditional make commit 02f9841 Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Sat Jul 29 19:45:43 2023 -0400 s2n-tls build section commit 86c4983 Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Sat Jul 29 11:56:32 2023 -0400 Update build documentation commit ea6d02a Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Fri Jul 28 16:49:21 2023 -0400 bindings: release 0.0.35 (aws#4122) commit 35d08ba Author: Justin Zhang <76919968+tinzh@users.noreply.github.com> Date: Fri Jul 28 12:31:21 2023 -0700 refactor(bench): separate out client and server connections in benching harness (aws#4113) Enables more better control of connections for benching experiments commit 65e74ca Author: Lindsay Stewart <slindsay@amazon.com> Date: Wed Jul 26 02:26:40 2023 -0700 Print error for 32bit test (aws#4107) commit b0b253e Author: toidiu <apoorv@toidiu.com> Date: Wed Jul 26 00:30:44 2023 -0700 ktls: set keys on socket and enable ktls (aws#4071) commit 403d5e6 Author: Lindsay Stewart <slindsay@amazon.com> Date: Tue Jul 25 16:03:09 2023 -0700 Trying to use an invalid ticket should not mutate state (aws#4110) commit bce2b1a Author: James Mayclin <maycj@amazon.com> Date: Tue Jul 25 14:44:33 2023 -0700 fix: get_session behavior for TLS 1.3 (aws#4104) commit 6881358 Author: Justin Zhang <76919968+tinzh@users.noreply.github.com> Date: Tue Jul 25 10:10:21 2023 -0700 feat(bench): add different certificate signature algorithms to benchmarks (aws#4080) commit aab13d5 Author: Justin Zhang <76919968+tinzh@users.noreply.github.com> Date: Mon Jul 24 18:17:30 2023 -0700 feat(bench): add memory bench with valgrind/massif (aws#4081) commit 20b0174 Author: Justin Zhang <76919968+tinzh@users.noreply.github.com> Date: Mon Jul 24 13:26:32 2023 -0700 feat(bench): add historical performance benchmark (aws#4083) commit 5cc827d Author: Doug Chapman <54039637+dougch@users.noreply.github.com> Date: Thu Jul 20 11:50:50 2023 -0700 nix: pin corretto version (aws#4103)

tinzh added 2 commits July 25, 2023 22:33

refactor

f3ec078

renamed some enums, made names clearer

a889007

tinzh requested review from jmayclin and maddeleine July 26, 2023 18:41

tinzh marked this pull request as ready for review July 26, 2023 18:42

maddeleine reviewed Jul 26, 2023

View reviewed changes

jmayclin reviewed Jul 26, 2023

View reviewed changes

tinzh added 4 commits July 26, 2023 23:23

add comment of third argument to function

d1a3c42

callout 2rtt handshake in harness

bfb9c44

ran rustfmt and changed where mtls bench group name logic is

d5ca8d1

removed old comment

0947ef0

tinzh requested review from maddeleine and jmayclin July 27, 2023 00:13

jmayclin reviewed Jul 27, 2023

View reviewed changes

rename TlsConnPair::unwrap to split

0997033

tinzh requested a review from jmayclin July 27, 2023 20:52

maddeleine approved these changes Jul 28, 2023

View reviewed changes

jmayclin reviewed Jul 28, 2023

View reviewed changes

bindings/rust/bench/benches/handshake.rs Outdated Show resolved Hide resolved

move internal functions out in benches

47bfd09

tinzh requested a review from jmayclin July 28, 2023 17:13

jmayclin approved these changes Jul 28, 2023

View reviewed changes

Merge branch 'main' into refactor-harness

8dc1bd3

tinzh enabled auto-merge (squash) July 28, 2023 18:34

tinzh merged commit 35d08ba into aws:main Jul 28, 2023
28 checks passed

tinzh deleted the refactor-harness branch August 2, 2023 20:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor benching harness to separate out client and server connections #4113

Refactor benching harness to separate out client and server connections #4113

tinzh commented Jul 25, 2023 •

edited

Loading

jmayclin Jul 26, 2023

tinzh Jul 26, 2023

jmayclin Jul 26, 2023

tinzh Jul 26, 2023

jmayclin Jul 26, 2023

tinzh Jul 26, 2023

jmayclin Jul 26, 2023

tinzh Jul 26, 2023

jmayclin Jul 27, 2023

tinzh Jul 27, 2023

jmayclin Jul 27, 2023

tinzh Jul 27, 2023

Refactor benching harness to separate out client and server connections #4113

Refactor benching harness to separate out client and server connections #4113

Conversation

tinzh commented Jul 25, 2023 • edited Loading

Description of changes:

Call-outs:

Testing:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tinzh commented Jul 25, 2023 •

edited

Loading