Skip to content
This repository has been archived by the owner on Nov 29, 2023. It is now read-only.

pod: add QUIC benchmarks #516

Closed
wants to merge 8 commits into from
Closed

pod: add QUIC benchmarks #516

wants to merge 8 commits into from

Conversation

kckeiks
Copy link
Collaborator

@kckeiks kckeiks commented Apr 20, 2023

Why

Task of #489

  • I removed the feature flags because AFAIK the plan is to run all tests by default. Please let me know if that's not right.
  • Client/Server are using a user-provided stream, I added some code to have them return the stream so that users can clean up the stream as needed by the protocol in which UFDP in running on. Open to suggestions of course.

Checklist

  • I have made corresponding changes to the tests
  • I have made corresponding changes to the documentation
  • I have run the app using my feature and ensured that no functionality is broken

@kckeiks kckeiks changed the base branch from main to feat/pod April 20, 2023 01:32
@kckeiks kckeiks marked this pull request as ready for review April 20, 2023 20:00
@kckeiks kckeiks requested review from ozwaldorf and qti3e April 20, 2023 20:01
let mut endpoint = Endpoint::client("0.0.0.0:0".parse().unwrap()).unwrap();
let client_config = quinn::ClientConfig::new(Arc::new(client_config()));
endpoint.set_default_client_config(client_config);
let stream = endpoint
Copy link
Collaborator Author

@kckeiks kckeiks Apr 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@qti3e @ozwaldorf what's nice about using QUIC here is that we don't have to open iterations connections like in TCP (if we were using TCP + TLS that would be a lot of connection setup overhead). Instead we can open one connection and send each request in a separate QUIC stream. They all get multiplexed in the wire.

Copy link
Member

@ozwaldorf ozwaldorf Apr 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really great point, and good to consider when we choose the main transport! I can see this being really useful if a gateway is using many concurrent ufdp connections to a node


#[cfg(feature = "bench-quic")]
{
let mut g = c.benchmark_group(format!("QUINN UFDP/{range}"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit so both impl can be run with QUIC

Suggested change
let mut g = c.benchmark_group(format!("QUINN UFDP/{range}"));
let mut g = c.benchmark_group(format!("QUINN QUIC UFDP/{range}"));

mod tls_utils {
use std::sync::Arc;

pub struct SkipServerVerification;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot, I'm going to remove this because it would be better to not skip the certificate verification so we can get a better idea of the performance.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the iterations in the bench are multiplexed over a single quic connection, this would only affect the initial connection right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's right.

content: &'static [u8],
tx_started: tokio::sync::oneshot::Sender<u16>,
) {
let server_config = ServerConfig::with_crypto(Arc::new(server_config()));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Make TLS configurable.

@ozwaldorf
Copy link
Member

ozwaldorf commented Apr 21, 2023

Just for visibility, s2n-quic is having an issue on ubuntu 22.04 (it's working on mac only atm):

Benchmarking S2N-QUIC UFDP/Content Size (Kilobyte)/1 request/1: Collecting 20 samples in estimated 11.116 s (8190 iterations)
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: MaxHandshakeDurationExceeded { max_handshake_duration: 10s, source: Location { file: "/home/oz/.cargo/registry/src/index.crates.io-6f17d22bba15001f/s2n-quic-transport-0.18.2/src/connection/connection_imp
l.rs", line: 1027, col: 24 } }', crates/ursa-pod/benches/e2e.rs:510:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

error: bench failed, to rerun pass `-p ursa-pod --bench e2e`

@kckeiks
Copy link
Collaborator Author

kckeiks commented Apr 21, 2023

Closing for #519

@kckeiks kckeiks closed this Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants