fix(bench): warn on all reset_vm_hwm errors, not just PermissionDenied
Issue #38: warn_reset_hwm() silently swallowed non-permission I/O errors from /proc/self/clear_refs (e.g. missing /proc, read-only procfs, kernel incompatibility). This left users unaware that VmHWM reset failed and memory peak data could be contaminated across suites. Changes: - runner.rs: all errors now produce a warning with specific failure reason; PermissionDenied retains 'try running as root' hint; AtomicBool warn-once prevents duplicate output across 7 suite runs - main.rs: preflight check now uses warn_reset_hwm() instead of the vague can_reset_vm_hwm(), sharing the same warn-once mechanism - metrics.rs: remove dead can_reset_vm_hwm() (no callers remaining) - tests: add hwm_warned_flag_prevents_reentry and warn_reset_hwm_does_not_panic
This commit is contained in:
@@ -59,9 +59,7 @@ fn main() {
|
|||||||
std::process::exit(1);
|
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...");
|
println!("Running benchmarks...");
|
||||||
let results = log_viewer_bench::runner::run_all(&config);
|
let results = log_viewer_bench::runner::run_all(&config);
|
||||||
|
|||||||
@@ -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
|
/// Get file inode number
|
||||||
pub fn get_inode(path: &Path) -> std::io::Result<u64> {
|
pub fn get_inode(path: &Path) -> std::io::Result<u64> {
|
||||||
let meta = fs::metadata(path)?;
|
let meta = fs::metadata(path)?;
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
use crate::metrics::MetricsCollector;
|
use crate::metrics::MetricsCollector;
|
||||||
use crate::types::BenchmarkResult;
|
use crate::types::BenchmarkResult;
|
||||||
use std::path::PathBuf;
|
use std::path::PathBuf;
|
||||||
|
use std::sync::atomic::{AtomicBool, Ordering};
|
||||||
|
|
||||||
/// All recognized benchmark suite identifiers.
|
/// All recognized benchmark suite identifiers.
|
||||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||||
@@ -65,14 +66,35 @@ pub struct BenchConfig {
|
|||||||
pub suites: Option<Vec<Suite>>,
|
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 let Err(e) = MetricsCollector::reset_vm_hwm() {
|
||||||
|
if HWM_WARNED.swap(true, Ordering::Relaxed) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
if e.kind() == std::io::ErrorKind::PermissionDenied {
|
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> {
|
pub fn run_all(config: &BenchConfig) -> Vec<BenchmarkResult> {
|
||||||
let mut results = Vec::new();
|
let mut results = Vec::new();
|
||||||
|
|
||||||
@@ -163,4 +185,37 @@ mod tests {
|
|||||||
let result: Result<Vec<Suite>, _> = names.iter().map(|s| s.parse()).collect();
|
let result: Result<Vec<Suite>, _> = names.iter().map(|s| s.parse()).collect();
|
||||||
assert!(result.is_err());
|
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();
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user