From cb5956fa5e59b3cb1ae7872f3a89059ad10eca12 Mon Sep 17 00:00:00 2001 From: Thomas Korrison Date: Mon, 27 Apr 2026 14:42:36 +0100 Subject: [PATCH] feat: introduce stable workload lookup by ID in benchmark suite - Added a `by_id` method to `WorkloadCase` for safe retrieval of workloads by their stable identifier, enhancing compile-time safety against reordering. - Updated benchmark files to utilize the new method, ensuring that workload selection is resilient to changes in the order of `STANDARD_WORKLOADS`. - Included tests to verify the correctness of workload ID lookups and uniqueness within suites. These changes improve the robustness of benchmark configurations and enhance the clarity of workload management in the library. --- bench-support/src/registry.rs | 50 ++++++++++++++++++++++++++++++++++- benches/reports.rs | 7 ++--- benches/runner.rs | 12 +++++---- benches/workloads.rs | 18 ++++++++----- src/store/weight.rs | 41 ++++++++++++++-------------- 5 files changed, 91 insertions(+), 37 deletions(-) diff --git a/bench-support/src/registry.rs b/bench-support/src/registry.rs index 7baa583..ce69ec6 100644 --- a/bench-support/src/registry.rs +++ b/bench-support/src/registry.rs @@ -479,8 +479,8 @@ pub const EXTENDED_WORKLOADS: &[WorkloadCase] = &[ }, ]; -/// Build a `WorkloadSpec` from a workload case and runtime parameters. impl WorkloadCase { + /// Build a [`WorkloadSpec`] from this case and runtime parameters. pub fn with_params(self, universe: u64, seed: u64) -> WorkloadSpec { WorkloadSpec { universe, @@ -488,6 +488,23 @@ impl WorkloadCase { seed, } } + + /// Look up a workload by its stable `id` in a suite slice. + /// + /// Use this instead of positional indexing into [`STANDARD_WORKLOADS`] or + /// [`EXTENDED_WORKLOADS`]; the order of those constants is not part of the + /// public contract and may change without notice. + /// + /// # Panics + /// + /// Panics if no workload in `suite` has the given `id`. + #[track_caller] + pub fn by_id<'a>(suite: &'a [WorkloadCase], id: &str) -> &'a WorkloadCase { + suite + .iter() + .find(|case| case.id == id) + .unwrap_or_else(|| panic!("workload `{id}` not found in suite")) + } } #[cfg(test)] @@ -551,4 +568,35 @@ mod tests { "duplicate display_name in POLICIES", ); } + + /// Pin the workload ids that benchmarks look up by name so renaming or + /// removing one fails compilation/tests instead of silently changing + /// which workload a report runs against. + #[test] + fn standard_workloads_expose_required_ids() { + for required in ["uniform", "hotset_90_10", "zipfian_1.0"] { + let case = WorkloadCase::by_id(STANDARD_WORKLOADS, required); + assert_eq!(case.id, required); + } + } + + #[test] + #[should_panic(expected = "workload `does_not_exist` not found in suite")] + fn by_id_panics_for_missing_workload() { + let _ = WorkloadCase::by_id(STANDARD_WORKLOADS, "does_not_exist"); + } + + #[test] + fn workload_ids_are_unique_within_each_suite() { + for (suite_name, suite) in [ + ("STANDARD_WORKLOADS", STANDARD_WORKLOADS), + ("EXTENDED_WORKLOADS", EXTENDED_WORKLOADS), + ] { + let mut ids: Vec<&str> = suite.iter().map(|c| c.id).collect(); + let total = ids.len(); + ids.sort_unstable(); + ids.dedup(); + assert_eq!(ids.len(), total, "duplicate workload id in {suite_name}"); + } + } } diff --git a/benches/reports.rs b/benches/reports.rs index c855bb6..b336fb8 100644 --- a/benches/reports.rs +++ b/benches/reports.rs @@ -23,7 +23,7 @@ use common::metrics::{ run_benchmark, }; use common::operation::{ReadThrough, run_operations}; -use common::registry::{EXTENDED_WORKLOADS, STANDARD_WORKLOADS}; +use common::registry::{EXTENDED_WORKLOADS, STANDARD_WORKLOADS, WorkloadCase}; const CAPACITY: usize = 4096; const UNIVERSE: u64 = 16_384; @@ -264,8 +264,9 @@ fn run_comprehensive_comparison() { fn run_detailed_single_benchmark() { println!("\n=== Detailed Benchmark Results ===\n"); - // Use first workload from standard suite (zipfian 1.0) - let workload_case = &STANDARD_WORKLOADS[3]; // zipfian_1.0 + // Look up by stable id so reordering STANDARD_WORKLOADS can't silently + // change which workload this report runs against. + let workload_case = WorkloadCase::by_id(STANDARD_WORKLOADS, "zipfian_1.0"); let spec = workload_case.with_params(UNIVERSE, SEED); let config = BenchmarkConfig { diff --git a/benches/runner.rs b/benches/runner.rs index 21225ac..d4ace3c 100644 --- a/benches/runner.rs +++ b/benches/runner.rs @@ -31,7 +31,7 @@ use common::metrics::{ run_benchmark, }; use common::operation::{ReadThrough, run_operations}; -use common::registry::STANDARD_WORKLOADS; +use common::registry::{STANDARD_WORKLOADS, WorkloadCase}; // Benchmark configuration constants const CAPACITY: usize = 4096; @@ -330,11 +330,13 @@ fn run_adaptation_benchmarks(artifact: &mut BenchmarkArtifact) { fn run_comprehensive_benchmarks(artifact: &mut BenchmarkArtifact) { println!("[4/4] Running comprehensive benchmarks..."); - // Use a subset of workloads for comprehensive benchmarks (they're slower) + // Use a subset of workloads for comprehensive benchmarks (they're slower). + // Look up by stable id so reordering STANDARD_WORKLOADS can't silently + // change which workloads are included in this report. let comprehensive_workloads = [ - &STANDARD_WORKLOADS[0], // uniform - &STANDARD_WORKLOADS[3], // zipfian_1.0 - &STANDARD_WORKLOADS[1], // hotset_90_10 + WorkloadCase::by_id(STANDARD_WORKLOADS, "uniform"), + WorkloadCase::by_id(STANDARD_WORKLOADS, "zipfian_1.0"), + WorkloadCase::by_id(STANDARD_WORKLOADS, "hotset_90_10"), ]; for workload_case in &comprehensive_workloads { diff --git a/benches/workloads.rs b/benches/workloads.rs index acd6816..7d318eb 100644 --- a/benches/workloads.rs +++ b/benches/workloads.rs @@ -31,7 +31,7 @@ use common::metrics::{ }; use common::operation::{ReadThrough, run_operations}; use common::registry::STANDARD_WORKLOADS; -use common::workload::WorkloadSpec; +use common::workload::{Workload, WorkloadGenerator, WorkloadSpec}; use criterion::{BenchmarkId, Criterion, Throughput, criterion_group, criterion_main}; const CAPACITY: usize = 4096; @@ -39,6 +39,15 @@ const UNIVERSE: u64 = 16_384; const OPS: usize = 200_000; const SEED: u64 = 42; +fn make_generator(workload: Workload) -> WorkloadGenerator { + WorkloadSpec { + universe: UNIVERSE, + workload, + seed: SEED, + } + .generator() +} + // ============================================================================ // Hit Rate Benchmarks // ============================================================================ @@ -61,12 +70,7 @@ fn bench_hit_rates(c: &mut Criterion) { let mut total = std::time::Duration::default(); for _ in 0..iters { let mut cache = make_cache(CAPACITY); - let mut generator = WorkloadSpec { - universe: UNIVERSE, - workload: wl, - seed: SEED, - } - .generator(); + let mut generator = make_generator(wl); let mut op_model = ReadThrough::new(1.0, SEED); let start = Instant::now(); let _ = run_operations( diff --git a/src/store/weight.rs b/src/store/weight.rs index 089789c..d3e2710 100644 --- a/src/store/weight.rs +++ b/src/store/weight.rs @@ -1183,9 +1183,8 @@ mod tests { use proptest::prelude::*; - #[allow(clippy::ptr_arg)] - fn weight_by_len(value: &String) -> usize { - value.len() + fn weight_by_len() -> impl Fn(&String) -> usize + Copy + Send + Sync { + |value: &String| value.len() } fn sum_entry_weights(store: &WeightStore) -> usize @@ -1247,7 +1246,7 @@ mod tests { #[test] fn weight_store_tracks_weight() { - let mut store = WeightStore::with_capacity(3, 10, weight_by_len); + let mut store = WeightStore::with_capacity(3, 10, weight_by_len()); assert_eq!(store.total_weight(), 0); assert_eq!(store.try_insert("k1", Arc::new("aa".to_string())), Ok(None)); assert_eq!(store.total_weight(), 2); @@ -1262,7 +1261,7 @@ mod tests { #[test] fn weight_store_enforces_capacity() { - let mut store = WeightStore::with_capacity(10, 5, weight_by_len); + let mut store = WeightStore::with_capacity(10, 5, weight_by_len()); assert_eq!( store.try_insert("k1", Arc::new("aaaaa".to_string())), Ok(None) @@ -1275,7 +1274,7 @@ mod tests { #[test] fn weight_store_update_adjusts_weight() { - let mut store = WeightStore::with_capacity(10, 10, weight_by_len); + let mut store = WeightStore::with_capacity(10, 10, weight_by_len()); assert_eq!(store.try_insert("k1", Arc::new("aa".to_string())), Ok(None)); assert_eq!(store.total_weight(), 2); assert_eq!( @@ -1287,7 +1286,7 @@ mod tests { #[test] fn weight_store_update_rejected_when_new_weight_exceeds_capacity() { - let mut store = WeightStore::with_capacity(4, 5, weight_by_len); + let mut store = WeightStore::with_capacity(4, 5, weight_by_len()); assert_eq!(store.try_insert("k1", Arc::new("aa".to_string())), Ok(None)); assert_eq!(store.try_insert("k2", Arc::new("bb".to_string())), Ok(None)); assert_eq!(store.total_weight(), 4); @@ -1315,7 +1314,7 @@ mod tests { #[test] fn weight_store_remove_missing_does_not_mutate_weight_or_metrics() { - let mut store = WeightStore::with_capacity(4, 20, weight_by_len); + let mut store = WeightStore::with_capacity(4, 20, weight_by_len()); assert_eq!(store.try_insert("k1", Arc::new("aa".to_string())), Ok(None)); let before_weight = store.total_weight(); let before_metrics = store.metrics(); @@ -1327,7 +1326,7 @@ mod tests { #[test] fn weight_store_clear_resets_contents_but_preserves_counters() { - let mut store = WeightStore::with_capacity(4, 20, weight_by_len); + let mut store = WeightStore::with_capacity(4, 20, weight_by_len()); assert_eq!(store.try_insert("k1", Arc::new("aa".to_string())), Ok(None)); assert_eq!( store.try_insert("k2", Arc::new("bbb".to_string())), @@ -1355,7 +1354,7 @@ mod tests { #[test] fn weight_store_api_surface_and_peek_semantics() { - let mut store = WeightStore::with_capacity(3, 9, weight_by_len); + let mut store = WeightStore::with_capacity(3, 9, weight_by_len()); assert_eq!(store.capacity(), 3); assert_eq!(store.capacity_weight(), 9); assert!(store.is_empty()); @@ -1635,7 +1634,7 @@ mod tests { #[cfg(feature = "concurrency")] #[test] fn concurrent_weight_store_basic_ops() { - let store = ConcurrentWeightStore::with_capacity(2, 10, weight_by_len); + let store = ConcurrentWeightStore::with_capacity(2, 10, weight_by_len()); let value = Arc::new("aa".to_string()); assert_eq!(store.try_insert("k1", value.clone()), Ok(None)); assert_eq!(store.get(&"k1"), Some(value.clone())); @@ -1683,7 +1682,7 @@ mod tests { #[cfg(feature = "concurrency")] #[test] fn concurrent_weight_store_failed_ops_do_not_increment_success_counters() { - let store = Arc::new(ConcurrentWeightStore::with_capacity(1, 1, weight_by_len)); + let store = Arc::new(ConcurrentWeightStore::with_capacity(1, 1, weight_by_len())); assert_eq!( store.try_insert("seed".to_string(), Arc::new("x".to_string())), Ok(None) @@ -1760,7 +1759,7 @@ mod tests { #[test] fn concurrent_weight_store_metrics_track_inserts() { let store: ConcurrentWeightStore<&str, String, _> = - ConcurrentWeightStore::with_capacity(100, 100_000, weight_by_len); + ConcurrentWeightStore::with_capacity(100, 100_000, weight_by_len()); store.try_insert("k1", Arc::new("v1".into())).unwrap(); store.try_insert("k2", Arc::new("v2".into())).unwrap(); @@ -1774,7 +1773,7 @@ mod tests { #[test] fn concurrent_weight_store_metrics_track_updates() { let store: ConcurrentWeightStore<&str, String, _> = - ConcurrentWeightStore::with_capacity(100, 100_000, weight_by_len); + ConcurrentWeightStore::with_capacity(100, 100_000, weight_by_len()); store.try_insert("k1", Arc::new("v1".into())).unwrap(); store.try_insert("k1", Arc::new("updated".into())).unwrap(); @@ -1787,7 +1786,7 @@ mod tests { #[test] fn concurrent_weight_store_metrics_track_removes() { let store: ConcurrentWeightStore<&str, String, _> = - ConcurrentWeightStore::with_capacity(100, 100_000, weight_by_len); + ConcurrentWeightStore::with_capacity(100, 100_000, weight_by_len()); store.try_insert("k1", Arc::new("v1".into())).unwrap(); store.remove(&"k1"); @@ -1800,7 +1799,7 @@ mod tests { #[test] fn concurrent_weight_store_metrics_track_hits_misses() { let store: ConcurrentWeightStore<&str, String, _> = - ConcurrentWeightStore::with_capacity(100, 100_000, weight_by_len); + ConcurrentWeightStore::with_capacity(100, 100_000, weight_by_len()); store.try_insert("k1", Arc::new("v1".into())).unwrap(); let _ = store.get(&"k1"); @@ -1818,7 +1817,7 @@ mod tests { #[test] fn weight_store_try_with_capacity_reserves_capacity() { let store: WeightStore<&str, String, _> = - WeightStore::try_with_capacity(16, 1_024, weight_by_len) + WeightStore::try_with_capacity(16, 1_024, weight_by_len()) .expect("try_with_capacity(16, ...) should succeed"); assert_eq!(store.capacity(), 16); assert_eq!(store.capacity_weight(), 1_024); @@ -1846,7 +1845,7 @@ mod tests { // immediately-full store. Matches the semantics of // `with_capacity(0, ...)`. let store: WeightStore<&str, String, _> = - WeightStore::try_with_capacity(0, 1_024, weight_by_len) + WeightStore::try_with_capacity(0, 1_024, weight_by_len()) .expect("try_with_capacity(0, ...) should succeed"); assert_eq!(store.capacity(), 0); assert!(store.is_empty()); @@ -1854,7 +1853,7 @@ mod tests { #[test] fn weight_store_clear_attributes_removes() { - let mut store = WeightStore::with_capacity(4, 1_024, weight_by_len); + let mut store = WeightStore::with_capacity(4, 1_024, weight_by_len()); for i in 0..4u32 { store .try_insert(i, Arc::new(format!("v{i}"))) @@ -1935,7 +1934,7 @@ mod tests { #[test] fn concurrent_weight_store_try_with_capacity_reserves_capacity() { let store: ConcurrentWeightStore<&str, String, _> = - ConcurrentWeightStore::try_with_capacity(16, 1_024, weight_by_len) + ConcurrentWeightStore::try_with_capacity(16, 1_024, weight_by_len()) .expect("try_with_capacity(16, ...) should succeed"); assert_eq!(store.capacity(), 16); assert_eq!(store.capacity_weight(), 1_024); @@ -1958,7 +1957,7 @@ mod tests { #[test] fn concurrent_weight_store_clear_attributes_removes() { let store: ConcurrentWeightStore = - ConcurrentWeightStore::with_capacity(4, 1_024, weight_by_len); + ConcurrentWeightStore::with_capacity(4, 1_024, weight_by_len()); for i in 0..4u32 { store .try_insert(i, Arc::new(format!("v{i}")))