From bb3ab602481c0ec61597ef76242a3008108d524b Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Sat, 23 Mar 2024 17:13:01 +1000 Subject: [PATCH 1/5] Don't fail if cached main-branch results don't exist --- .github/workflows/bench.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/bench.yml b/.github/workflows/bench.yml index 81ef297a9e..134f78f559 100644 --- a/.github/workflows/bench.yml +++ b/.github/workflows/bench.yml @@ -115,7 +115,7 @@ jobs: 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." From 9814bcd907039349fdf08fad39209a38515414d4 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sun, 24 Mar 2024 18:20:27 +0100 Subject: [PATCH 2/5] fix(transport/bench): increase transfer bench noise threshold (#1771) The default Criterion noise threshold is 0.01, i.e. ``` rust /// Changes the noise threshold for benchmarks in this group. The noise threshold /// is used to filter out small changes in performance from one run to the next, even if they /// are statistically significant. Sometimes benchmarking the same code twice will result in /// small but statistically significant differences solely because of noise. This provides a way /// to filter out some of these false positives at the cost of making it harder to detect small /// changes to the true performance of the benchmark. /// /// The default is 0.01, meaning that changes smaller than 1% will be ignored. /// /// # Panics /// /// Panics if the threshold is set to a negative value pub fn noise_threshold(&mut self, threshold: f64) -> &mut Self { ``` https://bheisler.github.io/criterion.rs/criterion/struct.BenchmarkGroup.html#method.noise_threshold Multiple runs of the `neqo-transport` `transfer/*` benchmarks showed between 2% and 3% performance regression on unrelated changes. - https://github.com/mozilla/neqo/actions/runs/8402727182/job/23012699901 - https://github.com/mozilla/neqo/actions/runs/8408164062/job/23024217712 To improve the signal-to-noise ratio of the benchmarks, set the noise threshold to 3%. --- neqo-transport/benches/transfer.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/neqo-transport/benches/transfer.rs b/neqo-transport/benches/transfer.rs index b13075a4ff..98bd29ff05 100644 --- a/neqo-transport/benches/transfer.rs +++ b/neqo-transport/benches/transfer.rs @@ -23,6 +23,7 @@ const TRANSFER_AMOUNT: usize = 1 << 22; // 4Mbyte fn benchmark_transfer(c: &mut Criterion, label: &str, seed: Option>) { let mut group = c.benchmark_group("transfer"); group.throughput(Throughput::Bytes(u64::try_from(TRANSFER_AMOUNT).unwrap())); + group.noise_threshold(0.03); group.bench_function(label, |b| { b.iter_batched( || { From 6691c1a570a37a388dc2495b0e592c572b4129b6 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Mon, 25 Mar 2024 02:38:50 +0200 Subject: [PATCH 3/5] ci: Run a daily baseline benchmark of `main` for the cache (#1770) --- .github/workflows/bench.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/bench.yml b/.github/workflows/bench.yml index 134f78f559..0e215f571f 100644 --- a/.github/workflows/bench.yml +++ b/.github/workflows/bench.yml @@ -2,6 +2,9 @@ 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 From d8eeddaad505710bb60361a4da66bf2743e64e1f Mon Sep 17 00:00:00 2001 From: Martin Thomson Date: Mon, 25 Mar 2024 17:16:18 +1100 Subject: [PATCH 4/5] Maybe make the timer wheel faster (#1763) * Maybe make the timer wheel faster * Add some panics docs * Work around mozpkix issue * Add a simple benchmark for the timer wheel * Remove workaround * Make clippy happy --------- Co-authored-by: Lars Eggert --- neqo-common/Cargo.toml | 5 ++++ neqo-common/benches/timer.rs | 39 ++++++++++++++++++++++++++++++ neqo-common/src/timer.rs | 46 +++++++++++++++++++++++++++--------- 3 files changed, 79 insertions(+), 11 deletions(-) create mode 100644 neqo-common/benches/timer.rs diff --git a/neqo-common/Cargo.toml b/neqo-common/Cargo.toml index 89eaa53890..069d67b834 100644 --- a/neqo-common/Cargo.toml +++ b/neqo-common/Cargo.toml @@ -21,6 +21,7 @@ qlog = { version = "0.12", default-features = false } time = { version = "0.3", default-features = false, features = ["formatting"] } [dev-dependencies] +criterion = { version = "0.5", default-features = false, features = ["html_reports"] } test-fixture = { path = "../test-fixture" } [features] @@ -33,3 +34,7 @@ features = ["timeapi"] [lib] # See https://github.com/bheisler/criterion.rs/blob/master/book/src/faq.md#cargo-bench-gives-unrecognized-option-errors-for-valid-command-line-options bench = false + +[[bench]] +name = "timer" +harness = false diff --git a/neqo-common/benches/timer.rs b/neqo-common/benches/timer.rs new file mode 100644 index 0000000000..5ac8019db4 --- /dev/null +++ b/neqo-common/benches/timer.rs @@ -0,0 +1,39 @@ +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use std::time::{Duration, Instant}; + +use criterion::{criterion_group, criterion_main, Criterion}; +use neqo_common::timer::Timer; +use test_fixture::now; + +fn benchmark_timer(c: &mut Criterion) { + c.bench_function("drain a timer quickly", |b| { + b.iter_batched_ref( + make_timer, + |(_now, timer)| { + while let Some(t) = timer.next_time() { + assert!(timer.take_next(t).is_some()); + } + }, + criterion::BatchSize::SmallInput, + ); + }); +} + +fn make_timer() -> (Instant, Timer<()>) { + const TIMES: &[u64] = &[1, 2, 3, 5, 8, 13, 21, 34]; + + let now = now(); + let mut timer = Timer::new(now, Duration::from_millis(777), 100); + for &t in TIMES { + timer.add(now + Duration::from_secs(t), ()); + } + (now, timer) +} + +criterion_group!(benches, benchmark_timer); +criterion_main!(benches); diff --git a/neqo-common/src/timer.rs b/neqo-common/src/timer.rs index a413252e08..3feddb2226 100644 --- a/neqo-common/src/timer.rs +++ b/neqo-common/src/timer.rs @@ -5,6 +5,7 @@ // except according to those terms. use std::{ + collections::VecDeque, mem, time::{Duration, Instant}, }; @@ -27,7 +28,7 @@ impl TimerItem { /// points). Time is relative, the wheel has an origin time and it is unable to represent times that /// are more than `granularity * capacity` past that time. pub struct Timer { - items: Vec>>, + items: Vec>>, now: Instant, granularity: Duration, cursor: usize, @@ -55,9 +56,14 @@ impl Timer { /// Return a reference to the time of the next entry. #[must_use] pub fn next_time(&self) -> Option { - for i in 0..self.items.len() { - let idx = self.bucket(i); - if let Some(t) = self.items[idx].first() { + let idx = self.bucket(0); + for i in idx..self.items.len() { + if let Some(t) = self.items[i].front() { + return Some(t.time); + } + } + for i in 0..idx { + if let Some(t) = self.items[i].front() { return Some(t.time); } } @@ -145,6 +151,9 @@ impl Timer { /// Given knowledge of the time an item was added, remove it. /// This requires use of a predicate that identifies matching items. + /// + /// # Panics + /// Impossible, I think. pub fn remove(&mut self, time: Instant, mut selector: F) -> Option where F: FnMut(&T) -> bool, @@ -167,7 +176,7 @@ impl Timer { break; } if selector(&self.items[bucket][i].item) { - return Some(self.items[bucket].remove(i).item); + return Some(self.items[bucket].remove(i).unwrap().item); } } // ... then forwards. @@ -176,7 +185,7 @@ impl Timer { break; } if selector(&self.items[bucket][i].item) { - return Some(self.items[bucket].remove(i).item); + return Some(self.items[bucket].remove(i).unwrap().item); } } None @@ -185,10 +194,25 @@ impl Timer { /// Take the next item, unless there are no items with /// a timeout in the past relative to `until`. pub fn take_next(&mut self, until: Instant) -> Option { - for i in 0..self.items.len() { - let idx = self.bucket(i); - if !self.items[idx].is_empty() && self.items[idx][0].time <= until { - return Some(self.items[idx].remove(0).item); + fn maybe_take(v: &mut VecDeque>, until: Instant) -> Option { + if !v.is_empty() && v[0].time <= until { + Some(v.pop_front().unwrap().item) + } else { + None + } + } + + let idx = self.bucket(0); + for i in idx..self.items.len() { + let res = maybe_take(&mut self.items[i], until); + if res.is_some() { + return res; + } + } + for i in 0..idx { + let res = maybe_take(&mut self.items[i], until); + if res.is_some() { + return res; } } None @@ -201,7 +225,7 @@ impl Timer { if until >= self.now + self.span() { // Drain everything, so a clean sweep. let mut empty_items = Vec::with_capacity(self.items.len()); - empty_items.resize_with(self.items.len(), Vec::default); + empty_items.resize_with(self.items.len(), VecDeque::default); let mut items = mem::replace(&mut self.items, empty_items); self.now = until; self.cursor = 0; From 50876af998b97b4a249be814b32f675704f9714a Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Mon, 25 Mar 2024 09:50:45 +0200 Subject: [PATCH 5/5] ci: Run `clippy` for all features except `gecko` (#1768) * ci: Run `clippy` for all features except `gecko` * Make clippy happy * Update .github/workflows/check.yml Co-authored-by: Max Inden Signed-off-by: Lars Eggert --------- Signed-off-by: Lars Eggert Co-authored-by: Max Inden --- .github/workflows/check.yml | 2 +- neqo-crypto/src/aead_fuzzing.rs | 3 +++ neqo-crypto/src/lib.rs | 2 +- neqo-transport/benches/range_tracker.rs | 16 +++++++++------- neqo-transport/benches/rx_stream_orderer.rs | 4 ++-- neqo-transport/benches/transfer.rs | 8 ++++---- neqo-transport/src/connection/tests/fuzzing.rs | 2 +- neqo-transport/src/connection/tests/handshake.rs | 5 +++-- 8 files changed, 24 insertions(+), 18 deletions(-) diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index e17d563905..9dc8ff2b7f 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -148,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 diff --git a/neqo-crypto/src/aead_fuzzing.rs b/neqo-crypto/src/aead_fuzzing.rs index 4e5a6de07f..1f3bfb14bd 100644 --- a/neqo-crypto/src/aead_fuzzing.rs +++ b/neqo-crypto/src/aead_fuzzing.rs @@ -20,6 +20,7 @@ pub struct FuzzingAead { } impl FuzzingAead { + #[allow(clippy::missing_errors_doc)] pub fn new( fuzzing: bool, version: Version, @@ -44,6 +45,7 @@ impl FuzzingAead { } } + #[allow(clippy::missing_errors_doc)] pub fn encrypt<'a>( &self, count: u64, @@ -61,6 +63,7 @@ impl FuzzingAead { Ok(&output[..l + 16]) } + #[allow(clippy::missing_errors_doc)] pub fn decrypt<'a>( &self, count: u64, diff --git a/neqo-crypto/src/lib.rs b/neqo-crypto/src/lib.rs index 2ec1b4a3ea..45f61f6127 100644 --- a/neqo-crypto/src/lib.rs +++ b/neqo-crypto/src/lib.rs @@ -9,7 +9,7 @@ mod aead; #[cfg(feature = "fuzzing")] -mod aead_fuzzing; +pub mod aead_fuzzing; pub mod agent; mod agentio; mod auth; diff --git a/neqo-transport/benches/range_tracker.rs b/neqo-transport/benches/range_tracker.rs index c2f78f4874..ee611cf4ea 100644 --- a/neqo-transport/benches/range_tracker.rs +++ b/neqo-transport/benches/range_tracker.rs @@ -11,30 +11,32 @@ const CHUNK: u64 = 1000; const END: u64 = 100_000; fn build_coalesce(len: u64) -> RangeTracker { let mut used = RangeTracker::default(); - used.mark_acked(0, CHUNK as usize); - used.mark_sent(CHUNK, END as usize); + let chunk = usize::try_from(CHUNK).expect("should fit"); + used.mark_acked(0, chunk); + used.mark_sent(CHUNK, usize::try_from(END).expect("should fit")); // leave a gap or it will coalesce here for i in 2..=len { // These do not get immediately coalesced when marking since they're not at the end or start - used.mark_acked(i * CHUNK, CHUNK as usize); + used.mark_acked(i * CHUNK, chunk); } used } fn coalesce(c: &mut Criterion, count: u64) { + let chunk = usize::try_from(CHUNK).expect("should fit"); c.bench_function( &format!("coalesce_acked_from_zero {count}+1 entries"), |b| { b.iter_batched_ref( || build_coalesce(count), |used| { - used.mark_acked(CHUNK, CHUNK as usize); + used.mark_acked(CHUNK, chunk); let tail = (count + 1) * CHUNK; - used.mark_sent(tail, CHUNK as usize); - used.mark_acked(tail, CHUNK as usize); + used.mark_sent(tail, chunk); + used.mark_acked(tail, chunk); }, criterion::BatchSize::SmallInput, - ) + ); }, ); } diff --git a/neqo-transport/benches/rx_stream_orderer.rs b/neqo-transport/benches/rx_stream_orderer.rs index 0a1e763e97..d58e11ee86 100644 --- a/neqo-transport/benches/rx_stream_orderer.rs +++ b/neqo-transport/benches/rx_stream_orderer.rs @@ -11,14 +11,14 @@ fn rx_stream_orderer() { let mut rx = RxStreamOrderer::new(); let data: &[u8] = &[0; 1337]; - for i in 0..100000 { + for i in 0..100_000 { rx.inbound_frame(i * 1337, data); } } fn criterion_benchmark(c: &mut Criterion) { c.bench_function("RxStreamOrderer::inbound_frame()", |b| { - b.iter(rx_stream_orderer) + b.iter(rx_stream_orderer); }); } diff --git a/neqo-transport/benches/transfer.rs b/neqo-transport/benches/transfer.rs index 98bd29ff05..32959f6cb5 100644 --- a/neqo-transport/benches/transfer.rs +++ b/neqo-transport/benches/transfer.rs @@ -20,7 +20,7 @@ const ZERO: Duration = Duration::from_millis(0); const JITTER: Duration = Duration::from_millis(10); const TRANSFER_AMOUNT: usize = 1 << 22; // 4Mbyte -fn benchmark_transfer(c: &mut Criterion, label: &str, seed: Option>) { +fn benchmark_transfer(c: &mut Criterion, label: &str, seed: &Option>) { let mut group = c.benchmark_group("transfer"); group.throughput(Throughput::Bytes(u64::try_from(TRANSFER_AMOUNT).unwrap())); group.noise_threshold(0.03); @@ -45,7 +45,7 @@ fn benchmark_transfer(c: &mut Criterion, label: &str, seed: Option