Merge branch 'main' into smallvec-tokens

.github/actions/rust/action.yml

@@ -30,7 +30,7 @@ runs:
 
     - name: Install Rust tools
       shell: bash
-      run: cargo +${{ inputs.version }} binstall --no-confirm cargo-llvm-cov cargo-nextest flamegraph cargo-hack cargo-mutants
+      run: cargo +${{ inputs.version }} binstall --no-confirm cargo-llvm-cov cargo-nextest flamegraph cargo-hack cargo-mutants hyperfine
 
     # sccache slows CI down, so we leave it disabled.
     # Leaving the steps below commented out, so we can re-evaluate enabling it later.

.github/workflows/bench.yml

@@ -2,14 +2,17 @@ name: Bench
 on:
   workflow_call:
   workflow_dispatch:
+  schedule:
+    # Run at 1 AM each day, so there is a `main`-branch baseline in the cache.
+    - cron: '0 1 * * *'
 env:
   CARGO_PROFILE_BENCH_BUILD_OVERRIDE_DEBUG: true
   CARGO_PROFILE_RELEASE_DEBUG: true
   CARGO_TERM_COLOR: always
   RUST_BACKTRACE: 1
   TOOLCHAIN: nightly
   RUSTFLAGS: -C link-arg=-fuse-ld=lld -C link-arg=-Wl,--no-rosegment, -C force-frame-pointers=yes
-  PERF_CMD: record -o perf.data -F997 --call-graph fp -g
+  PERF_OPT: record -F997 --call-graph fp -g
 
 jobs:
   bench:
@@ -22,9 +25,17 @@ jobs:
       LD_LIBRARY_PATH: ${{ github.workspace }}/dist/Release/lib
 
     steps:
-      - name: Checkout
+      - name: Checkout neqo
         uses: actions/checkout@v4
 
+      - name: Checkout msquic
+        uses: actions/checkout@v4
+        with:
+          repository: microsoft/msquic
+          ref: main
+          path: msquic
+          submodules: true
+
       - name: Set PATH
         run: echo "/home/bench/.cargo/bin" >> "${GITHUB_PATH}"
 
@@ -37,10 +48,17 @@ jobs:
       - name: Fetch and build NSS and NSPR
         uses: ./.github/actions/nss
 
-      - name: Build
+      - name: Build neqo
         run: |
           cargo "+$TOOLCHAIN" bench --features bench --no-run
-          cargo "+$TOOLCHAIN" build --release --bin neqo-client --bin neqo-server
+          cargo "+$TOOLCHAIN" build --release
+
+      - name: Build msquic
+        run: |
+          mkdir -p msquic/build
+          cd msquic/build
+          cmake -GNinja -DCMAKE_BUILD_TYPE=RelWithDebInfo -DQUIC_BUILD_TOOLS=1 -DQUIC_BUILD_PERF=1 ..
+          cmake --build .
 
       - name: Download cached main-branch results
         id: criterion-cache
@@ -60,64 +78,115 @@ jobs:
           taskset -c 0 nice -n -20 \
             cargo "+$TOOLCHAIN" bench --features bench -- --noplot | tee results.txt
 
-      # Pin the transfer benchmark to core 0 and run it at elevated priority inside perf.
-      # Work around https://github.com/flamegraph-rs/flamegraph/issues/248 by passing explicit perf arguments.
-      - name: Profile cargo bench transfer
-        run: |
-          taskset -c 0 nice -n -20 \
-            cargo "+$TOOLCHAIN" flamegraph -v -c "$PERF_CMD" --features bench --bench transfer -- \
-              --bench --exact "Run multiple transfers with varying seeds" --noplot
-
-      - name: Profile client/server transfer
-        run: |
-          { mkdir server; \
-            cd server; \
-            taskset -c 0 nice -n -20 \
-            cargo "+$TOOLCHAIN" flamegraph -v -c "$PERF_CMD" \
-              --bin neqo-server -- --db ../test-fixture/db "$HOST:4433" || true; } &
-          mkdir client; \
-            cd client; \
-            time taskset -c 1 nice -n -20 \
-            cargo "+$TOOLCHAIN" flamegraph -v -c "$PERF_CMD" \
-              --bin neqo-client -- --output-dir . "https://$HOST:4433/$SIZE"
-          killall -INT neqo-server
-          cd ${{ github.workspace }}
-          [ "$(wc -c < client/"$SIZE")" -eq "$SIZE" ] || exit 1
+      # Compare various configurations of neqo against msquic, and gather perf data
+      # during the hyperfine runs.
+      - name: Compare neqo and msquic
         env:
-          HOST: localhost
-          SIZE: 1073741824 # 1 GB
+          HOST: 127.0.0.1
+          PORT: 4433
+          SIZE: 134217728 # 128 MB
+        run: |
+          TMP=$(mktemp -d)
+          # Make a cert and key for msquic.
+          openssl req -nodes -new -x509 -keyout "$TMP/key" -out "$TMP/cert" -subj "/CN=DOMAIN" 2>/dev/null
+          # Make a test file for msquic to serve.
+          truncate -s "$SIZE" "$TMP/$SIZE"
+          # Define the commands to run for each client and server.
+          declare -A client_cmd=(
+            ["neqo"]="target/release/neqo-client _cc _pacing --output-dir . -o -a hq-interop -Q 1 https://$HOST:$PORT/$SIZE"
+            ["msquic"]="msquic/build/bin/Release/quicinterop -test:D -custom:$HOST -port:$PORT -urls:https://$HOST:$PORT/$SIZE"
+          )
+          declare -A server_cmd=(
+            ["neqo"]="target/release/neqo-server _cc _pacing -o -a hq-interop -Q 1 $HOST:$PORT 2> /dev/null"
+            ["msquic"]="msquic/build/bin/Release/quicinteropserver -root:$TMP -listen:$HOST -port:$PORT -file:$TMP/cert -key:$TMP/key -noexit > /dev/null || true"
+          )
+
+          # Replace various placeholders in the commands with the actual values.
+          # Also generate an extension to append to the file name.
+          function transmogrify {
+            CMD=$1
+            local cc=$2
+            local pacing=$3
+            if [ "$cc" != "" ]; then
+              CMD=${CMD//_cc/--cc $cc}
+              EXT="-$cc"
+            fi
+            if [ "$pacing" == "on" ]; then
+              CMD=${CMD//_pacing/}
+              EXT="$EXT-pacing"
+            else
+              CMD=${CMD//_pacing/--no-pacing}
+              EXT="$EXT-nopacing"
+            fi
+          }
+
+          for server in msquic neqo; do
+            for client in msquic neqo; do
+              # msquic doesn't let us configure the congestion control or pacing.
+              if [ "$client" == "msquic" ] && [ "$server" == "msquic" ]; then
+                cc_opt=("")
+                pacing_opt=("")
+              else
+                cc_opt=("reno" "cubic")
+                pacing_opt=("on" "")
+              fi
+              for cc in "${cc_opt[@]}"; do
+                for pacing in "${pacing_opt[@]}"; do
+                  # Make a tag string for this test, for the results.
+                  TAG="$client,$server,$cc,$pacing"
+                  echo "Running benchmarks for $TAG" | tee -a comparison.txt
+                  transmogrify "${server_cmd[$server]}" "$cc" "$pacing"
+                  # shellcheck disable=SC2086
+                  taskset -c 0 nice -n -20 \
+                    perf $PERF_OPT -o "$client-$server$EXT.server.perf" $CMD &
+                  PID=$!
+                  transmogrify "${client_cmd[$client]}" "$cc" "$pacing"
+                  # shellcheck disable=SC2086
+                  taskset -c 1 nice -n -20 \
+                    perf $PERF_OPT -o "$client-$server$EXT.client.perf" \
+                      hyperfine -N --output null -w 1 -s "sleep 1" -n "$TAG" -u millisecond --export-markdown step.md "$CMD" |
+                      tee -a comparison.txt
+                  echo >> comparison.txt
+                  kill $PID
+                  cat step.md >> steps.md
+                  # Sanity check the size of the last retrieved file.
+                  [ "$(wc -c <"$SIZE")" -eq "$SIZE" ] || exit 1
+                done
+              done
+            done
+          done
+          # Merge the results tables generated by hyperfine into a single table.
+          echo "Transfer of $SIZE bytes over loopback." > comparison.md
+          awk '(!/^\| Command/ || !c++) && (!/^\|:/ || !d++)' < steps.md |\
+            sed -E 's/`//g; s/^\|:/\|:---\|:---\|:---\|:/g; s/,/ \| /g; s/^\| Command/\| Client \| Server \| CC \| Pacing/g' >> comparison.md
+          rm -r "$TMP"
 
       # Re-enable turboboost, hyperthreading and use powersave governor.
       - name: Restore machine
         run: sudo /root/bin/unprep.sh
         if: success() || failure() || cancelled()
 
-      - name: Convert for profiler.firefox.com
+      - name: Post-process perf data
         run: |
-          perf script -i perf.data -F +pid > transfer.perf &
-          perf script -i client/perf.data -F +pid > client.perf &
-          perf script -i server/perf.data -F +pid > server.perf &
+          for f in *.perf; do
+            # Convert for profiler.firefox.com
+            perf script -i "$f" -F +pid > "$f.fx" &
+            # Generate perf reports
+            perf report -i "$f" --no-children --stdio > "$f.txt" &
+            # Generate flamegraphs
+            flamegraph --perfdata "$f" --palette rust -o "${f//.perf/.svg}" &
+          done
           wait
-          mv flamegraph.svg transfer.svg
-          mv client/flamegraph.svg client.svg
-          mv server/flamegraph.svg server.svg
           rm neqo.svg
 
-      - name: Generate perf reports
-        run: |
-          perf report -i perf.data --no-children --stdio > transfer.perf.txt &
-          perf report -i client/perf.data --no-children --stdio > client.perf.txt &
-          perf report -i server/perf.data --no-children --stdio > server.perf.txt &
-          wait
-
       - name: Format results as Markdown
         id: results
         run: |
           {
             echo "### Benchmark results"
             echo
           } > results.md
-          SHA=$(cat target/criterion/baseline-sha.txt)
+          SHA=$(cat target/criterion/baseline-sha.txt || true)
           if [ -n "$SHA" ]; then
             {
               echo "Performance differences relative to $SHA."
@@ -131,6 +200,11 @@ jobs:
                     -e 's/^([a-z0-9].*)$/* **\1**/gi' \
                     -e 's/(change:[^%]*% )([^%]*%)(.*)/\1**\2**\3/gi' \
               >> results.md
+          {
+            echo "### Client/server transfer results"
+            cat comparison.md
+          } >> results.md
+          cat results.md > "$GITHUB_STEP_SUMMARY"
 
       - name: Remember main-branch push URL
         if: github.ref == 'refs/heads/main'
@@ -157,6 +231,7 @@ jobs:
           path: |
             *.svg
             *.perf
+            *.perf.fx
             *.txt
             results.*
             target/criterion*

.github/workflows/check.yml

@@ -113,7 +113,7 @@ jobs:
       - name: Run tests and determine coverage
         run: |
           # shellcheck disable=SC2086
-          cargo +${{ matrix.rust-toolchain }} llvm-cov nextest $BUILD_TYPE --all-targets --features ci --no-fail-fast --lcov --output-path lcov.info
+          cargo +${{ matrix.rust-toolchain }} llvm-cov nextest $BUILD_TYPE --features ci --no-fail-fast --lcov --output-path lcov.info
           cargo +${{ matrix.rust-toolchain }} bench --features bench --no-run
 
       - name: Run client/server transfer
@@ -122,6 +122,8 @@ jobs:
           cargo +${{ matrix.rust-toolchain }} build $BUILD_TYPE --bin neqo-client --bin neqo-server
           "target/$BUILD_DIR/neqo-server" "$HOST:4433" &
           PID=$!
+          # Give the server time to start.
+          sleep 1
           "target/$BUILD_DIR/neqo-client" --output-dir . "https://$HOST:4433/$SIZE"
           kill $PID
           [ "$(wc -c <"$SIZE")" -eq "$SIZE" ] || exit 1
@@ -146,7 +148,7 @@ jobs:
           # respective default features only. Can reveal warnings otherwise
           # hidden given that a plain cargo clippy combines all features of the
           # workspace. See e.g. https://github.com/mozilla/neqo/pull/1695.
-          cargo +${{ matrix.rust-toolchain }} hack clippy --all-targets -- -D warnings || ${{ matrix.rust-toolchain == 'nightly' }}
+          cargo +${{ matrix.rust-toolchain }} hack clippy --all-targets --feature-powerset --exclude-features gecko -- -D warnings || ${{ matrix.rust-toolchain == 'nightly' }}
         if: success() || failure()
 
       - name: Check rustdoc links

neqo-bin/Cargo.toml

@@ -25,6 +25,7 @@ workspace = true
 [dependencies]
 # neqo-bin is not used in Firefox, so we can be liberal with dependency versions
 clap = { version = "4.4", default-features = false, features = ["std", "color", "help", "usage", "error-context", "suggestions", "derive"] }
+clap-verbosity-flag = { version = "2.2", default-features = false }
 futures = { version = "0.3", default-features = false, features = ["alloc"] }
 hex = { version = "0.4", default-features = false, features = ["std"] }
 log = { version = "0.4", default-features = false }

neqo-bin/src/bin/client/http09.rs

@@ -17,7 +17,7 @@ use std::{
     time::Instant,
 };
 
-use neqo_common::{event::Provider, Datagram};
+use neqo_common::{event::Provider, qdebug, qinfo, qwarn, Datagram};
 use neqo_crypto::{AuthenticationStatus, ResumptionToken};
 use neqo_transport::{
     Connection, ConnectionEvent, EmptyConnectionIdGenerator, Error, Output, State, StreamId,
@@ -50,21 +50,21 @@ impl<'a> super::Handler for Handler<'a> {
                     self.read(client, stream_id)?;
                 }
                 ConnectionEvent::SendStreamWritable { stream_id } => {
-                    println!("stream {stream_id} writable");
+                    qdebug!("stream {stream_id} writable");
                 }
                 ConnectionEvent::SendStreamComplete { stream_id } => {
-                    println!("stream {stream_id} complete");
+                    qdebug!("stream {stream_id} complete");
                 }
                 ConnectionEvent::SendStreamCreatable { stream_type } => {
-                    println!("stream {stream_type:?} creatable");
+                    qdebug!("stream {stream_type:?} creatable");
                     if stream_type == StreamType::BiDi {
                         self.download_urls(client);
                     }
                 }
                 ConnectionEvent::StateChange(
                     State::WaitInitial | State::Handshaking | State::Connected,
                 ) => {
-                    println!("{event:?}");
+                    qdebug!("{event:?}");
                     self.download_urls(client);
                 }
                 ConnectionEvent::StateChange(State::Confirmed) => {
@@ -74,7 +74,7 @@ impl<'a> super::Handler for Handler<'a> {
                     self.token = Some(token);
                 }
                 _ => {
-                    println!("Unhandled event {event:?}");
+                    qwarn!("Unhandled event {event:?}");
                 }
             }
         }
@@ -153,6 +153,10 @@ impl super::Client for Connection {
     fn is_closed(&self) -> bool {
         matches!(self.state(), State::Closed(..))
     }
+
+    fn stats(&self) -> neqo_transport::Stats {
+        self.stats()
+    }
 }
 
 impl<'b> Handler<'b> {
@@ -183,7 +187,7 @@ impl<'b> Handler<'b> {
 
     fn download_next(&mut self, client: &mut Connection) -> bool {
         if self.key_update.needed() {
-            println!("Deferring requests until after first key update");
+            qdebug!("Deferring requests until after first key update");
             return false;
         }
         let url = self
@@ -192,7 +196,7 @@ impl<'b> Handler<'b> {
             .expect("download_next called with empty queue");
         match client.stream_create(StreamType::BiDi) {
             Ok(client_stream_id) => {
-                println!("Created stream {client_stream_id} for {url}");
+                qinfo!("Created stream {client_stream_id} for {url}");
                 let req = format!("GET {}\r\n", url.path());
                 _ = client
                     .stream_send(client_stream_id, req.as_bytes())
@@ -203,7 +207,7 @@ impl<'b> Handler<'b> {
                 true
             }
             Err(e @ (Error::StreamLimitError | Error::ConnectionState)) => {
-                println!("Cannot create stream {e:?}");
+                qwarn!("Cannot create stream {e:?}");
                 self.url_queue.push_front(url);
                 false
             }
@@ -231,9 +235,9 @@ impl<'b> Handler<'b> {
             if let Some(out_file) = maybe_out_file {
                 out_file.write_all(&data[..sz])?;
             } else if !output_read_data {
-                println!("READ[{stream_id}]: {sz} bytes");
+                qdebug!("READ[{stream_id}]: {sz} bytes");
             } else {
-                println!(
+                qdebug!(
                     "READ[{}]: {}",
                     stream_id,
                     String::from_utf8(data.clone()).unwrap()
@@ -248,7 +252,7 @@ impl<'b> Handler<'b> {
     fn read(&mut self, client: &mut Connection, stream_id: StreamId) -> Res<()> {
         match self.streams.get_mut(&stream_id) {
             None => {
-                println!("Data on unexpected stream: {stream_id}");
+                qwarn!("Data on unexpected stream: {stream_id}");
                 return Ok(());
             }
             Some(maybe_out_file) => {
@@ -263,7 +267,7 @@ impl<'b> Handler<'b> {
                     if let Some(mut out_file) = maybe_out_file.take() {
                         out_file.flush()?;
                     } else {
-                        println!("<FIN[{stream_id}]>");
+                        qinfo!("<FIN[{stream_id}]>");
                     }
                     self.streams.remove(&stream_id);
                     self.download_urls(client);