Repository navigation
Add qperf, a QUIC-based performance tool and regression tests - #3826
robgjansen wants to merge 6 commits into
Conversation
71e2aac to
83a35c6
Compare
|
@sporksmith I could use some feedback on how we ought to organize the qperf tests here and wrangle the CI so that we only need to build qperf and run its tests in a single CI instance. (I was trying to avoid setting it up as a 'standard' extra test, because that gets built everywhere, but otherwise the CI scripts don't make this easy...) |
|
OK, so it was much easier to ignore the sscache build and just rebuild shadow+qperf in the new CI instance. That seems to work OK and the job finishes in roughly the same time as the others. 🚀 |
The new package is only built when extra tests are built. It uses the QUIC rust library from Cloudflare called quiche to establish a QUIC connection and transfer a configurable number of bytes in both directions between a client and server while logging QUIC metrics for later analysis. The new package uses smol, a lightweight async runtime, to simplify the handling of concurrent network send_to/recv_from and timeout events. Smol pulls in quite a few dependencies for the async runtime components, which increases build time. Thus, building qperf and running qperf tests is only done in the extra_test configuration.
This enables us to define criteria for test cases to pass or fail for specific network setups, according to the bandwidth, latency, and packet loss of the test and according to tolerances that can be configured as cli args. We should be able to run this following the qperf tests similar to how we do with the tgen perf tests.
We set wide tolerances for bandwidth and rtt metrics for now, with the goal of narrowing them over time as we improve shadow.
sporksmith
left a comment
sporksmith
left a comment
There was a problem hiding this comment.
A few suggestions, but approving since I don't think anything here is a hard blocker.
Disclaimer - best practices and pitfalls in async rust aren't super fresh in my mind; @opara-tor might have a better eye there. OTOH idk that this code particularly needs careful scrutiny.
| /// else in the local directory if `QLOGDIR` is not set. | ||
| pub fn enable_quic_qlog(conn: &mut Connection, prefix: &str) -> std::io::Result<()> { | ||
| // Use the local dir if the env var is not set. | ||
| let qlog_dir = std::env::var("QLOGDIR").unwrap_or(String::from("./")); |
There was a problem hiding this comment.
optional bike-shedding: Any particular reason to do this by envvar instead of by command-line arg? I tend to prefer command-line args for being more discoverable and less error-prone, though admittedly envvars can provide some useful flexibility.
| /// # Typical workflow | ||
| /// 1. Call [`Self::connect`] (client) or [`Self::listen`] (server) to | ||
| /// obtain an instance. | ||
| /// 2. Invoke [`Self::into_transfer`] with the desired payload length; the |
There was a problem hiding this comment.
From the client? server? both?
| /// method drives the event loop until the stream and connection have both | ||
| /// closed. | ||
| pub struct ConnectionManager { | ||
| /// The underlying [`smol::net::UdpSocket`] used for all network I/O. |
There was a problem hiding this comment.
nit, here and throughout - it's nice that the doc-comment gives the fully qualified type here; maybe it's worth doing it in the actual declaration too. Fully qualifying is particularly useful since we're dealing with similar sounding types from multiple crates; it's a little hard to keep track of what's from what.
There was a problem hiding this comment.
(And maybe drop the references to the types in the doc-comments, since fully qualifying in the decl would make them completely redundant with the decls themselves)
| conn: Connection, | ||
| /// Static, fixed-size buffer to hold inbound UDP packets until we transfer | ||
| /// the data to QUIC. | ||
| recv_buf: Vec<u8>, |
There was a problem hiding this comment.
Optional - from the description it sounds like this could be the slightly more primitive type Box<[u8]>. Or if the size will always be known at compile time, maybe Box<[u8; BUF_CAPACITY]>
| recv_buf: Vec<u8>, | ||
| /// Static, fixed-size buffer to hold outbound UDP packets from QUIC until | ||
| /// we transfer them to the kernel/network. | ||
| send_buf: Vec<u8>, |
| /// **and** the QUIC connection has entered the closed state | ||
| /// ([`quiche::Connection::is_closed`]). Any I/O or QUIC error is | ||
| /// propagated. | ||
| pub async fn into_transfer(&mut self, send_len: usize) -> anyhow::Result<()> { |
There was a problem hiding this comment.
Then into_ in the name is a little confusing. I'd expect it to take self or mut self rather than a reference &mut self, thus transforming self into something (the transfer result).
Maybe process_transfer ...?
Or if self is in some useless state after this finishes, maybe it should take mut self? (Or maybe that doesn't place nice with async...?)
| [[bin]] | ||
| name = "qperf" | ||
| path = "src/main.rs" | ||
| required-features = ["extra_tests"] |
There was a problem hiding this comment.
I'm a little confused - since this is its own crate, and the only build output is qperf, I think it doesn't make much sense to put everything behind a feature. Instead at some higher level we can omit the qperf crate when extra-tests isn't selected.
Maybe it'd also make more sense to put this in a sibling directory to test; it's a little confusing to have the source for some crates (qperf) in a subdir of another. So e.g. src/extra-tests/qperf.
Alternatively, using the features roughly as you are now, I think we could not make qperf its own crate, and instead make it part of the test crate, enabled only when the extra-tests feature is enabled.
If we do that, it might make sense to add an extra level of indirection in the features - have a qperf feature that enables building the qperf bin and pulls in the deps it needs, and an extra-tests feature that enables qperf and any others we later add.
| --workspace \ | ||
| --exclude shadow-tests \ | ||
| --exclude shadow-shim \ | ||
| --exclude qperf \ |
There was a problem hiding this comment.
It's a little unfortunate to have to call out individual extra tests here. I think this goes away if we make qperf part of the tests crate.
Alternatively I think we could change this invocation to just build the shadow-rs crate, which will pull in the other crates it needs, instead of saying "build everything except ..."
| @@ -0,0 +1,91 @@ | |||
| # ------------------------------------------------------------------------ | |||
There was a problem hiding this comment.
Ok, but an alternative if we didn't want to add more fancy cmake would be a python script that takes the parameters as command-line args and generates the yaml. (or generates json, which is valid yaml 1.2)
| - name: Build | ||
| run: | | ||
| cd shadow | ||
| ./setup build -j4 --test --perf --werror |
There was a problem hiding this comment.
Is there a reason we rebuild shadow here instead of reusing the same shadow that other extra-tests use? See "download shadow" step in the tgen test, for example
This adds the implementation of a basic two node (client/server) transfer tool that uses QUIC (the Cloudflare quiche rust crate) to transfer a configurable number of bytes and log various performance metrics along the way. I also added a rust utility to parse the quiche sqlog output and apply a series of performance acceptance tests to each transfer result.
This enables us to build performance tests from a standard QUIC implementation. Specifically, it helps us test congestion behaviors, buffer bloat, etc., of Shadow's UDP layer (i.e., isolating it from our TCP implementation). (Quiche supports multiple congestion protocols, which we could explore testing in the future or comparing to future TCP counterpart implementations if we eventually add those.)
Because building and running qperf takes quite a bit of extra cycles, this is all set up to build and run only in the
--extraconfiguration (also mirrors our tgen perf tests).The initial set of performance acceptance tests are set up with wide tolerances, which I hope we'll be able to narrow later as we improve Shadow over time.