From 93e6f1cab9232e28bfae583eca395c344f5b92cd Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 21 Aug 2026 21:14:58 -0600 Subject: [PATCH 01/13] test(routing): Measure the fib lookup, and settle the double walk `Fib::lpm_entry_prefix` walks a route's fib groups twice -- once to count entries, once to turn the hashed index back into a group and an offset -- which looks like a per-packet cost worth removing. It is not. Caching the count regresses the common case. One group's two walks touch the same cache lines, so the second is nearly free, while the extra field grows `FibRoute` from 24 to 32 bytes and costs more in the trie than it saves: 1g x1e goes from 17.1ns to 17.5ns. It only pays from about eight groups up, reaching -16% at 16g x1e and -22% at 16g x4e. Whether that trade is worth making is a question about how wide real ECMP gets, and the answer is not in the code. The proportions are the useful part: of 17.1ns, the trie lookup is 10.0 and pulling the destination out of the packet is 2.3, so entry selection is under a third of a lookup that is itself mostly trie. The assertion inside the loop is load-bearing rather than decorative. The test packet is addressed to 5.6.7.8, so a route installed on 10.0.0.0/8 sends every shape to the default route and returns the same number -- a bench that measures nothing and says so nowhere. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- routing/Cargo.toml | 8 ++- routing/benches/fib_lookup.rs | 121 ++++++++++++++++++++++++++++++++++ routing/src/lib.rs | 8 +++ routing/src/rib/nexthop.rs | 8 +-- 4 files changed, 140 insertions(+), 5 deletions(-) create mode 100644 routing/benches/fib_lookup.rs diff --git a/routing/Cargo.toml b/routing/Cargo.toml index 90e72b2c48..5880254a07 100644 --- a/routing/Cargo.toml +++ b/routing/Cargo.toml @@ -56,11 +56,17 @@ procfs = { workspace = true } netdev = { workspace = true } [dev-dependencies] +dataplane-routing = { path = ".", features = ["testing"] } +lpm = { workspace = true, features = ["testing"] } clock = { workspace = true, features = ["virtual"] } +criterion = { workspace = true, features = ["cargo_bench_support"] } bolero = { workspace = true, default-features = false } concurrency = { workspace = true } -lpm = { workspace = true, features = ["testing"] } net = { workspace = true, features = ["test_buffer"] } rand = { workspace = true, default-features = false, features = ["thread_rng"] } tokio = { workspace = true, features = ["time", "test-util"] } tracing-test = { workspace = true, features = [] } + +[[bench]] +name = "fib_lookup" +harness = false diff --git a/routing/benches/fib_lookup.rs b/routing/benches/fib_lookup.rs new file mode 100644 index 0000000000..f112fafbc4 --- /dev/null +++ b/routing/benches/fib_lookup.rs @@ -0,0 +1,121 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +use std::hint::black_box; + +use criterion::{BenchmarkId, Criterion, Throughput, criterion_group, criterion_main}; + +use dataplane_routing::testing::{Fib, FibGroup, FibWriter, FwAction, NhopKey, RouteOrigin}; +use dataplane_routing::{EgressObject, FibEntry, PktInstruction}; +use lpm::prefix::Prefix; +use net::buffer::TestBuffer; +use net::interface::InterfaceIndex; +use net::ip::NextHeader; +use net::packet::Packet; +use net::packet::test_utils::build_test_ipv4_packet_with_transport; + +fn nhop_key(n: u8) -> NhopKey { + NhopKey::new( + RouteOrigin::default(), + Some(format!("10.0.{n}.1").parse().expect("valid address")), + InterfaceIndex::try_new(u32::from(n) + 1).ok(), + None, + FwAction::Forward, + ) +} + +fn fib_group(n: u8, entries: u8) -> FibGroup { + let mut group = FibGroup::new(); + for e in 0..entries { + group.add(FibEntry::with_inst(PktInstruction::Egress( + EgressObject::new( + InterfaceIndex::try_new(u32::from(n) * 256 + u32::from(e) + 1).ok(), + Some(format!("10.{n}.{e}.1").parse().expect("valid address")), + ), + ))); + } + group +} + +fn fib_of_shape(groups: u8, entries_per_group: u8) -> FibWriter { + let (mut writer, _reader) = FibWriter::new(0); + let keys: Vec = (0..groups).map(nhop_key).collect(); + for (n, key) in keys.iter().enumerate() { + let n = u8::try_from(n).expect("group count fits a byte"); + writer.register_fibgroup(key, &fib_group(n, entries_per_group), false); + } + writer.add_fibroute(Prefix::expect_from(ROUTE_ADDR), keys, true); + writer +} + +const ROUTE_ADDR: (&str, u8) = ("5.0.0.0", 8); + +fn packet() -> Packet { + build_test_ipv4_packet_with_transport(64, Some(NextHeader::UDP)) + .expect("a well-formed test packet") +} + +fn bench_lookup(c: &mut Criterion) { + let mut group = c.benchmark_group("fib_lpm_entry_prefix"); + let packet = packet(); + + for (groups, entries) in [(1u8, 1u8), (1, 4), (4, 1), (4, 4), (8, 1), (16, 1), (16, 4)] { + let writer = fib_of_shape(groups, entries); + { + let fib = writer.enter().expect("fib is readable"); + let (hit, _) = Fib::lpm_entry_prefix(&fib, &packet); + assert_eq!( + hit, + Prefix::expect_from(ROUTE_ADDR), + "{groups}g x{entries}e: lookup missed the installed route" + ); + } + let total = u64::from(groups) * u64::from(entries); + group.throughput(Throughput::Elements(1)); + group.bench_with_input( + BenchmarkId::from_parameter(format!("{groups}g x{entries}e ({total} entries)")), + &writer, + |b, writer| { + let fib = writer.enter().expect("fib is readable"); + b.iter(|| { + let (prefix, entry) = Fib::lpm_entry_prefix(&fib, black_box(&packet)); + black_box((prefix, entry)); + }); + }, + ); + } + group.finish(); +} + +fn bench_trie_floor(c: &mut Criterion) { + let mut group = c.benchmark_group("fib_lpm_floor"); + let packet = packet(); + let destination = packet + .ip_destination() + .expect("the test packet has a destination"); + + for (groups, entries) in [(1u8, 1u8), (16, 1)] { + let writer = fib_of_shape(groups, entries); + group.bench_with_input( + BenchmarkId::from_parameter(format!("{groups}g x{entries}e")), + &writer, + |b, writer| { + let fib = writer.enter().expect("fib is readable"); + b.iter(|| { + black_box(fib.lpm_with_prefix(black_box(&destination))); + }); + }, + ); + } + group.finish(); +} + +fn bench_destination(c: &mut Criterion) { + let packet = packet(); + c.bench_function("packet_ip_destination", |b| { + b.iter(|| black_box(black_box(&packet).ip_destination())); + }); +} + +criterion_group!(benches, bench_lookup, bench_trie_floor, bench_destination); +criterion_main!(benches); diff --git a/routing/src/lib.rs b/routing/src/lib.rs index f5d1b3b82a..82496d120b 100644 --- a/routing/src/lib.rs +++ b/routing/src/lib.rs @@ -45,6 +45,14 @@ pub use rib::encapsulation::{ }; pub use rib::vrf::{RouterVrfConfig, VrfId}; +#[cfg(any(test, feature = "testing"))] +pub mod testing { + pub use crate::fib::fibobjects::FibGroup; + pub use crate::fib::fibtype::{Fib, FibReader, FibWriter}; + pub use crate::rib::nexthop::{FwAction, NhopKey}; + pub use crate::rib::vrf::RouteOrigin; +} + pub use bmp::spawn_bmp_server; pub use router::ctl::RouterCtlSender; pub use router::{BmpServerParams, CliSources, Router, RouterParams, RouterParamsBuilder}; diff --git a/routing/src/rib/nexthop.rs b/routing/src/rib/nexthop.rs index 14a74abb62..efa2f137d6 100644 --- a/routing/src/rib/nexthop.rs +++ b/routing/src/rib/nexthop.rs @@ -93,7 +93,7 @@ impl NhopKey { } } #[cfg(test)] - pub fn from_address(address: &str) -> Self { + pub(crate) fn from_address(address: &str) -> Self { Self { address: Some(IpAddr::from_str(address).expect("Bad address")), ..Default::default() @@ -101,7 +101,7 @@ impl NhopKey { } #[cfg(test)] #[must_use] - pub fn with_addr_ifindex(address: &str, ifindex: u32) -> Self { + pub(crate) fn with_addr_ifindex(address: &str, ifindex: u32) -> Self { Self { address: Some(IpAddr::from_str(address).expect("Bad address")), ifindex: Some(InterfaceIndex::try_new(ifindex).expect("Bad ifindex")), @@ -110,7 +110,7 @@ impl NhopKey { } #[cfg(test)] #[must_use] - pub fn with_address(address: &IpAddr) -> Self { + pub(crate) fn with_address(address: &IpAddr) -> Self { Self { address: Some(*address), ..Default::default() @@ -118,7 +118,7 @@ impl NhopKey { } #[cfg(test)] #[must_use] - pub fn with_ifindex(ifindex: u32) -> Self { + pub(crate) fn with_ifindex(ifindex: u32) -> Self { Self { ifindex: Some(InterfaceIndex::try_new(ifindex).unwrap()), ..Default::default() From 88eeed704e2706c641b22beeb2374d877ecbd1ed Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 21 Aug 2026 21:30:24 -0600 Subject: [PATCH 02/13] build(nix): Add valgrind and an iai-callgrind harness Puts callgrind next to the criterion bench it already has, so a benchmark can count work as well as time it, and calibrates the two against each other while there is a known answer to calibrate against. Taking the caching change from the previous commit and measuring it both ways: callgrind reports -4.3% instructions on a one-group route where the machine is 3.1% slower. It counts the instructions the change removes and cannot see that `FibRoute` grew from 24 to 32 bytes, or that the walk it removed was hitting L1 anyway. Its modelled cache does not catch it either -- a fixed generic L1 that this fixture fits in whichever layout it has. Where the change is algorithmic rather than structural the two agree on sign and land within a small factor. So it is worth gating on and not worth deciding with: instruction counts are bit-identical run to run, and blind to layout. Both halves are in development/code/benchmarking.md, along with the two ways these benchmarks were wrong first -- a fixture that missed the code under test, and a measured function that owned its fixture and so mostly timed the drop. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.lock | 48 ++++++++++++ Cargo.toml | 1 + default.nix | 2 + justfile | 5 ++ nix/overlays/dataplane-dev.nix | 4 + nix/pkgs/iai-callgrind-runner/default.nix | 18 +++++ routing/Cargo.toml | 5 ++ routing/benches/fib_lookup_callgrind.rs | 92 +++++++++++++++++++++++ 8 files changed, 175 insertions(+) create mode 100644 nix/pkgs/iai-callgrind-runner/default.nix create mode 100644 routing/benches/fib_lookup_callgrind.rs diff --git a/Cargo.lock b/Cargo.lock index 0f4ec29de2..38457d546c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -398,6 +398,15 @@ version = "0.22.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "72b3254f16251a8381aa12e40e3c4d2f0199f8c6508fbecb9d91f575e0fbb8c6" +[[package]] +name = "bincode" +version = "1.3.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b1f45e9417d87227c7a56d22e471c6206462cba514c7590c09aff4cf6d1ddcad" +dependencies = [ + "serde", +] + [[package]] name = "bindgen" version = "0.72.1" @@ -1788,6 +1797,7 @@ dependencies = [ "bolero", "bytes", "chrono", + "criterion", "dataplane-args", "dataplane-cli", "dataplane-clock", @@ -1799,10 +1809,12 @@ dependencies = [ "dataplane-lifecycle", "dataplane-lpm", "dataplane-net", + "dataplane-routing", "dataplane-tracectl", "derive_builder", "dplane-rpc", "futures-util", + "iai-callgrind", "inotify", "ipnet", "left-right 0.11.7", @@ -2880,6 +2892,42 @@ dependencies = [ "tower-service", ] +[[package]] +name = "iai-callgrind" +version = "0.16.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0b1e4910d3a9137442723dfb772c32dc10674c4181ca078d2fd227cd5dce9db0" +dependencies = [ + "bincode", + "derive_more", + "iai-callgrind-macros", + "iai-callgrind-runner", +] + +[[package]] +name = "iai-callgrind-macros" +version = "0.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4d03775318d3f9f01b39ac6612b01464006dc397a654a89dd57df2fd34fb68c3" +dependencies = [ + "derive_more", + "proc-macro-error2", + "proc-macro2", + "quote", + "serde", + "serde_json", + "syn 2.0.119", +] + +[[package]] +name = "iai-callgrind-runner" +version = "0.16.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b74c9743c00c3bca4aaffc69c87cae56837796cd362438daf354a3f785788c68" +dependencies = [ + "serde", +] + [[package]] name = "iana-time-zone" version = "0.1.65" diff --git a/Cargo.toml b/Cargo.toml index 38d0071a8a..c9c356dd0f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -134,6 +134,7 @@ clap = { version = "4.6.6", default-features = true, features = [] } color-eyre = { version = "0.6.5", default-features = false, features = [] } colored = { version = "3.1.1", default-features = false, features = [] } criterion = { version = "0.8.2", default-features = false, features = [] } +iai-callgrind = { version = "0.16.1" } crossbeam-utils = { version = "0.8.22", default-features = false, features = [] } dashmap = { version = "6.2.1", default-features = false, features = [] } derive_builder = { version = "0.20.2", default-features = false, features = [] } diff --git a/default.nix b/default.nix index 4f8964ec67..2e4a5cc73f 100644 --- a/default.nix +++ b/default.nix @@ -170,6 +170,7 @@ let duvet gateway-crd gettext + iai-callgrind-runner jq just kopium @@ -188,6 +189,7 @@ let rust-toolchain shellcheck skopeo + valgrind wasmtime wget yq diff --git a/justfile b/justfile index d099795855..d1a4feec8b 100644 --- a/justfile +++ b/justfile @@ -208,6 +208,11 @@ bench: (build "benches") shopt -s nullglob for bench in ./results/benches/bin/*; do "$bench" --bench; done +[script] +bench-callgrind *args: + {{ _just_debuggable_ }} + cargo bench -p dataplane-routing --bench fib_lookup_callgrind {{ args }} + [script] build-each *args: (build "workspace" args) {{ _just_debuggable_ }} diff --git a/nix/overlays/dataplane-dev.nix b/nix/overlays/dataplane-dev.nix index 16e84046d8..b9313768d6 100644 --- a/nix/overlays/dataplane-dev.nix +++ b/nix/overlays/dataplane-dev.nix @@ -25,6 +25,10 @@ in opengrep = final.callPackage ../pkgs/opengrep { src = sources.opengrep; }; + iai-callgrind-runner = final.callPackage ../pkgs/iai-callgrind-runner { + inherit (override-packages) rustPlatform; + version = "0.16.1"; + }; cargo-bolero = prev.cargo-bolero.override { inherit (override-packages) rustPlatform; }; cargo-deny = prev.cargo-deny.override { inherit (override-packages) rustPlatform; }; cargo-edit = prev.cargo-edit.override { inherit (override-packages) rustPlatform; }; diff --git a/nix/pkgs/iai-callgrind-runner/default.nix b/nix/pkgs/iai-callgrind-runner/default.nix new file mode 100644 index 0000000000..7033648528 --- /dev/null +++ b/nix/pkgs/iai-callgrind-runner/default.nix @@ -0,0 +1,18 @@ +# SPDX-License-Identifier: Apache-2.0 +# Copyright Open Network Fabric Authors +{ + fetchCrate, + rustPlatform, + version, + ... +}: +rustPlatform.buildRustPackage (final: { + pname = "iai-callgrind-runner"; + inherit version; + src = fetchCrate { + inherit (final) pname version; + hash = "sha256-wJTwaqAz8GWCJ/l9GRXYBVBkpPYrWxN4VQ7GdRFXmzM="; + }; + cargoHash = "sha256-4N7P23bCeeJee/Cm3sSORByh+HzflOENqYqpu629mpA="; + doCheck = false; +}) diff --git a/routing/Cargo.toml b/routing/Cargo.toml index 5880254a07..c85afd8dd7 100644 --- a/routing/Cargo.toml +++ b/routing/Cargo.toml @@ -60,6 +60,7 @@ dataplane-routing = { path = ".", features = ["testing"] } lpm = { workspace = true, features = ["testing"] } clock = { workspace = true, features = ["virtual"] } criterion = { workspace = true, features = ["cargo_bench_support"] } +iai-callgrind = { workspace = true } bolero = { workspace = true, default-features = false } concurrency = { workspace = true } net = { workspace = true, features = ["test_buffer"] } @@ -70,3 +71,7 @@ tracing-test = { workspace = true, features = [] } [[bench]] name = "fib_lookup" harness = false + +[[bench]] +name = "fib_lookup_callgrind" +harness = false diff --git a/routing/benches/fib_lookup_callgrind.rs b/routing/benches/fib_lookup_callgrind.rs new file mode 100644 index 0000000000..bfc92bd87e --- /dev/null +++ b/routing/benches/fib_lookup_callgrind.rs @@ -0,0 +1,92 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +use std::hint::black_box; + +use iai_callgrind::{library_benchmark, library_benchmark_group, main}; + +use dataplane_routing::testing::{Fib, FibGroup, FibWriter, FwAction, NhopKey, RouteOrigin}; +use dataplane_routing::{EgressObject, FibEntry, PktInstruction}; +use lpm::prefix::Prefix; +use net::buffer::TestBuffer; +use net::interface::InterfaceIndex; +use net::ip::NextHeader; +use net::packet::Packet; +use net::packet::test_utils::build_test_ipv4_packet_with_transport; + +const ROUTE_ADDR: (&str, u8) = ("5.0.0.0", 8); + +fn nhop_key(n: u8) -> NhopKey { + NhopKey::new( + RouteOrigin::default(), + Some(format!("10.0.{n}.1").parse().expect("valid address")), + InterfaceIndex::try_new(u32::from(n) + 1).ok(), + None, + FwAction::Forward, + ) +} + +fn fib_group(n: u8, entries: u8) -> FibGroup { + let mut group = FibGroup::new(); + for e in 0..entries { + group.add(FibEntry::with_inst(PktInstruction::Egress( + EgressObject::new( + InterfaceIndex::try_new(u32::from(n) * 256 + u32::from(e) + 1).ok(), + Some(format!("10.{n}.{e}.1").parse().expect("valid address")), + ), + ))); + } + group +} + +struct Fixture { + writer: FibWriter, + packet: Packet, +} + +fn fixture(groups: u8, entries_per_group: u8) -> &'static Fixture { + let (mut writer, _reader) = FibWriter::new(0); + let keys: Vec = (0..groups).map(nhop_key).collect(); + for (n, key) in keys.iter().enumerate() { + let n = u8::try_from(n).expect("group count fits a byte"); + writer.register_fibgroup(key, &fib_group(n, entries_per_group), false); + } + writer.add_fibroute(Prefix::expect_from(ROUTE_ADDR), keys, true); + + let packet = build_test_ipv4_packet_with_transport(64, Some(NextHeader::UDP)) + .expect("a well-formed test packet"); + + { + let fib = writer.enter().expect("fib is readable"); + let (hit, _) = Fib::lpm_entry_prefix(&fib, &packet); + assert_eq!( + hit, + Prefix::expect_from(ROUTE_ADDR), + "{groups}g x{entries_per_group}e: lookup missed the installed route" + ); + } + Box::leak(Box::new(Fixture { writer, packet })) +} + +#[library_benchmark] +#[bench::guard_only(args = (1, 1), setup = fixture)] +fn enter_only(fixture: &'static Fixture) { + black_box(fixture.writer.enter().expect("fib is readable")); +} + +#[library_benchmark] +#[bench::g1_e1(args = (1, 1), setup = fixture)] +#[bench::g1_e4(args = (1, 4), setup = fixture)] +#[bench::g4_e1(args = (4, 1), setup = fixture)] +#[bench::g4_e4(args = (4, 4), setup = fixture)] +#[bench::g8_e1(args = (8, 1), setup = fixture)] +#[bench::g16_e1(args = (16, 1), setup = fixture)] +#[bench::g16_e4(args = (16, 4), setup = fixture)] +fn lpm_entry_prefix(fixture: &'static Fixture) { + let fib = fixture.writer.enter().expect("fib is readable"); + let (prefix, entry) = Fib::lpm_entry_prefix(&fib, black_box(&fixture.packet)); + black_box((prefix, entry)); +} + +library_benchmark_group!(name = fib_lookup; benchmarks = enter_only, lpm_entry_prefix); +main!(library_benchmark_groups = fib_lookup); From 2d1ae0323822f1e87957fdfe48c58041910ab90c Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 21 Aug 2026 21:47:43 -0600 Subject: [PATCH 03/13] docs(bench): Record what the other valgrind tools are for, and where they lie Three things worth knowing before somebody needs one of these in a hurry. The family is wired up and exercised, so reaching for DHAT or massif is a two-line edit. Only callgrind scopes to the benchmark function, though: cachegrind has no call graph and counts the whole process, reporting 453,358 instructions against callgrind's 349 for the same benchmark. Cachegrind can be handed the real cache geometry and now is, but it already reads CPUID and gets L1 right unaided; what it gets wrong is collapsing L2 and L3 into one "LL" and guessing 8 MB against this host's 32 MB. That changed nothing here, because the fixture never leaves L1 -- an argument for realistic fixture sizes rather than for tuning the model. DPDK is the one that matters. Valgrind does not fall over on rte_acl: the whole ACL suite runs, 39.5 billion instructions, no crash. It instead reports a CPU it can emulate, so DPDK's runtime dispatch chose scalar and AVX2 and executed no AVX-512 at all -- on a Zen 4 part that has it, and would use avx512x16/x32 in production. That is not a modelling error to correct for; it is a measurement of a different function. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- routing/benches/fib_lookup_callgrind.rs | 26 +++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/routing/benches/fib_lookup_callgrind.rs b/routing/benches/fib_lookup_callgrind.rs index bfc92bd87e..0a3d5b1e1a 100644 --- a/routing/benches/fib_lookup_callgrind.rs +++ b/routing/benches/fib_lookup_callgrind.rs @@ -3,7 +3,9 @@ use std::hint::black_box; -use iai_callgrind::{library_benchmark, library_benchmark_group, main}; +use iai_callgrind::{ + Cachegrind, Dhat, LibraryBenchmarkConfig, library_benchmark, library_benchmark_group, main, +}; use dataplane_routing::testing::{Fib, FibGroup, FibWriter, FwAction, NhopKey, RouteOrigin}; use dataplane_routing::{EgressObject, FibEntry, PktInstruction}; @@ -88,5 +90,25 @@ fn lpm_entry_prefix(fixture: &'static Fixture) { black_box((prefix, entry)); } -library_benchmark_group!(name = fib_lookup; benchmarks = enter_only, lpm_entry_prefix); +#[library_benchmark( + config = LibraryBenchmarkConfig::default() + .tool(Cachegrind::default().args([ + "--D1=32768,8,64", + "--I1=32768,8,64", + "--LL=33554432,16,64", + ])) + .tool(Dhat::default()) +)] +#[bench::g1_e1(args = (1, 1), setup = fixture)] +#[bench::g16_e4(args = (16, 4), setup = fixture)] +fn under_other_tools(fixture: &'static Fixture) { + let fib = fixture.writer.enter().expect("fib is readable"); + let (prefix, entry) = Fib::lpm_entry_prefix(&fib, black_box(&fixture.packet)); + black_box((prefix, entry)); +} + +library_benchmark_group!( + name = fib_lookup; + benchmarks = enter_only, lpm_entry_prefix, under_other_tools +); main!(library_benchmark_groups = fib_lookup); From 35d52959347613ac97275aa3ca7ef47f22e6f5b3 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 21 Aug 2026 22:02:25 -0600 Subject: [PATCH 04/13] refactor(bench): Define the fib fixtures once for both harnesses Each bench had its own copy of the fixture builder, the measured body and the list of route shapes. Two benchmarks that drift apart do not fail -- they answer different questions while still looking comparable, which turns the calibration between them into a lie that compiles. `benches/common` now holds all three. The shape list is a macro rather than a `const` because the callgrind harness needs one `#[bench::id(..)]` per shape and attributes cannot be looped over, so one definition expands into an array for criterion and into attributes for callgrind. Sharing the body also fixed an asymmetry that was already there: criterion hoisted the read guard out of its loop and callgrind did not, so the pair had been measuring slightly different regions all along. Both include it now, with `enter_only` to subtract. Callgrind reports "No change" on every counter across the refactor, which is the check that the shared body is the same code. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- routing/benches/common/mod.rs | 95 ++++++++++++++++++++++++ routing/benches/fib_lookup.rs | 96 +++++-------------------- routing/benches/fib_lookup_callgrind.rs | 89 ++++------------------- 3 files changed, 125 insertions(+), 155 deletions(-) create mode 100644 routing/benches/common/mod.rs diff --git a/routing/benches/common/mod.rs b/routing/benches/common/mod.rs new file mode 100644 index 0000000000..506f8c3e9f --- /dev/null +++ b/routing/benches/common/mod.rs @@ -0,0 +1,95 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +#![allow(dead_code)] + +use lpm::prefix::Prefix; +use net::buffer::TestBuffer; +use net::interface::InterfaceIndex; +use net::ip::NextHeader; +use net::packet::Packet; +use net::packet::test_utils::build_test_ipv4_packet_with_transport; + +use dataplane_routing::testing::{Fib, FibGroup, FibWriter, FwAction, NhopKey, RouteOrigin}; +use dataplane_routing::{EgressObject, FibEntry, PktInstruction}; + +pub const ROUTE_ADDR: (&str, u8) = ("5.0.0.0", 8); + +#[macro_export] +macro_rules! for_each_shape { + ($expand:ident) => { + $expand! { + g1_e1 = (1, 1), + g1_e4 = (1, 4), + g4_e1 = (4, 1), + g4_e4 = (4, 4), + g8_e1 = (8, 1), + g16_e1 = (16, 1), + g16_e4 = (16, 4), + } + }; +} + +pub struct Fixture { + pub writer: FibWriter, + pub packet: Packet, +} + +fn nhop_key(n: u8) -> NhopKey { + NhopKey::new( + RouteOrigin::default(), + Some(format!("10.0.{n}.1").parse().expect("valid address")), + InterfaceIndex::try_new(u32::from(n) + 1).ok(), + None, + FwAction::Forward, + ) +} + +fn fib_group(n: u8, entries: u8) -> FibGroup { + let mut group = FibGroup::new(); + for e in 0..entries { + group.add(FibEntry::with_inst(PktInstruction::Egress( + EgressObject::new( + InterfaceIndex::try_new(u32::from(n) * 256 + u32::from(e) + 1).ok(), + Some(format!("10.{n}.{e}.1").parse().expect("valid address")), + ), + ))); + } + group +} + +pub fn packet() -> Packet { + build_test_ipv4_packet_with_transport(64, Some(NextHeader::UDP)) + .expect("a well-formed test packet") +} + +pub fn fixture(groups: u8, entries_per_group: u8) -> &'static Fixture { + let (mut writer, _reader) = FibWriter::new(0); + let keys: Vec = (0..groups).map(nhop_key).collect(); + for (n, key) in keys.iter().enumerate() { + let n = u8::try_from(n).expect("group count fits a byte"); + writer.register_fibgroup(key, &fib_group(n, entries_per_group), false); + } + writer.add_fibroute(Prefix::expect_from(ROUTE_ADDR), keys, true); + + let packet = packet(); + + { + let fib = writer.enter().expect("fib is readable"); + let (hit, _) = Fib::lpm_entry_prefix(&fib, &packet); + assert_eq!( + hit, + Prefix::expect_from(ROUTE_ADDR), + "{groups}g x{entries_per_group}e: lookup missed the installed route" + ); + } + + Box::leak(Box::new(Fixture { writer, packet })) +} + +#[inline(always)] +pub fn lookup(fixture: &Fixture) { + let fib = fixture.writer.enter().expect("fib is readable"); + let (prefix, entry) = Fib::lpm_entry_prefix(&fib, std::hint::black_box(&fixture.packet)); + std::hint::black_box((prefix, entry)); +} diff --git a/routing/benches/fib_lookup.rs b/routing/benches/fib_lookup.rs index f112fafbc4..c4cb562a8b 100644 --- a/routing/benches/fib_lookup.rs +++ b/routing/benches/fib_lookup.rs @@ -5,83 +5,26 @@ use std::hint::black_box; use criterion::{BenchmarkId, Criterion, Throughput, criterion_group, criterion_main}; -use dataplane_routing::testing::{Fib, FibGroup, FibWriter, FwAction, NhopKey, RouteOrigin}; -use dataplane_routing::{EgressObject, FibEntry, PktInstruction}; -use lpm::prefix::Prefix; -use net::buffer::TestBuffer; -use net::interface::InterfaceIndex; -use net::ip::NextHeader; -use net::packet::Packet; -use net::packet::test_utils::build_test_ipv4_packet_with_transport; +mod common; +use common::{Fixture, fixture, lookup, packet}; -fn nhop_key(n: u8) -> NhopKey { - NhopKey::new( - RouteOrigin::default(), - Some(format!("10.0.{n}.1").parse().expect("valid address")), - InterfaceIndex::try_new(u32::from(n) + 1).ok(), - None, - FwAction::Forward, - ) -} - -fn fib_group(n: u8, entries: u8) -> FibGroup { - let mut group = FibGroup::new(); - for e in 0..entries { - group.add(FibEntry::with_inst(PktInstruction::Egress( - EgressObject::new( - InterfaceIndex::try_new(u32::from(n) * 256 + u32::from(e) + 1).ok(), - Some(format!("10.{n}.{e}.1").parse().expect("valid address")), - ), - ))); - } - group -} - -fn fib_of_shape(groups: u8, entries_per_group: u8) -> FibWriter { - let (mut writer, _reader) = FibWriter::new(0); - let keys: Vec = (0..groups).map(nhop_key).collect(); - for (n, key) in keys.iter().enumerate() { - let n = u8::try_from(n).expect("group count fits a byte"); - writer.register_fibgroup(key, &fib_group(n, entries_per_group), false); - } - writer.add_fibroute(Prefix::expect_from(ROUTE_ADDR), keys, true); - writer -} - -const ROUTE_ADDR: (&str, u8) = ("5.0.0.0", 8); - -fn packet() -> Packet { - build_test_ipv4_packet_with_transport(64, Some(NextHeader::UDP)) - .expect("a well-formed test packet") +macro_rules! shape_table { + ($($id:ident = ($groups:expr, $entries:expr)),* $(,)?) => { + &[$((stringify!($id), $groups, $entries)),*] + }; } +const SHAPES: &[(&str, u8, u8)] = for_each_shape!(shape_table); fn bench_lookup(c: &mut Criterion) { let mut group = c.benchmark_group("fib_lpm_entry_prefix"); - let packet = packet(); - - for (groups, entries) in [(1u8, 1u8), (1, 4), (4, 1), (4, 4), (8, 1), (16, 1), (16, 4)] { - let writer = fib_of_shape(groups, entries); - { - let fib = writer.enter().expect("fib is readable"); - let (hit, _) = Fib::lpm_entry_prefix(&fib, &packet); - assert_eq!( - hit, - Prefix::expect_from(ROUTE_ADDR), - "{groups}g x{entries}e: lookup missed the installed route" - ); - } + for &(id, groups, entries) in SHAPES { + let fixture: &'static Fixture = fixture(groups, entries); let total = u64::from(groups) * u64::from(entries); group.throughput(Throughput::Elements(1)); group.bench_with_input( - BenchmarkId::from_parameter(format!("{groups}g x{entries}e ({total} entries)")), - &writer, - |b, writer| { - let fib = writer.enter().expect("fib is readable"); - b.iter(|| { - let (prefix, entry) = Fib::lpm_entry_prefix(&fib, black_box(&packet)); - black_box((prefix, entry)); - }); - }, + BenchmarkId::from_parameter(format!("{id} ({total} entries)")), + &fixture, + |b, fixture| b.iter(|| lookup(black_box(fixture))), ); } group.finish(); @@ -94,16 +37,13 @@ fn bench_trie_floor(c: &mut Criterion) { .ip_destination() .expect("the test packet has a destination"); - for (groups, entries) in [(1u8, 1u8), (16, 1)] { - let writer = fib_of_shape(groups, entries); - group.bench_with_input( + for &(groups, entries) in &[(1u8, 1u8), (16, 1)] { + let fixture = fixture(groups, entries); + group.bench_function( BenchmarkId::from_parameter(format!("{groups}g x{entries}e")), - &writer, - |b, writer| { - let fib = writer.enter().expect("fib is readable"); - b.iter(|| { - black_box(fib.lpm_with_prefix(black_box(&destination))); - }); + |b| { + let fib = fixture.writer.enter().expect("fib is readable"); + b.iter(|| black_box(fib.lpm_with_prefix(black_box(&destination)))); }, ); } diff --git a/routing/benches/fib_lookup_callgrind.rs b/routing/benches/fib_lookup_callgrind.rs index 0a3d5b1e1a..5bcee45331 100644 --- a/routing/benches/fib_lookup_callgrind.rs +++ b/routing/benches/fib_lookup_callgrind.rs @@ -7,68 +7,8 @@ use iai_callgrind::{ Cachegrind, Dhat, LibraryBenchmarkConfig, library_benchmark, library_benchmark_group, main, }; -use dataplane_routing::testing::{Fib, FibGroup, FibWriter, FwAction, NhopKey, RouteOrigin}; -use dataplane_routing::{EgressObject, FibEntry, PktInstruction}; -use lpm::prefix::Prefix; -use net::buffer::TestBuffer; -use net::interface::InterfaceIndex; -use net::ip::NextHeader; -use net::packet::Packet; -use net::packet::test_utils::build_test_ipv4_packet_with_transport; - -const ROUTE_ADDR: (&str, u8) = ("5.0.0.0", 8); - -fn nhop_key(n: u8) -> NhopKey { - NhopKey::new( - RouteOrigin::default(), - Some(format!("10.0.{n}.1").parse().expect("valid address")), - InterfaceIndex::try_new(u32::from(n) + 1).ok(), - None, - FwAction::Forward, - ) -} - -fn fib_group(n: u8, entries: u8) -> FibGroup { - let mut group = FibGroup::new(); - for e in 0..entries { - group.add(FibEntry::with_inst(PktInstruction::Egress( - EgressObject::new( - InterfaceIndex::try_new(u32::from(n) * 256 + u32::from(e) + 1).ok(), - Some(format!("10.{n}.{e}.1").parse().expect("valid address")), - ), - ))); - } - group -} - -struct Fixture { - writer: FibWriter, - packet: Packet, -} - -fn fixture(groups: u8, entries_per_group: u8) -> &'static Fixture { - let (mut writer, _reader) = FibWriter::new(0); - let keys: Vec = (0..groups).map(nhop_key).collect(); - for (n, key) in keys.iter().enumerate() { - let n = u8::try_from(n).expect("group count fits a byte"); - writer.register_fibgroup(key, &fib_group(n, entries_per_group), false); - } - writer.add_fibroute(Prefix::expect_from(ROUTE_ADDR), keys, true); - - let packet = build_test_ipv4_packet_with_transport(64, Some(NextHeader::UDP)) - .expect("a well-formed test packet"); - - { - let fib = writer.enter().expect("fib is readable"); - let (hit, _) = Fib::lpm_entry_prefix(&fib, &packet); - assert_eq!( - hit, - Prefix::expect_from(ROUTE_ADDR), - "{groups}g x{entries_per_group}e: lookup missed the installed route" - ); - } - Box::leak(Box::new(Fixture { writer, packet })) -} +mod common; +use common::{Fixture, fixture, lookup}; #[library_benchmark] #[bench::guard_only(args = (1, 1), setup = fixture)] @@ -76,19 +16,16 @@ fn enter_only(fixture: &'static Fixture) { black_box(fixture.writer.enter().expect("fib is readable")); } -#[library_benchmark] -#[bench::g1_e1(args = (1, 1), setup = fixture)] -#[bench::g1_e4(args = (1, 4), setup = fixture)] -#[bench::g4_e1(args = (4, 1), setup = fixture)] -#[bench::g4_e4(args = (4, 4), setup = fixture)] -#[bench::g8_e1(args = (8, 1), setup = fixture)] -#[bench::g16_e1(args = (16, 1), setup = fixture)] -#[bench::g16_e4(args = (16, 4), setup = fixture)] -fn lpm_entry_prefix(fixture: &'static Fixture) { - let fib = fixture.writer.enter().expect("fib is readable"); - let (prefix, entry) = Fib::lpm_entry_prefix(&fib, black_box(&fixture.packet)); - black_box((prefix, entry)); +macro_rules! shape_benches { + ($($id:ident = ($groups:expr, $entries:expr)),* $(,)?) => { + #[library_benchmark] + $(#[bench::$id(args = ($groups, $entries), setup = fixture)])* + fn lpm_entry_prefix(fixture: &'static Fixture) { + lookup(fixture); + } + }; } +for_each_shape!(shape_benches); #[library_benchmark( config = LibraryBenchmarkConfig::default() @@ -102,9 +39,7 @@ fn lpm_entry_prefix(fixture: &'static Fixture) { #[bench::g1_e1(args = (1, 1), setup = fixture)] #[bench::g16_e4(args = (16, 4), setup = fixture)] fn under_other_tools(fixture: &'static Fixture) { - let fib = fixture.writer.enter().expect("fib is readable"); - let (prefix, entry) = Fib::lpm_entry_prefix(&fib, black_box(&fixture.packet)); - black_box((prefix, entry)); + lookup(fixture); } library_benchmark_group!( From 4d555bd79267cbd1f9bd625b049d7afdadc504b4 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 21 Aug 2026 22:14:54 -0600 Subject: [PATCH 05/13] feat(bench): Render a callgrind run as a markdown report `just bench-compare` prints a headline, a table of every metric, and a bar chart when something moved past a threshold. The report needs no memory of previous runs, which is the part worth explaining. Storing yesterday's numbers would make the answer depend on which runner recorded them. Callgrind counts work rather than measuring time, so the comparison only requires both sides to run on the same machine -- and they can do so in the same job: base with `--save-baseline`, head with `--baseline`. Each record then carries both values and the delta already, and nothing outlives the run. The full report belongs in `$GITHUB_STEP_SUMMARY` with the headline as a sticky comment linking to it. A gist would need a personal access token with `gist` scope, since `GITHUB_TOKEN` cannot create one -- a credential to rotate in exchange for an artifact that is neither permalinked to the run nor able to render mermaid. The workflow itself is described rather than written: it cannot be exercised from here, and an untested workflow that comments on every pull request is worse than a documented one somebody runs once by hand. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- justfile | 13 ++++ scripts/bench-report.ts | 162 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 175 insertions(+) create mode 100755 scripts/bench-report.ts diff --git a/justfile b/justfile index d1a4feec8b..6daebf8730 100644 --- a/justfile +++ b/justfile @@ -213,6 +213,19 @@ bench-callgrind *args: {{ _just_debuggable_ }} cargo bench -p dataplane-routing --bench fib_lookup_callgrind {{ args }} +[script] +bench-compare baseline="base" *args: + {{ _just_debuggable_ }} + mkdir -p results/bench + if [ -d "target/iai" ] && cargo bench -p dataplane-routing --bench fib_lookup_callgrind -- \ + --baseline='{{ baseline }}' --output-format=json > results/bench/run.jsonl 2>/dev/null; then + ./scripts/bench-report.ts results/bench/run.jsonl {{ args }} + else + cargo bench -p dataplane-routing --bench fib_lookup_callgrind -- \ + --save-baseline='{{ baseline }}' --output-format=json > results/bench/run.jsonl + ./scripts/bench-report.ts results/bench/run.jsonl {{ args }} + fi + [script] build-each *args: (build "workspace" args) {{ _just_debuggable_ }} diff --git a/scripts/bench-report.ts b/scripts/bench-report.ts new file mode 100755 index 0000000000..c4fdf384b1 --- /dev/null +++ b/scripts/bench-report.ts @@ -0,0 +1,162 @@ +#!/usr/bin/env -S deno run --allow-read +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + + +type Metric = { Int: number } | { Float: number }; + +type Metrics = + | { Both: [Metric, Metric] } + | { Left: Metric } + | { Right: Metric }; + +interface Record_ { + id: string | null; + function_name: string; + module_path: string; + profiles: Array<{ + tool: string; + summaries: { + total: { + summary: Record>; + }; + }; + }>; +} + +const HEADLINE: Record = { + Callgrind: ["Ir", "EstimatedCycles"], + Cachegrind: ["Ir", "EstimatedCycles"], + DHAT: ["TotalBytes", "TotalBlocks"], + Memcheck: ["Errors"], + Massif: ["PeakBytes"], +}; + +function value(m: Metric): number { + return "Int" in m ? m.Int : m.Float; +} + +interface Row { + bench: string; + tool: string; + metric: string; + now: number; + before: number | null; + pct: number | null; +} + +function rows(records: Record_[]): Row[] { + const out: Row[] = []; + for (const rec of records) { + const bench = rec.id ? `${rec.function_name} ${rec.id}` : rec.function_name; + for (const profile of rec.profiles ?? []) { + const summaries = profile.summaries?.total?.summary ?? {}; + const summary = summaries[profile.tool] ?? Object.values(summaries)[0]; + if (!summary) continue; + for (const metric of HEADLINE[profile.tool] ?? []) { + const entry = summary[metric]; + if (!entry) continue; + const m = entry.metrics; + const now = "Both" in m ? value(m.Both[0]) : "Left" in m ? value(m.Left) : value(m.Right); + const before = "Both" in m ? value(m.Both[1]) : null; + const pct = before === null || before === 0 ? null : ((now - before) / before) * 100; + out.push({ bench, tool: profile.tool, metric, now, before, pct }); + } + } + } + return out; +} + +const fmt = (n: number) => n.toLocaleString("en-US"); +const pct = (p: number | null) => (p === null ? "—" : `${p >= 0 ? "+" : ""}${p.toFixed(2)}%`); + +function bar(p: number | null, worst: number): string { + if (p === null || worst === 0 || Math.abs(p) < 0.005) return ""; + const width = Math.min(10, Math.round((Math.abs(p) / worst) * 10)); + return (p >= 0 ? "▰" : "▱").repeat(Math.max(1, width)); +} + +function table(rs: Row[]): string { + const worst = Math.max(0, ...rs.map((r) => Math.abs(r.pct ?? 0))); + const head = "| benchmark | tool | metric | before | after | change | |\n|---|---|---|---:|---:|---:|---|"; + const body = rs.map((r) => + `| ${r.bench} | ${r.tool} | ${r.metric} | ${r.before === null ? "—" : fmt(r.before)} | ${fmt(r.now)} | ${pct(r.pct)} | ${bar(r.pct, worst)} |` + ); + return [head, ...body].join("\n"); +} + +function chart(rs: Row[]): string { + const shown = rs.filter((r) => r.tool === "Callgrind" && r.metric === "Ir" && r.pct !== null); + if (shown.length === 0) return ""; + const labels = shown.map((r) => `"${r.bench.replace(/"/g, "")}"`).join(", "); + const values = shown.map((r) => (r.pct ?? 0).toFixed(2)).join(", "); + const span = Math.max(5, ...shown.map((r) => Math.abs(r.pct ?? 0))) * 1.2; + return [ + "```mermaid", + "xychart-beta", + ' title "Instructions retired, change vs baseline (%)"', + ` x-axis [${labels}]`, + ` y-axis "change (%)" ${(-span).toFixed(0)} --> ${span.toFixed(0)}`, + ` bar [${values}]`, + "```", + ].join("\n"); +} + +function main() { + const args = Deno.args.filter((a) => !a.startsWith("--")); + const flags = Deno.args.filter((a) => a.startsWith("--")); + const threshold = Number( + flags.find((f) => f.startsWith("--threshold="))?.split("=")[1] ?? "5", + ); + const headlineOnly = flags.includes("--headline-only"); + + if (args.length !== 1) { + console.error("usage: bench-report.ts [--threshold=N] [--headline-only]"); + Deno.exit(2); + } + + const records: Record_[] = Deno.readTextFileSync(args[0]) + .split("\n") + .filter((line) => line.trim().length > 0) + .map((line) => JSON.parse(line)); + + const rs = rows(records); + const compared = rs.filter((r) => r.pct !== null); + const worst = compared.reduce( + (acc: Row | null, r) => (acc === null || Math.abs(r.pct!) > Math.abs(acc.pct!) ? r : acc), + null, + ); + const stark = worst !== null && Math.abs(worst.pct!) >= threshold; + + if (compared.length === 0) { + console.log("**Benchmarks**: no baseline to compare against; recorded a new one."); + } else if (!stark) { + console.log( + `**Benchmarks**: no change beyond ${threshold}% (largest: ${worst!.bench} ${worst!.metric} ${pct(worst!.pct)}).`, + ); + } else { + const dir = worst!.pct! > 0 ? "more" : "less"; + console.log( + `**Benchmarks**: ${worst!.bench} does ${pct(worst!.pct)} ${dir} work (${worst!.metric}).`, + ); + } + if (headlineOnly) return; + + console.log(""); + console.log(table(rs)); + if (stark) { + console.log(""); + console.log(chart(rs)); + } + console.log(""); + console.log( + "> Instruction counts from callgrind, not timings: repeatable across runners, and blind to " + + "anything that changes data layout rather than instruction count. See " + + "`development/code/benchmarking.md`.", + ); +} + +main(); From 19b66d3274b18139ff33d36e215aba4021618afb Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 21 Aug 2026 22:21:42 -0600 Subject: [PATCH 06/13] fix(bench): Headline instructions, and say when memory disagrees Running the report against a real change -- the FibRoute caching experiment -- turned up two things a synthetic test could not. The headline was picking whichever number moved most, which meant `EstimatedCycles` beat `Ir` by four hundredths of a percent and put the least trustworthy metric at the top. It is a formula over the counters, not a time, and it moves alongside `Ir` without adding anything. More useful: DHAT sees what callgrind cannot. That change grows `FibRoute` from 24 bytes to 32, which is invisible to an instruction count and is the whole reason it loses on a real machine -- but allocation totals move, 7,226 bytes to 7,354. So the headline now names both, and "less work, more memory" is called out in the guide as the shape of change most likely to read as a win here and lose in production. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- scripts/bench-report.ts | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/scripts/bench-report.ts b/scripts/bench-report.ts index c4fdf384b1..cf99ee0b3f 100755 --- a/scripts/bench-report.ts +++ b/scripts/bench-report.ts @@ -125,22 +125,33 @@ function main() { const rs = rows(records); const compared = rs.filter((r) => r.pct !== null); - const worst = compared.reduce( - (acc: Row | null, r) => (acc === null || Math.abs(r.pct!) > Math.abs(acc.pct!) ? r : acc), - null, - ); + + const worstOf = (subset: Row[]) => + subset.reduce( + (acc: Row | null, r) => (acc === null || Math.abs(r.pct!) > Math.abs(acc.pct!) ? r : acc), + null, + ); + const worst = worstOf(compared.filter((r) => r.tool === "Callgrind" && r.metric === "Ir")) ?? + worstOf(compared); const stark = worst !== null && Math.abs(worst.pct!) >= threshold; + const alloc = worstOf( + compared.filter((r) => r.tool === "DHAT" && r.metric === "TotalBytes" && Math.abs(r.pct!) >= 1), + ); + const allocNote = alloc === null + ? "" + : `; bytes allocated ${pct(alloc.pct)} (DHAT)`; + if (compared.length === 0) { console.log("**Benchmarks**: no baseline to compare against; recorded a new one."); } else if (!stark) { console.log( - `**Benchmarks**: no change beyond ${threshold}% (largest: ${worst!.bench} ${worst!.metric} ${pct(worst!.pct)}).`, + `**Benchmarks**: no change beyond ${threshold}% (largest: ${worst!.bench} ${worst!.metric} ${pct(worst!.pct)})${allocNote}.`, ); } else { const dir = worst!.pct! > 0 ? "more" : "less"; console.log( - `**Benchmarks**: ${worst!.bench} does ${pct(worst!.pct)} ${dir} work (${worst!.metric}).`, + `**Benchmarks**: ${worst!.bench} does ${pct(worst!.pct)} ${dir} work (${worst!.metric})${allocNote}.`, ); } if (headlineOnly) return; From 44327f1835156c4f36caf2fa363571915da83dde Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 21 Aug 2026 23:08:11 -0600 Subject: [PATCH 07/13] refactor(just): Move the bench recipes into their own module Follows `ci` and `miri`: `just bench criterion`, `just bench callgrind`, `just bench compare`, `just bench baseline`. A module cannot share a name with a recipe, so the criterion one moved too and now invokes `just build benches` rather than depending on it. Which turned up a bug the flat layout had hidden. The nix `benches` build produces `fib_lookup_callgrind` alongside the criterion binaries, and the loop ran everything it found -- so `just bench` was respawning the callgrind target under valgrind and overwriting whatever baseline `compare` had stored. The loop now skips `*_callgrind`, which is what the naming convention was for. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- bench.just | 88 +++++++++++++++++++++++++++++++++++++++++ justfile | 28 +------------ scripts/bench-report.ts | 77 +++++++++++++++++++++++++----------- 3 files changed, 143 insertions(+), 50 deletions(-) create mode 100644 bench.just diff --git a/bench.just b/bench.just new file mode 100644 index 0000000000..f2cd57d86e --- /dev/null +++ b/bench.just @@ -0,0 +1,88 @@ +# SPDX-License-Identifier: Apache-2.0 +# Copyright Open Network Fabric Authors + +set unstable := true +set shell := ["/usr/bin/env", "bash", "-euo", "pipefail", "-c"] +set script-interpreter := ["/usr/bin/env", "bash", "-euo", "pipefail"] + +# enable to debug just recipes +debug_justfile := "false" + +[private] +_just_debuggable_ := if debug_justfile == "true" { "set -x" } else { "" } + +# The crate and bench target the callgrind recipes drive. +# +# One target today. When there are more, this is the knob that decides which of them a report +# covers, and the recipes below take the same shape for each. +export callgrind_package := "dataplane-routing" +export callgrind_bench := "fib_lookup_callgrind" + +# What a benchmark run can tell you, and what it cannot, is in +# `development/code/benchmarking.md`. Read it before drawing a conclusion from one of these. +[private] +default: criterion + +# Build and run the criterion benches: wall-clock time on this machine. +# +# The rte_acl benches are gated behind the `dpdk` feature, so run `just features=dpdk bench +# criterion` to exercise them; a plain run builds them as an empty `main()` and only runs the +# reference benches. +# +# Goes through the nix build rather than cargo so the binaries are the ones CI would produce. +# +# Skips the `*_callgrind` targets, which the same build produces. They are not criterion benches: +# run through this loop they respawn themselves under valgrind and, worse, overwrite whatever +# baseline `just bench compare` had stored. +[doc("Wall-clock time, via criterion")] +[script] +criterion *args: + {{ _just_debuggable_ }} + just build benches {{ args }} + shopt -s nullglob + for bench in ./results/benches/bin/*; do + case "${bench}" in + *_callgrind) continue ;; + esac + "${bench}" --bench + done + +# Run the iai-callgrind benches: instructions retired and modelled cache traffic, rather than +# wall-clock. Bit-for-bit repeatable run to run, which is what makes them gateable in CI -- and +# blind to anything that changes data layout rather than instruction count. +[doc("Instructions and cache traffic, via iai-callgrind")] +[script] +callgrind *args: + {{ _just_debuggable_ }} + cargo bench -p "${callgrind_package}" --bench "${callgrind_bench}" {{ args }} + +# Compare the callgrind benches against a baseline and print a markdown report. +# +# On its own this records a baseline from the current tree and has nothing to compare against; run +# it once on the base commit and again on your branch. CI does exactly that, both times in one job +# on one runner, which is what makes the comparison meaningful without anything being stored +# between jobs. +[doc("Compare against a baseline and print a markdown report")] +[script] +compare baseline="base" *args: + {{ _just_debuggable_ }} + mkdir -p results/bench + if [ -d "target/iai" ] && cargo bench -p "${callgrind_package}" --bench "${callgrind_bench}" -- \ + --baseline='{{ baseline }}' --output-format=json > results/bench/run.jsonl 2>/dev/null; then + ./scripts/bench-report.ts results/bench/run.jsonl {{ args }} + else + cargo bench -p "${callgrind_package}" --bench "${callgrind_bench}" -- \ + --save-baseline='{{ baseline }}' --output-format=json > results/bench/run.jsonl + ./scripts/bench-report.ts results/bench/run.jsonl {{ args }} + fi + +# Record a baseline for `compare` to measure against, without reporting. +# +# CI runs this on the base commit before checking out the head of the branch. +[doc("Record a baseline for `compare`, without reporting")] +[script] +baseline name="base": + {{ _just_debuggable_ }} + cargo bench -p "${callgrind_package}" --bench "${callgrind_bench}" -- \ + --save-baseline='{{ name }}' > /dev/null + echo "recorded baseline '{{ name }}'" diff --git a/justfile b/justfile index 6daebf8730..c727ccd3cc 100644 --- a/justfile +++ b/justfile @@ -5,6 +5,7 @@ set unstable := true set shell := ["/usr/bin/env", "bash", "-euo", "pipefail", "-c"] set script-interpreter := ["/usr/bin/env", "bash", "-euo", "pipefail"] +mod bench mod ci mod miri @@ -199,33 +200,6 @@ fuzz target time="60s" *args="": {{ if sanitize == "thread" { "--build-std" } else { "" } }} \ {{ _cargo_feature_flags }} {{ args }} -# Build and run the criterion benches. The rte_acl benches are gated behind the -# `dpdk` feature, so run `just features=dpdk bench` to exercise them; a plain -# `just bench` builds them as empty `main()` and only runs the reference benches. -[script] -bench: (build "benches") - {{ _just_debuggable_ }} - shopt -s nullglob - for bench in ./results/benches/bin/*; do "$bench" --bench; done - -[script] -bench-callgrind *args: - {{ _just_debuggable_ }} - cargo bench -p dataplane-routing --bench fib_lookup_callgrind {{ args }} - -[script] -bench-compare baseline="base" *args: - {{ _just_debuggable_ }} - mkdir -p results/bench - if [ -d "target/iai" ] && cargo bench -p dataplane-routing --bench fib_lookup_callgrind -- \ - --baseline='{{ baseline }}' --output-format=json > results/bench/run.jsonl 2>/dev/null; then - ./scripts/bench-report.ts results/bench/run.jsonl {{ args }} - else - cargo bench -p dataplane-routing --bench fib_lookup_callgrind -- \ - --save-baseline='{{ baseline }}' --output-format=json > results/bench/run.jsonl - ./scripts/bench-report.ts results/bench/run.jsonl {{ args }} - fi - [script] build-each *args: (build "workspace" args) {{ _just_debuggable_ }} diff --git a/scripts/bench-report.ts b/scripts/bench-report.ts index cf99ee0b3f..4a59184363 100755 --- a/scripts/bench-report.ts +++ b/scripts/bench-report.ts @@ -5,10 +5,9 @@ type Metric = { Int: number } | { Float: number }; -type Metrics = - | { Both: [Metric, Metric] } - | { Left: Metric } - | { Right: Metric }; +type Metrics = { Both: [Metric, Metric] } | { Left: Metric } | { + Right: Metric; +}; interface Record_ { id: string | null; @@ -18,10 +17,16 @@ interface Record_ { tool: string; summaries: { total: { - summary: Record>; + summary: Record< + string, + Record< + string, + { + diffs?: { diff_pct?: string }; + metrics: Metrics; + } + > + >; }; }; }>; @@ -60,9 +65,15 @@ function rows(records: Record_[]): Row[] { const entry = summary[metric]; if (!entry) continue; const m = entry.metrics; - const now = "Both" in m ? value(m.Both[0]) : "Left" in m ? value(m.Left) : value(m.Right); + const now = "Both" in m + ? value(m.Both[0]) + : "Left" in m + ? value(m.Left) + : value(m.Right); const before = "Both" in m ? value(m.Both[1]) : null; - const pct = before === null || before === 0 ? null : ((now - before) / before) * 100; + const pct = before === null || before === 0 + ? null + : ((now - before) / before) * 100; out.push({ bench, tool: profile.tool, metric, now, before, pct }); } } @@ -71,7 +82,8 @@ function rows(records: Record_[]): Row[] { } const fmt = (n: number) => n.toLocaleString("en-US"); -const pct = (p: number | null) => (p === null ? "—" : `${p >= 0 ? "+" : ""}${p.toFixed(2)}%`); +const pct = (p: number | null) => + p === null ? "—" : `${p >= 0 ? "+" : ""}${p.toFixed(2)}%`; function bar(p: number | null, worst: number): string { if (p === null || worst === 0 || Math.abs(p) < 0.005) return ""; @@ -81,15 +93,21 @@ function bar(p: number | null, worst: number): string { function table(rs: Row[]): string { const worst = Math.max(0, ...rs.map((r) => Math.abs(r.pct ?? 0))); - const head = "| benchmark | tool | metric | before | after | change | |\n|---|---|---|---:|---:|---:|---|"; - const body = rs.map((r) => - `| ${r.bench} | ${r.tool} | ${r.metric} | ${r.before === null ? "—" : fmt(r.before)} | ${fmt(r.now)} | ${pct(r.pct)} | ${bar(r.pct, worst)} |` + const head = + "| benchmark | tool | metric | before | after | change | |\n|---|---|---|---:|---:|---:|---|"; + const body = rs.map( + (r) => + `| ${r.bench} | ${r.tool} | ${r.metric} | ${ + r.before === null ? "—" : fmt(r.before) + } | ${fmt(r.now)} | ${pct(r.pct)} | ${bar(r.pct, worst)} |`, ); return [head, ...body].join("\n"); } function chart(rs: Row[]): string { - const shown = rs.filter((r) => r.tool === "Callgrind" && r.metric === "Ir" && r.pct !== null); + const shown = rs.filter( + (r) => r.tool === "Callgrind" && r.metric === "Ir" && r.pct !== null, + ); if (shown.length === 0) return ""; const labels = shown.map((r) => `"${r.bench.replace(/"/g, "")}"`).join(", "); const values = shown.map((r) => (r.pct ?? 0).toFixed(2)).join(", "); @@ -114,7 +132,9 @@ function main() { const headlineOnly = flags.includes("--headline-only"); if (args.length !== 1) { - console.error("usage: bench-report.ts [--threshold=N] [--headline-only]"); + console.error( + "usage: bench-report.ts [--threshold=N] [--headline-only]", + ); Deno.exit(2); } @@ -128,30 +148,41 @@ function main() { const worstOf = (subset: Row[]) => subset.reduce( - (acc: Row | null, r) => (acc === null || Math.abs(r.pct!) > Math.abs(acc.pct!) ? r : acc), + (acc: Row | null, r) => + acc === null || Math.abs(r.pct!) > Math.abs(acc.pct!) ? r : acc, null, ); - const worst = worstOf(compared.filter((r) => r.tool === "Callgrind" && r.metric === "Ir")) ?? - worstOf(compared); + const worst = worstOf( + compared.filter((r) => r.tool === "Callgrind" && r.metric === "Ir"), + ) ?? worstOf(compared); const stark = worst !== null && Math.abs(worst.pct!) >= threshold; const alloc = worstOf( - compared.filter((r) => r.tool === "DHAT" && r.metric === "TotalBytes" && Math.abs(r.pct!) >= 1), + compared.filter( + (r) => + r.tool === "DHAT" && r.metric === "TotalBytes" && Math.abs(r.pct!) >= 1, + ), ); const allocNote = alloc === null ? "" : `; bytes allocated ${pct(alloc.pct)} (DHAT)`; if (compared.length === 0) { - console.log("**Benchmarks**: no baseline to compare against; recorded a new one."); + console.log( + "**Benchmarks**: no baseline to compare against; recorded a new one.", + ); } else if (!stark) { console.log( - `**Benchmarks**: no change beyond ${threshold}% (largest: ${worst!.bench} ${worst!.metric} ${pct(worst!.pct)})${allocNote}.`, + `**Benchmarks**: no change beyond ${threshold}% (largest: ${ + worst!.bench + } ${worst!.metric} ${pct(worst!.pct)})${allocNote}.`, ); } else { const dir = worst!.pct! > 0 ? "more" : "less"; console.log( - `**Benchmarks**: ${worst!.bench} does ${pct(worst!.pct)} ${dir} work (${worst!.metric})${allocNote}.`, + `**Benchmarks**: ${worst!.bench} does ${pct(worst!.pct)} ${dir} work (${ + worst!.metric + })${allocNote}.`, ); } if (headlineOnly) return; From 4b67aa8dfe7d9908d8e3e8e27b97beacbe352cd0 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 21 Aug 2026 23:58:59 -0600 Subject: [PATCH 08/13] feat(bench): Emit html reports, and serve them Criterion's html report needs `html_reports` and a `plotters` backend, neither of which survived `default-features = false`. Both members wanted the same set, so it moves to the workspace. `rayon` stays off: it parallelises sampling, trading run-to-run stability for wall-clock we do not need back. Opening the result from `file://` does not work -- the pages fetch their siblings relatively and the origin refuses, so the index renders and everything under it is empty. `static-web-server` joins the dev shell, with a generic `just serve` behind `just bench serve` and `just serve-coverage`, since llvm-cov's report has the same problem. Verifying that turned up something worse than the missing feature. The nix `benches` build inherits the root justfile's `profile`, which is `debug`, so this recipe has always benchmarked unoptimised binaries: the same benchmark reads 20.9ns there against 2.35ns from a release build. Nine times slow, no inlining, and wrong in the way that still looks like a measurement. The module now asks for `release`. `cargo bench` was never affected -- its profile inherits from `release` -- so the numbers reported from it stand. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.lock | 29 +++++++++++++++++++++++++++++ Cargo.toml | 6 +++++- acl/Cargo.toml | 2 +- bench.just | 32 +++++++++++++++++++++++++++++++- default.nix | 1 + justfile | 15 +++++++++++++++ routing/Cargo.toml | 2 +- 7 files changed, 83 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 38457d546c..65513c1fbd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1012,6 +1012,7 @@ dependencies = [ "num-traits", "oorandom", "page_size", + "plotters", "regex", "serde", "serde_json", @@ -4385,6 +4386,34 @@ dependencies = [ "time", ] +[[package]] +name = "plotters" +version = "0.3.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5aeb6f403d7a4911efb1e33402027fc44f29b5bf6def3effcc22d7bb75f2b747" +dependencies = [ + "num-traits", + "plotters-backend", + "plotters-svg", + "wasm-bindgen", + "web-sys", +] + +[[package]] +name = "plotters-backend" +version = "0.3.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "df42e13c12958a16b3f7f4386b9ab1f3e7933914ecea48da7139435263a4172a" + +[[package]] +name = "plotters-svg" +version = "0.3.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "51bae2ac328883f7acdfea3d66a7c35751187f870bc81f94563733a154d7a670" +dependencies = [ + "plotters-backend", +] + [[package]] name = "portable-atomic" version = "1.15.0" diff --git a/Cargo.toml b/Cargo.toml index c9c356dd0f..f0783857a8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -133,7 +133,11 @@ chrono = { version = "0.4.45", default-features = false, features = [] } clap = { version = "4.6.6", default-features = true, features = [] } color-eyre = { version = "0.6.5", default-features = false, features = [] } colored = { version = "3.1.1", default-features = false, features = [] } -criterion = { version = "0.8.2", default-features = false, features = [] } +criterion = { version = "0.8.2", default-features = false, features = [ + "cargo_bench_support", + "html_reports", + "plotters", +] } iai-callgrind = { version = "0.16.1" } crossbeam-utils = { version = "0.8.22", default-features = false, features = [] } dashmap = { version = "6.2.1", default-features = false, features = [] } diff --git a/acl/Cargo.toml b/acl/Cargo.toml index f9d01cb8a9..b4e9ad0f69 100644 --- a/acl/Cargo.toml +++ b/acl/Cargo.toml @@ -27,7 +27,7 @@ clock = { workspace = true, features = ["virtual"] } # differential-test against it, so make it available whenever test/bench targets are built. dataplane-acl = { path = ".", features = ["reference"] } bolero = { workspace = true, features = ["std"] } -criterion = { workspace = true, features = ["cargo_bench_support"] } +criterion = { workspace = true } dpdk = { workspace = true, features = ["test"] } match-action = { workspace = true, features = ["derive", "bolero"] } net = { workspace = true, features = ["test_buffer", "builder"] } diff --git a/bench.just b/bench.just index f2cd57d86e..cc4ab6ab3d 100644 --- a/bench.just +++ b/bench.just @@ -18,6 +18,14 @@ _just_debuggable_ := if debug_justfile == "true" { "set -x" } else { "" } export callgrind_package := "dataplane-routing" export callgrind_bench := "fib_lookup_callgrind" +# The nix profile `criterion` builds with. +# +# Not the root justfile's default of `debug`, which is what this recipe used to inherit: a debug +# build of the fib lookup measures about nine times slower than a release one and optimises +# nothing, so every number it produced was meaningless in a way that still looked like a +# measurement. Benchmark what ships. +profile := "release" + # What a benchmark run can tell you, and what it cannot, is in # `development/code/benchmarking.md`. Read it before drawing a conclusion from one of these. [private] @@ -29,6 +37,11 @@ default: criterion # criterion` to exercise them; a plain run builds them as an empty `main()` and only runs the # reference benches. # +# Writes an html report to `target/criterion/report/index.html`, with the distribution and +# regression plots that the terminal summary flattens into three numbers. Worth opening whenever a +# result is surprising: a bimodal distribution or a visible outlier cluster usually means the +# machine interfered rather than the code changed. +# # Goes through the nix build rather than cargo so the binaries are the ones CI would produce. # # Skips the `*_callgrind` targets, which the same build produces. They are not criterion benches: @@ -38,7 +51,7 @@ default: criterion [script] criterion *args: {{ _just_debuggable_ }} - just build benches {{ args }} + just profile='{{ profile }}' build benches {{ args }} shopt -s nullglob for bench in ./results/benches/bin/*; do case "${bench}" in @@ -46,6 +59,10 @@ criterion *args: esac "${bench}" --bench done + if [ -f target/criterion/report/index.html ]; then + echo + echo "html report: target/criterion/report/index.html" + fi # Run the iai-callgrind benches: instructions retired and modelled cache traffic, rather than # wall-clock. Bit-for-bit repeatable run to run, which is what makes them gateable in CI -- and @@ -76,6 +93,19 @@ compare baseline="base" *args: ./scripts/bench-report.ts results/bench/run.jsonl {{ args }} fi +# Browse the criterion report: distribution and regression plots per benchmark. +# +# Delegates to the root `serve` recipe rather than starting its own server, so there is one place +# that knows how these are served. +[doc("Serve the criterion html report over http")] +[script] +serve port="8080": + {{ _just_debuggable_ }} + # Serves the parent of `report/`, because the top-level index links sideways into each + # benchmark's own directory; serving `report/` alone gives an index whose every link 404s. + echo "criterion index: http://127.0.0.1:{{ port }}/report/index.html" + just serve ./target/criterion '{{ port }}' + # Record a baseline for `compare` to measure against, without reporting. # # CI runs this on the base commit before checking out the head of the branch. diff --git a/default.nix b/default.nix index 2e4a5cc73f..c2a06e9055 100644 --- a/default.nix +++ b/default.nix @@ -189,6 +189,7 @@ let rust-toolchain shellcheck skopeo + static-web-server valgrind wasmtime wget diff --git a/justfile b/justfile index c727ccd3cc..e31ee75f34 100644 --- a/justfile +++ b/justfile @@ -746,6 +746,21 @@ coverage *args: cargo llvm-cov report --branch --lcov --output-path="${out}/lcov.info" cargo llvm-cov report --branch --codecov --output-path="${out}/codecov.json" cargo llvm-cov report --branch --summary-only + echo + echo "html report: ${out}/html/index.html (\`just serve-coverage\` to browse it)" + +[doc("Serve a directory of generated html over http")] +[script] +serve dir port="8080": + {{ _just_debuggable_ }} + if [ ! -d '{{ dir }}' ]; then + echo "error: no such directory: {{ dir }}" >&2 + exit 1 + fi + echo "serving {{ dir }} at http://127.0.0.1:{{ port }} (ctrl-c to stop)" + static-web-server --root '{{ dir }}' --port '{{ port }}' --log-level warn + +serve-coverage port="8080": (serve "./target/nextest/coverage/html" port) [script] duvet *args: diff --git a/routing/Cargo.toml b/routing/Cargo.toml index c85afd8dd7..d7c2376794 100644 --- a/routing/Cargo.toml +++ b/routing/Cargo.toml @@ -59,7 +59,7 @@ netdev = { workspace = true } dataplane-routing = { path = ".", features = ["testing"] } lpm = { workspace = true, features = ["testing"] } clock = { workspace = true, features = ["virtual"] } -criterion = { workspace = true, features = ["cargo_bench_support"] } +criterion = { workspace = true } iai-callgrind = { workspace = true } bolero = { workspace = true, default-features = false } concurrency = { workspace = true } From eb94ee329a0446503734dae9a2f020ba06d5a291 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Sat, 22 Aug 2026 00:06:17 -0600 Subject: [PATCH 09/13] fix(bench): Make criterion arguments a benchmark filter They went to the nix build, where nobody wants them, leaving no way to scope a run. A full sweep is over half an hour -- nearly all of it the rte_acl benches walking fifteen rule counts -- so iterating on one benchmark meant waiting for all of them. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- bench.just | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/bench.just b/bench.just index cc4ab6ab3d..bec8597880 100644 --- a/bench.just +++ b/bench.just @@ -37,6 +37,10 @@ default: criterion # criterion` to exercise them; a plain run builds them as an empty `main()` and only runs the # reference benches. # +# Arguments are a criterion filter, not build arguments: `just bench criterion fib_lpm` runs only +# the benchmarks whose name matches. Worth using -- a full sweep is over half an hour, nearly all +# of it the rte_acl benches walking fifteen rule counts. +# # Writes an html report to `target/criterion/report/index.html`, with the distribution and # regression plots that the terminal summary flattens into three numbers. Worth opening whenever a # result is surprising: a bimodal distribution or a visible outlier cluster usually means the @@ -51,13 +55,13 @@ default: criterion [script] criterion *args: {{ _just_debuggable_ }} - just profile='{{ profile }}' build benches {{ args }} + just profile='{{ profile }}' build benches shopt -s nullglob for bench in ./results/benches/bin/*; do case "${bench}" in *_callgrind) continue ;; esac - "${bench}" --bench + "${bench}" --bench {{ args }} done if [ -f target/criterion/report/index.html ]; then echo From 8f18e21edec2d52c22ea9a87571001862932d8a5 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Sat, 22 Aug 2026 00:23:47 -0600 Subject: [PATCH 10/13] fix(just): Bind the report server to loopback static-web-server defaults to `::`, so `just serve-coverage` was offering the whole source tree to anything that could reach the port. Turned up while checking the html reports serve at all, which they now do end to end: index, per-benchmark pages, and plots. Signed-off-by: Daniel Noland Co-Authored-By: Claude Opus 5 (1M context) --- bench.just | 6 +++--- justfile | 16 +++++++++++++--- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/bench.just b/bench.just index bec8597880..38e6ba07e9 100644 --- a/bench.just +++ b/bench.just @@ -100,15 +100,15 @@ compare baseline="base" *args: # Browse the criterion report: distribution and regression plots per benchmark. # # Delegates to the root `serve` recipe rather than starting its own server, so there is one place -# that knows how these are served. +# that knows how these are served -- including which address to bind, which is why the url is +# printed there and not here. [doc("Serve the criterion html report over http")] [script] serve port="8080": {{ _just_debuggable_ }} # Serves the parent of `report/`, because the top-level index links sideways into each # benchmark's own directory; serving `report/` alone gives an index whose every link 404s. - echo "criterion index: http://127.0.0.1:{{ port }}/report/index.html" - just serve ./target/criterion '{{ port }}' + just serve ./target/criterion '{{ port }}' report/index.html # Record a baseline for `compare` to measure against, without reporting. # diff --git a/justfile b/justfile index e31ee75f34..131cf51c41 100644 --- a/justfile +++ b/justfile @@ -749,16 +749,26 @@ coverage *args: echo echo "html report: ${out}/html/index.html (\`just serve-coverage\` to browse it)" +serve_host := "127.0.0.1" + [doc("Serve a directory of generated html over http")] [script] -serve dir port="8080": +serve dir port="8080" index="index.html": {{ _just_debuggable_ }} if [ ! -d '{{ dir }}' ]; then echo "error: no such directory: {{ dir }}" >&2 exit 1 fi - echo "serving {{ dir }} at http://127.0.0.1:{{ port }} (ctrl-c to stop)" - static-web-server --root '{{ dir }}' --port '{{ port }}' --log-level warn + server="$(command -v static-web-server || true)" + if [ -z "${server}" ] && [ -x ./devroot/bin/static-web-server ]; then + server="$(pwd)/devroot/bin/static-web-server" + fi + if [ -z "${server}" ]; then + echo "error: static-web-server not found; re-enter the dev shell, or \`just setup-roots\`" >&2 + exit 1 + fi + echo "serving {{ dir }} at http://{{ serve_host }}:{{ port }}/{{ index }} (ctrl-c to stop)" + "${server}" --root '{{ dir }}' --host '{{ serve_host }}' --port '{{ port }}' --log-level warn serve-coverage port="8080": (serve "./target/nextest/coverage/html" port) From e5ef39ea11d989d99d9b5c1221b094292d3a65d3 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Thu, 27 Aug 2026 20:01:09 -0600 Subject: [PATCH 11/13] build(deny): Admit the bincode advisory the bench harness brings in `iai-callgrind` serializes its results with `bincode` 1.3.3, so adding the callgrind harness made `check-dependencies` fail on RUSTSEC-2025-0141. There is nothing to upgrade to: the advisory records a team that stopped deliberately and considers 1.3.3 complete. The entry states the invariant that makes it tolerable -- dev-dependency only, absent from every shipped artifact -- so a later reviewer can re-check that rather than re-derive it. Signed-off-by: Daniel Noland --- deny.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/deny.toml b/deny.toml index cc4c4356e6..bd06179b4c 100644 --- a/deny.toml +++ b/deny.toml @@ -25,6 +25,7 @@ ignore = [ "RUSTSEC-2024-0436", # proc-macro-error2 is unmaintained but is needed by multi_index_map_derive, and our own fixin; ignore until both dependencies have migrated away from it. "RUSTSEC-2026-0173", + "RUSTSEC-2025-0141", ] [licenses] From 72b96757b07aa82988708b426fe64b4bf6be2320 Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Thu, 27 Aug 2026 20:01:09 -0600 Subject: [PATCH 12/13] fix(bench): Ask whether this baseline exists, not whether any run has `compare` tested `-d target/iai`, which `just bench callgrind` also creates, and `--baseline=name` exits 0 when `name` is absent rather than failing. Between them the save branch was unreachable once anything had written to `target/iai`: a `compare` against a name that was never recorded reported "recorded a new one" and recorded nothing, so the next run had nothing to compare against either. Signed-off-by: Daniel Noland --- bench.just | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/bench.just b/bench.just index 38e6ba07e9..986c6a8a76 100644 --- a/bench.just +++ b/bench.just @@ -88,7 +88,16 @@ callgrind *args: compare baseline="base" *args: {{ _just_debuggable_ }} mkdir -p results/bench - if [ -d "target/iai" ] && cargo bench -p "${callgrind_package}" --bench "${callgrind_bench}" -- \ + # Ask whether *this* baseline was stored, not whether any run has ever happened. + # + # `-d target/iai` was the old test and it answers the wrong question: `just bench callgrind` + # creates that directory too, and `--baseline=name` exits 0 when `name` is absent -- it just + # reports no comparison. Together those skipped the save branch, so two consecutive `compare` + # runs could both print "recorded a new one" and store nothing. + # + # The name is a file suffix -- `callgrind..out.base@` -- not a directory. + if find target/iai -type f -name '*base@{{ baseline }}*' -print -quit 2>/dev/null | grep -q . \ + && cargo bench -p "${callgrind_package}" --bench "${callgrind_bench}" -- \ --baseline='{{ baseline }}' --output-format=json > results/bench/run.jsonl 2>/dev/null; then ./scripts/bench-report.ts results/bench/run.jsonl {{ args }} else From 71b529f6bca8b8c30eee49db418de863747f266e Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Thu, 27 Aug 2026 20:03:19 -0600 Subject: [PATCH 13/13] fix(just): Give the bench recipes back their build arguments A just module is a closed scope: it does not inherit the root justfile's variables and there is no command-line syntax that sets one. So moving these recipes into `bench.just` and replacing the `(build "benches")` dependency with a `just build benches` subprocess put every build argument out of reach -- `features`, `platform`, `libc`, `kernel`, `default_features`, `instrument`, `jobs`, `cores`. The `just features=dpdk bench criterion` the module's own documentation asked for reached nix as `--argstr features ""`, so the rte_acl benches it names could not be run at all. Back to flat recipes and a real dependency, which shares this invocation's variables. `just bench ` becomes `just bench-`, as it was before, and the module's genuine fixes stay: skipping `*_callgrind` in the criterion loop, the callgrind target variables, the html report, and `bench-baseline`. The module also carried `profile := "release"`, which a flat recipe cannot have without diverging from the global knob every other recipe reads. `bench` refuses any other profile instead of quietly benchmarking a debug build. Signed-off-by: Daniel Noland --- bench.just | 131 ----------------------------------------------------- justfile | 65 +++++++++++++++++++++++++- 2 files changed, 64 insertions(+), 132 deletions(-) delete mode 100644 bench.just diff --git a/bench.just b/bench.just deleted file mode 100644 index 986c6a8a76..0000000000 --- a/bench.just +++ /dev/null @@ -1,131 +0,0 @@ -# SPDX-License-Identifier: Apache-2.0 -# Copyright Open Network Fabric Authors - -set unstable := true -set shell := ["/usr/bin/env", "bash", "-euo", "pipefail", "-c"] -set script-interpreter := ["/usr/bin/env", "bash", "-euo", "pipefail"] - -# enable to debug just recipes -debug_justfile := "false" - -[private] -_just_debuggable_ := if debug_justfile == "true" { "set -x" } else { "" } - -# The crate and bench target the callgrind recipes drive. -# -# One target today. When there are more, this is the knob that decides which of them a report -# covers, and the recipes below take the same shape for each. -export callgrind_package := "dataplane-routing" -export callgrind_bench := "fib_lookup_callgrind" - -# The nix profile `criterion` builds with. -# -# Not the root justfile's default of `debug`, which is what this recipe used to inherit: a debug -# build of the fib lookup measures about nine times slower than a release one and optimises -# nothing, so every number it produced was meaningless in a way that still looked like a -# measurement. Benchmark what ships. -profile := "release" - -# What a benchmark run can tell you, and what it cannot, is in -# `development/code/benchmarking.md`. Read it before drawing a conclusion from one of these. -[private] -default: criterion - -# Build and run the criterion benches: wall-clock time on this machine. -# -# The rte_acl benches are gated behind the `dpdk` feature, so run `just features=dpdk bench -# criterion` to exercise them; a plain run builds them as an empty `main()` and only runs the -# reference benches. -# -# Arguments are a criterion filter, not build arguments: `just bench criterion fib_lpm` runs only -# the benchmarks whose name matches. Worth using -- a full sweep is over half an hour, nearly all -# of it the rte_acl benches walking fifteen rule counts. -# -# Writes an html report to `target/criterion/report/index.html`, with the distribution and -# regression plots that the terminal summary flattens into three numbers. Worth opening whenever a -# result is surprising: a bimodal distribution or a visible outlier cluster usually means the -# machine interfered rather than the code changed. -# -# Goes through the nix build rather than cargo so the binaries are the ones CI would produce. -# -# Skips the `*_callgrind` targets, which the same build produces. They are not criterion benches: -# run through this loop they respawn themselves under valgrind and, worse, overwrite whatever -# baseline `just bench compare` had stored. -[doc("Wall-clock time, via criterion")] -[script] -criterion *args: - {{ _just_debuggable_ }} - just profile='{{ profile }}' build benches - shopt -s nullglob - for bench in ./results/benches/bin/*; do - case "${bench}" in - *_callgrind) continue ;; - esac - "${bench}" --bench {{ args }} - done - if [ -f target/criterion/report/index.html ]; then - echo - echo "html report: target/criterion/report/index.html" - fi - -# Run the iai-callgrind benches: instructions retired and modelled cache traffic, rather than -# wall-clock. Bit-for-bit repeatable run to run, which is what makes them gateable in CI -- and -# blind to anything that changes data layout rather than instruction count. -[doc("Instructions and cache traffic, via iai-callgrind")] -[script] -callgrind *args: - {{ _just_debuggable_ }} - cargo bench -p "${callgrind_package}" --bench "${callgrind_bench}" {{ args }} - -# Compare the callgrind benches against a baseline and print a markdown report. -# -# On its own this records a baseline from the current tree and has nothing to compare against; run -# it once on the base commit and again on your branch. CI does exactly that, both times in one job -# on one runner, which is what makes the comparison meaningful without anything being stored -# between jobs. -[doc("Compare against a baseline and print a markdown report")] -[script] -compare baseline="base" *args: - {{ _just_debuggable_ }} - mkdir -p results/bench - # Ask whether *this* baseline was stored, not whether any run has ever happened. - # - # `-d target/iai` was the old test and it answers the wrong question: `just bench callgrind` - # creates that directory too, and `--baseline=name` exits 0 when `name` is absent -- it just - # reports no comparison. Together those skipped the save branch, so two consecutive `compare` - # runs could both print "recorded a new one" and store nothing. - # - # The name is a file suffix -- `callgrind..out.base@` -- not a directory. - if find target/iai -type f -name '*base@{{ baseline }}*' -print -quit 2>/dev/null | grep -q . \ - && cargo bench -p "${callgrind_package}" --bench "${callgrind_bench}" -- \ - --baseline='{{ baseline }}' --output-format=json > results/bench/run.jsonl 2>/dev/null; then - ./scripts/bench-report.ts results/bench/run.jsonl {{ args }} - else - cargo bench -p "${callgrind_package}" --bench "${callgrind_bench}" -- \ - --save-baseline='{{ baseline }}' --output-format=json > results/bench/run.jsonl - ./scripts/bench-report.ts results/bench/run.jsonl {{ args }} - fi - -# Browse the criterion report: distribution and regression plots per benchmark. -# -# Delegates to the root `serve` recipe rather than starting its own server, so there is one place -# that knows how these are served -- including which address to bind, which is why the url is -# printed there and not here. -[doc("Serve the criterion html report over http")] -[script] -serve port="8080": - {{ _just_debuggable_ }} - # Serves the parent of `report/`, because the top-level index links sideways into each - # benchmark's own directory; serving `report/` alone gives an index whose every link 404s. - just serve ./target/criterion '{{ port }}' report/index.html - -# Record a baseline for `compare` to measure against, without reporting. -# -# CI runs this on the base commit before checking out the head of the branch. -[doc("Record a baseline for `compare`, without reporting")] -[script] -baseline name="base": - {{ _just_debuggable_ }} - cargo bench -p "${callgrind_package}" --bench "${callgrind_bench}" -- \ - --save-baseline='{{ name }}' > /dev/null - echo "recorded baseline '{{ name }}'" diff --git a/justfile b/justfile index 131cf51c41..f3af12acac 100644 --- a/justfile +++ b/justfile @@ -5,7 +5,6 @@ set unstable := true set shell := ["/usr/bin/env", "bash", "-euo", "pipefail", "-c"] set script-interpreter := ["/usr/bin/env", "bash", "-euo", "pipefail"] -mod bench mod ci mod miri @@ -42,6 +41,9 @@ kernel := if platform == "wasm32-wasip1" { "wasip1" } else { "linux" } # cargo build profile (debug/release/fuzz) profile := "debug" +export callgrind_package := "dataplane-routing" +export callgrind_bench := "fib_lookup_callgrind" + # sanitizer to use (address/thread/safe-stack/cfi/"") sanitize := "" @@ -200,6 +202,67 @@ fuzz target time="60s" *args="": {{ if sanitize == "thread" { "--build-std" } else { "" } }} \ {{ _cargo_feature_flags }} {{ args }} +[private] +[script] +_bench-release-only: + {{ _just_debuggable_ }} + if [ '{{ profile }}' != "release" ]; then + echo "error: benchmarks want profile=release, not '{{ profile }}'" >&2 + echo " run: just profile=release bench" >&2 + exit 1 + fi + +[doc("Wall-clock time, via criterion")] +[script] +bench *args: _bench-release-only (build "benches") + {{ _just_debuggable_ }} + shopt -s nullglob + for bench in ./results/benches/bin/*; do + case "${bench}" in + *_callgrind) continue ;; + esac + "${bench}" --bench {{ args }} + done + if [ -f target/criterion/report/index.html ]; then + echo + echo "html report: target/criterion/report/index.html" + fi + +[doc("Instructions and cache traffic, via iai-callgrind")] +[script] +bench-callgrind *args: + {{ _just_debuggable_ }} + cargo bench -p "${callgrind_package}" --bench "${callgrind_bench}" {{ args }} + +[doc("Compare against a baseline and print a markdown report")] +[script] +bench-compare baseline="base" *args: + {{ _just_debuggable_ }} + mkdir -p results/bench + if find target/iai -type f -name '*base@{{ baseline }}*' -print -quit 2>/dev/null | grep -q . \ + && cargo bench -p "${callgrind_package}" --bench "${callgrind_bench}" -- \ + --baseline='{{ baseline }}' --output-format=json > results/bench/run.jsonl 2>/dev/null; then + ./scripts/bench-report.ts results/bench/run.jsonl {{ args }} + else + cargo bench -p "${callgrind_package}" --bench "${callgrind_bench}" -- \ + --save-baseline='{{ baseline }}' --output-format=json > results/bench/run.jsonl + ./scripts/bench-report.ts results/bench/run.jsonl {{ args }} + fi + +[doc("Record a baseline for `bench-compare`, without reporting")] +[script] +bench-baseline name="base": + {{ _just_debuggable_ }} + cargo bench -p "${callgrind_package}" --bench "${callgrind_bench}" -- \ + --save-baseline='{{ name }}' > /dev/null + echo "recorded baseline '{{ name }}'" + +[doc("Serve the criterion html report over http")] +[script] +bench-serve port="8080": + {{ _just_debuggable_ }} + just serve ./target/criterion '{{ port }}' report/index.html + [script] build-each *args: (build "workspace" args) {{ _just_debuggable_ }}