fix(bench): [M23] prevent single_frame_tail/head overlap for small files

- Extract shared FRAME_LINES constant into suites/mod.rs
- Add select_frame_positions() helper with 3*FRAME_LINES threshold
  to guarantee non-overlapping head/middle/tail ranges
- Guard bench_reverse_scan against total <= FRAME_LINES
- Add 9 boundary tests for position selection (0..1M lines)

Closes #42

crates/bench/Cargo.toml

@@ -3,6 +3,10 @@ name = "log-viewer-bench"
 version = "0.1.0"
 edition = "2024"
 
+[lib]
+name = "log_viewer_bench"
+path = "src/lib.rs"
+
 [[bin]]
 name = "log-viewer-bench"
 path = "src/main.rs"

crates/bench/src/data_gen.rs

@@ -21,15 +21,7 @@ pub fn ensure_test_file(path: &Path) -> std::io::Result<TestFileInfo> {
 fn get_file_info(path: &Path) -> std::io::Result<TestFileInfo> {
     let metadata = fs::metadata(path)?;
     let size_bytes = metadata.len();
-
+    let line_count = count_existing_lines(path)?;
-    let file = fs::File::open(path)?;
-    let mut reader = BufReader::new(file);
-    let mut line_count: u64 = 0;
-    let mut buf = Vec::new();
-    while reader.read_until(b'\n', &mut buf)? > 0 {
-        line_count += 1;
-        buf.clear();
-    }
 
     let avg_line_length = if line_count > 0 {
         size_bytes as f64 / line_count as f64

crates/bench/src/main.rs

@@ -25,6 +25,21 @@ struct Args {
 fn main() {
     let args = Args::parse();
 
+    let suites = match args.suites {
+        Some(names) => {
+            let parsed: Result<Vec<_>, _> =
+                names.iter().map(|s| s.parse::<log_viewer_bench::runner::Suite>()).collect();
+            match parsed {
+                Ok(s) => Some(s),
+                Err(e) => {
+                    eprintln!("error: {e}");
+                    std::process::exit(1);
+                }
+            }
+        }
+        None => None,
+    };
+
     println!("=== Benchmark: mmap vs pread ===");
     println!("Test file: {}", args.test_file.display());
     println!("Quick mode: {}", args.quick);
@@ -33,7 +48,7 @@ fn main() {
     let config = log_viewer_bench::runner::BenchConfig {
         test_file: args.test_file.clone(),
         quick_mode: args.quick,
-        suites: args.suites,
+        suites,
     };
 
     if !config.test_file.exists() {
@@ -44,9 +59,7 @@ fn main() {
         std::process::exit(1);
     }
 
-    if !log_viewer_bench::metrics::MetricsCollector::can_reset_vm_hwm() {
+    log_viewer_bench::runner::warn_reset_hwm();
-        eprintln!("WARNING: VmHWM reset unavailable (no root). Memory peak values may be contaminated across tests.");
-    }
 
     println!("Running benchmarks...");
     let results = log_viewer_bench::runner::run_all(&config);

crates/bench/src/metrics.rs

@@ -80,14 +80,6 @@ impl MetricsCollector {
         })
     }
 
-    /// Check if we can reset VmHWM (i.e., can open `/proc/self/clear_refs` for writing)
-    pub fn can_reset_vm_hwm() -> bool {
-        std::fs::OpenOptions::new()
-            .write(true)
-            .open("/proc/self/clear_refs")
-            .is_ok()
-    }
-
     /// Get file inode number
     pub fn get_inode(path: &Path) -> std::io::Result<u64> {
         let meta = fs::metadata(path)?;

crates/bench/src/report.rs

@@ -43,14 +43,14 @@ pub fn format_report(results: &[BenchmarkResult]) -> String {
 
         let mut variants: Vec<(String, String)> = Vec::new();
         for r in category_results {
-            let key = format!("{} ({})", r.backend, r.variant);
             if !variants
                 .iter()
-                .any(|(b, v)| format!("{} ({})", b, v) == key)
+                .any(|(b, v)| b == &r.backend && v == &r.variant)
             {
                 variants.push((r.backend.clone(), r.variant.clone()));
             }
         }
+        variants.sort();
 
         report.push_str("### Latency\n\n");
         report.push_str("| Test |");
@@ -143,7 +143,13 @@ pub fn format_report(results: &[BenchmarkResult]) -> String {
             report.push_str("| Test | Variant | RSS | Peak RSS | Page Faults |\n");
             report.push_str("|------|---------|-----|----------|-------------|\n");
 
-            for r in category_results {
+            let mut mem_rows: Vec<&BenchmarkResult> =
+                category_results.iter().copied().collect();
+            mem_rows.sort_by(|a, b| {
+                (&a.test_name, &a.backend, &a.variant)
+                    .cmp(&(&b.test_name, &b.backend, &b.variant))
+            });
+            for r in mem_rows {
                 let variant_label = format!("{} ({})", r.backend, r.variant);
                 report.push_str(&format!(
                     "| {} | {} | {} | {} | {} |\n",
@@ -157,7 +163,7 @@ pub fn format_report(results: &[BenchmarkResult]) -> String {
             report.push('\n');
         }
 
-        let extras: Vec<(String, String, Vec<(String, f64)>)> = category_results
+        let mut extras: Vec<(String, String, Vec<(String, f64)>)> = category_results
             .iter()
             .filter(|r| !r.extra.is_empty())
             .map(|r| {
@@ -171,6 +177,7 @@ pub fn format_report(results: &[BenchmarkResult]) -> String {
                 )
             })
             .collect();
+        extras.sort_by(|a, b| (&a.0, &a.1).cmp(&(&b.0, &b.1)));
 
         if !extras.is_empty() {
             report.push_str("### Extra Metrics\n\n");
@@ -205,3 +212,50 @@ fn capitalize(s: &str) -> String {
         Some(f) => f.to_uppercase().chain(c).collect(),
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::collections::HashMap;
+
+    fn make_result(
+        category: &str,
+        test_name: &str,
+        backend: &str,
+        variant: &str,
+        latency_us: Vec<u64>,
+    ) -> BenchmarkResult {
+        BenchmarkResult {
+            category: category.to_string(),
+            test_name: test_name.to_string(),
+            backend: backend.to_string(),
+            variant: variant.to_string(),
+            latency_us,
+            rss_kb: 0,
+            rss_peak_kb: 0,
+            page_faults: 0,
+            extra: HashMap::new(),
+        }
+    }
+
+    #[test]
+    fn report_ordering_independent_of_input_order() {
+        let set_a = vec![
+            make_result("sequential", "read_1mb", "pread", "default", vec![100, 110, 105]),
+            make_result("sequential", "read_1mb", "mmap", "default", vec![80, 85, 90]),
+            make_result("sequential", "read_4kb", "pread", "default", vec![10, 12, 11]),
+            make_result("sequential", "read_4kb", "mmap", "default", vec![8, 9, 7]),
+        ];
+
+        let set_b = vec![
+            make_result("sequential", "read_4kb", "mmap", "default", vec![8, 9, 7]),
+            make_result("sequential", "read_1mb", "mmap", "default", vec![80, 85, 90]),
+            make_result("sequential", "read_4kb", "pread", "default", vec![10, 12, 11]),
+            make_result("sequential", "read_1mb", "pread", "default", vec![100, 110, 105]),
+        ];
+
+        let report_a = format_report(&set_a);
+        let report_b = format_report(&set_b);
+        assert_eq!(report_a, report_b, "Reports must be identical regardless of input order");
+    }
+}

crates/bench/src/runner.rs

@@ -1,59 +1,221 @@
 use crate::metrics::MetricsCollector;
 use crate::types::BenchmarkResult;
 use std::path::PathBuf;
+use std::sync::atomic::{AtomicBool, Ordering};
+
+/// All recognized benchmark suite identifiers.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum Suite {
+    Startup,
+    Render,
+    Jump,
+    Memory,
+    Growth,
+    Rotation,
+    Concurrent,
+}
+
+impl Suite {
+    /// All valid suite identifiers, in execution order.
+    pub const ALL: &[Suite] = &[
+        Suite::Startup,
+        Suite::Render,
+        Suite::Jump,
+        Suite::Memory,
+        Suite::Growth,
+        Suite::Rotation,
+        Suite::Concurrent,
+    ];
+
+    #[must_use]
+    pub const fn as_str(self) -> &'static str {
+        match self {
+            Suite::Startup => "startup",
+            Suite::Render => "render",
+            Suite::Jump => "jump",
+            Suite::Memory => "memory",
+            Suite::Growth => "growth",
+            Suite::Rotation => "rotation",
+            Suite::Concurrent => "concurrent",
+        }
+    }
+}
+
+impl std::str::FromStr for Suite {
+    type Err = String;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        Suite::ALL
+            .iter()
+            .find(|suite| suite.as_str() == s)
+            .copied()
+            .ok_or_else(|| {
+                let valid = Suite::ALL
+                    .iter()
+                    .map(|s| s.as_str())
+                    .collect::<Vec<_>>()
+                    .join(", ");
+                format!("invalid value '{s}' for '--suites': valid values are {valid}")
+            })
+    }
+}
 
 pub struct BenchConfig {
     pub test_file: PathBuf,
     pub quick_mode: bool,
-    pub suites: Option<Vec<String>>,
+    pub suites: Option<Vec<Suite>>,
 }
 
-fn warn_reset_hwm() {
+/// Track whether we have already warned about VmHWM reset failure.
+/// Prevents duplicate warnings across multiple suite runs.
+static HWM_WARNED: AtomicBool = AtomicBool::new(false);
+
+pub fn warn_reset_hwm() {
     if let Err(e) = MetricsCollector::reset_vm_hwm() {
+        if HWM_WARNED.swap(true, Ordering::Relaxed) {
+            return;
+        }
         if e.kind() == std::io::ErrorKind::PermissionDenied {
-            eprintln!("WARNING: VmHWM reset requires root: {e}");
+            eprintln!(
+                "WARNING: Failed to reset VmHWM via /proc/self/clear_refs: {e}. \
+                 Memory peak values may be contaminated across benchmark suites. \
+                 Try running as root."
+            );
+        } else {
+            eprintln!(
+                "WARNING: Failed to reset VmHWM via /proc/self/clear_refs: {e}. \
+                 Memory peak values may be contaminated across benchmark suites."
+            );
         }
     }
 }
 
+#[cfg(test)]
+fn reset_hwm_warned() {
+    HWM_WARNED.store(false, Ordering::Relaxed);
+}
+
 pub fn run_all(config: &BenchConfig) -> Vec<BenchmarkResult> {
     let mut results = Vec::new();
 
-    let should_run = |name: &str| -> bool {
+    let should_run = |suite: Suite| -> bool {
         match &config.suites {
-            Some(suites) => suites.iter().any(|s| s == name),
+            Some(suites) => suites.contains(&suite),
             None => true,
         }
     };
 
-    if should_run("startup") {
+    if should_run(Suite::Startup) {
         warn_reset_hwm();
         results.extend(crate::suites::startup::run(config));
     }
-    if should_run("render") {
+    if should_run(Suite::Render) {
         warn_reset_hwm();
         results.extend(crate::suites::render::run(config));
     }
-    if should_run("jump") {
+    if should_run(Suite::Jump) {
         warn_reset_hwm();
         results.extend(crate::suites::jump::run(config));
     }
-    if should_run("memory") {
+    if should_run(Suite::Memory) {
         warn_reset_hwm();
         results.extend(crate::suites::memory::run(config));
     }
-    if should_run("growth") {
+    if should_run(Suite::Growth) {
         warn_reset_hwm();
         results.extend(crate::suites::growth::run(config));
     }
-    if should_run("rotation") {
+    if should_run(Suite::Rotation) {
         warn_reset_hwm();
         results.extend(crate::suites::rotation::run(config));
     }
-    if should_run("concurrent") {
+    if should_run(Suite::Concurrent) {
         warn_reset_hwm();
         results.extend(crate::suites::concurrent::run(config));
     }
 
     results
 }
+
+#[cfg(test)]
+mod tests {
+    use super::Suite;
+    use std::str::FromStr;
+
+    #[test]
+    fn parse_all_valid_suites() {
+        let expected = [
+            ("startup", Suite::Startup),
+            ("render", Suite::Render),
+            ("jump", Suite::Jump),
+            ("memory", Suite::Memory),
+            ("growth", Suite::Growth),
+            ("rotation", Suite::Rotation),
+            ("concurrent", Suite::Concurrent),
+        ];
+        for (s, expected_suite) in expected {
+            assert_eq!(Suite::from_str(s).unwrap(), expected_suite, "failed to parse '{s}'");
+        }
+    }
+
+    #[test]
+    fn misspelled_suite_returns_error() {
+        let err = Suite::from_str("startp").unwrap_err();
+        assert!(
+            err.contains("invalid value 'startp'"),
+            "error should mention the bad value: {err}"
+        );
+    }
+
+    #[test]
+    fn error_message_lists_all_valid_values() {
+        let err = Suite::from_str("bogus").unwrap_err();
+        for name in Suite::ALL.iter().map(|s| s.as_str()) {
+            assert!(
+                err.contains(name),
+                "error should list valid suite '{name}': {err}"
+            );
+        }
+    }
+
+    #[test]
+    fn mixed_valid_invalid_stops_at_first_error() {
+        // "startup" is valid, "zzz" is not — collect hits the first Err
+        let names = ["startup".to_string(), "zzz".to_string()];
+        let result: Result<Vec<Suite>, _> = names.iter().map(|s| s.parse()).collect();
+        assert!(result.is_err());
+    }
+
+    #[test]
+    fn hwm_warned_flag_prevents_reentry() {
+        use super::reset_hwm_warned;
+        use std::sync::atomic::Ordering;
+
+        reset_hwm_warned();
+        assert!(
+            !super::HWM_WARNED.load(Ordering::Relaxed),
+            "flag should be false after reset"
+        );
+
+        // Simulate the flag being set (as warn_reset_hwm would do on error)
+        super::HWM_WARNED.store(true, Ordering::Relaxed);
+
+        // swap should now return true (old value), indicating already warned
+        assert!(
+            super::HWM_WARNED.swap(true, Ordering::Relaxed),
+            "swap should return the previous value (true)"
+        );
+    }
+
+    #[test]
+    fn warn_reset_hwm_does_not_panic() {
+        use super::reset_hwm_warned;
+
+        reset_hwm_warned();
+        // Whether reset_vm_hwm succeeds or fails, warn_reset_hwm must not panic.
+        // Multiple calls must also be safe.
+        super::warn_reset_hwm();
+        super::warn_reset_hwm();
+        super::warn_reset_hwm();
+    }
+}

crates/bench/src/suites/jump.rs

@@ -1,5 +1,6 @@
 use std::collections::HashMap;
 
+use super::FRAME_LINES;
 use crate::metrics::MetricsCollector;
 use crate::mmap_reader::{
     MmapReaderPhaseAware, MmapReaderPlain, MmapReaderPopulate, MmapReaderRandom,
@@ -210,13 +211,17 @@ fn bench_reverse_scan<B: FileReaderBackend>(
     let path = &config.test_file;
     let reader = B::open(path).expect("Failed to open file");
     let total = reader.total_lines();
-    let iterations: usize = if config.quick_mode { 5 } else { 10 };
 
-    let mut latencies = Vec::with_capacity(iterations * 35);
+    if total <= FRAME_LINES {
+        reader.close();
+        return vec![];
+    }
+
+    let iterations: usize = if config.quick_mode { 5 } else { 10 };
+    let mut latencies = Vec::with_capacity(iterations * FRAME_LINES);
 
     for _ in 0..iterations {
-        // Read lines backwards from end: last line, last-1, ..., last-34
+        let start = total - FRAME_LINES;
-        let start = total.saturating_sub(35);
         for i in (start..total).rev() {
             let t = std::time::Instant::now();
             let _ = reader.get_line(i);
@@ -229,7 +234,7 @@ fn bench_reverse_scan<B: FileReaderBackend>(
     reader.close();
 
     let mut extra = HashMap::new();
-    extra.insert("lines_per_scan".into(), 35.0);
+    extra.insert("lines_per_scan".into(), FRAME_LINES as f64);
     extra.insert("iterations".into(), iterations as f64);
 
     vec![BenchmarkResult {

crates/bench/src/suites/mod.rs

@@ -5,3 +5,8 @@ pub mod memory;
 pub mod render;
 pub mod rotation;
 pub mod startup;
+
+/// Number of lines read per single-frame render benchmark.
+/// Also used as the scan window size for reverse-scan benchmarks.
+/// Shared across suites to ensure consistent workload sizing.
+pub(crate) const FRAME_LINES: usize = 35;

crates/bench/src/suites/render.rs

@@ -1,5 +1,6 @@
 use std::collections::HashMap;
 
+use super::FRAME_LINES;
 use crate::metrics::MetricsCollector;
 use crate::mmap_reader::{
     MmapReaderPhaseAware, MmapReaderPlain, MmapReaderPopulate, MmapReaderRandom,
@@ -88,15 +89,9 @@ fn bench_single_frame<B: FileReaderBackend>(
     let total = reader.total_lines();
     let mut results = Vec::new();
 
-    let positions = [
+    for (pos_name, start_line) in select_frame_positions(total) {
-        ("head", 0),
+        let mut latencies = Vec::with_capacity(FRAME_LINES);
-        ("middle", total / 2),
+        for i in 0..FRAME_LINES {
-        ("tail", total.saturating_sub(35)),
-    ];
-
-    for (pos_name, start_line) in positions {
-        let mut latencies = Vec::with_capacity(35);
-        for i in 0..35 {
             let line_idx = start_line + i;
             if line_idx >= total {
                 break;
@@ -126,6 +121,33 @@ fn bench_single_frame<B: FileReaderBackend>(
     results
 }
 
+/// Select non-overlapping benchmark positions for single-frame tests.
+///
+/// Returns `(label, start_line)` pairs. Positions are chosen so that no
+/// two frames overlap, skipping any position that cannot fit without
+/// colliding with a previously selected range.
+pub(crate) fn select_frame_positions(total: usize) -> Vec<(&'static str, usize)> {
+    let frame = FRAME_LINES;
+    if total == 0 {
+        return vec![];
+    }
+
+    let mut positions: Vec<(&'static str, usize)> = vec![("head", 0)];
+
+    // Need at least 3 full frames to place head, middle, and tail without overlap.
+    if total >= 3 * frame {
+        let head_end = frame;
+        let tail_start = total - frame;
+        let gap = tail_start - head_end;
+        // Center the middle frame within the gap, ensuring it fits entirely
+        let middle_start = head_end + gap.saturating_sub(frame) / 2;
+        positions.push(("middle", middle_start));
+        positions.push(("tail", tail_start));
+    }
+
+    positions
+}
+
 fn bench_continuous_scroll<B: FileReaderBackend>(
     backend: &str,
     variant: &str,
@@ -175,3 +197,119 @@ fn bench_continuous_scroll<B: FileReaderBackend>(
         extra,
     }]
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn starts(pos: &[(&str, usize)]) -> Vec<usize> {
+        pos.iter().map(|&(_, s)| s).collect()
+    }
+
+    fn labels<'a>(pos: &[(&'a str, usize)]) -> Vec<&'a str> {
+        pos.iter().map(|&(l, _)| l).collect()
+    }
+
+    fn ranges_overlap(a_start: usize, b_start: usize) -> bool {
+        let a_end = a_start + FRAME_LINES;
+        let b_end = b_start + FRAME_LINES;
+        a_start < b_end && b_start < a_end
+    }
+
+    #[test]
+    fn zero_lines_no_positions() {
+        assert!(select_frame_positions(0).is_empty());
+    }
+
+    #[test]
+    fn one_line_head_only() {
+        let pos = select_frame_positions(1);
+        assert_eq!(labels(&pos), vec!["head"]);
+        assert_eq!(starts(&pos), vec![0]);
+    }
+
+    #[test]
+    fn at_frame_lines_head_only() {
+        let pos = select_frame_positions(FRAME_LINES);
+        assert_eq!(labels(&pos), vec!["head"]);
+        assert_eq!(starts(&pos), vec![0]);
+    }
+
+    #[test]
+    fn just_above_threshold_head_middle_tail() {
+        let total = 3 * FRAME_LINES;
+        let pos = select_frame_positions(total);
+        assert_eq!(labels(&pos), vec!["head", "middle", "tail"]);
+        assert_eq!(starts(&pos)[0], 0);
+        assert_eq!(*starts(&pos).last().unwrap(), total - FRAME_LINES);
+        // Verify no overlap
+        for i in 0..pos.len() {
+            for j in (i + 1)..pos.len() {
+                assert!(
+                    !ranges_overlap(pos[i].1, pos[j].1),
+                    "overlap: {:?} @ {} vs {:?} @ {} (total={})",
+                    pos[i].0, pos[i].1, pos[j].0, pos[j].1, total
+                );
+            }
+        }
+    }
+
+    #[test]
+    fn no_overlap_at_total_70() {
+        // total=70 < 3*35=105, so only head is returned
+        let pos = select_frame_positions(70);
+        assert_eq!(labels(&pos), vec!["head"]);
+    }
+
+    #[test]
+    fn no_overlap_at_total_104() {
+        let pos = select_frame_positions(104);
+        for i in 0..pos.len() {
+            for j in (i + 1)..pos.len() {
+                assert!(
+                    !ranges_overlap(pos[i].1, pos[j].1),
+                    "overlap at total=104: {:?} @ {} vs {:?} @ {}",
+                    pos[i].0, pos[i].1, pos[j].0, pos[j].1
+                );
+            }
+        }
+    }
+
+    #[test]
+    fn no_overlap_at_total_105() {
+        let pos = select_frame_positions(105);
+        for i in 0..pos.len() {
+            for j in (i + 1)..pos.len() {
+                assert!(
+                    !ranges_overlap(pos[i].1, pos[j].1),
+                    "overlap at total=105: {:?} @ {} vs {:?} @ {}",
+                    pos[i].0, pos[i].1, pos[j].0, pos[j].1
+                );
+            }
+        }
+    }
+
+    #[test]
+    fn middle_is_centered_between_head_and_tail() {
+        let total = 1000;
+        let pos = select_frame_positions(total);
+        assert_eq!(pos.len(), 3);
+
+        let (_, head_start) = pos[0];
+        let (_, middle_start) = pos[1];
+        let (_, tail_start) = pos[2];
+
+        assert_eq!(head_start, 0);
+        assert_eq!(tail_start, total - FRAME_LINES);
+
+        let head_end = head_start + FRAME_LINES;
+        let gap = tail_start - head_end;
+        assert_eq!(middle_start, head_end + gap.saturating_sub(FRAME_LINES) / 2);
+    }
+
+    #[test]
+    fn large_file_all_three_positions() {
+        let pos = select_frame_positions(1_000_000);
+        assert_eq!(labels(&pos), vec!["head", "middle", "tail"]);
+    }
+}

crates/bench/src/suites/startup.rs

@@ -40,7 +40,8 @@ pub fn run(config: &BenchConfig) -> Vec<BenchmarkResult> {
     ));
 
     if !config.quick_mode {
-        if MetricsCollector::clear_file_cache(&config.test_file).is_ok() {
+        match MetricsCollector::clear_file_cache(&config.test_file) {
+            Ok(()) => {
                 results.extend(bench_cold_open::<MmapReaderPlain>("mmap", "plain", config));
                 results.extend(bench_cold_open::<MmapReaderSequential>(
                     "mmap",
@@ -70,6 +71,12 @@ pub fn run(config: &BenchConfig) -> Vec<BenchmarkResult> {
                     config,
                 ));
             }
+            Err(e) => {
+                eprintln!(
+                    "WARNING: Failed to clear file cache; skipping cold startup benchmarks because results would not be cold: {e}"
+                );
+            }
+        }
     }
 
     results
@@ -88,8 +95,15 @@ fn bench_cold_open<B: FileReaderBackend>(
     variant: &str,
     config: &BenchConfig,
 ) -> Vec<BenchmarkResult> {
-    let _ = MetricsCollector::clear_file_cache(&config.test_file);
+    match MetricsCollector::clear_file_cache(&config.test_file) {
-    open_and_measure::<B>(backend, variant, &config.test_file, "cold_open")
+        Ok(()) => open_and_measure::<B>(backend, variant, &config.test_file, "cold_open"),
+        Err(e) => {
+            eprintln!(
+                "WARNING: Failed to clear file cache for {backend}/{variant}; skipping cold benchmark: {e}"
+            );
+            Vec::new()
+        }
+    }
 }
 
 fn open_and_measure<B: FileReaderBackend>(