From bcbd370120d1c0bac4768e72a8a166bfdf6e04d8 Mon Sep 17 00:00:00 2001 From: martin Date: Sat, 13 Dec 2025 20:52:13 +0100 Subject: [PATCH] fix: optimizations --- crates/faulx/src/processes.rs | 68 +++++++++++++++++++++++++++----- crates/procedre/src/process.rs | 72 ++++++++++++++++++++++------------ 2 files changed, 104 insertions(+), 36 deletions(-) diff --git a/crates/faulx/src/processes.rs b/crates/faulx/src/processes.rs index 462555b..84dd47f 100644 --- a/crates/faulx/src/processes.rs +++ b/crates/faulx/src/processes.rs @@ -96,16 +96,22 @@ struct Stat { fn check_stat(pid: i32) -> Option { let stat_path = format!("{PROC}/{pid}/stat"); let contents = fs::read_to_string(stat_path).ok()?; + + // Split iterator is more efficient than collecting let mut parts = contents.split_whitespace(); - for _ in 0..4 { - parts.next()?; - } + // Skip first 4 fields (pid, comm, state, ppid) + // Use nth() which is more efficient than multiple next() calls + parts.nth(3)?; + + // 5th field is pgrp let pgrp: i32 = parts.next()?.parse().ok()?; - for _ in 0..16 { - parts.next()?; - } + // Skip next 16 fields to reach starttime (field 22) + // Use nth(15) instead of loop + parts.nth(15)?; + + // Field 22 is starttime let starttime: f64 = parts.next()?.parse().ok()?; Some(Stat { pgrp, starttime }) } @@ -198,7 +204,7 @@ mod tests { fn test_parse_pid_valid() { assert_eq!(parse_pid_from_bytes(b"1"), Some(1)); assert_eq!(parse_pid_from_bytes(b"12345"), Some(12345)); - assert_eq!(parse_pid_from_bytes(b"429496729"), Some(429496729)); + assert_eq!(parse_pid_from_bytes(b"429496729"), Some(429_496_729)); } #[test] @@ -214,7 +220,7 @@ mod tests { .duration_since(UNIX_EPOCH) .unwrap() .as_nanos(); - std::env::temp_dir().join(format!("fake_proc_{}", nanos)) + std::env::temp_dir().join(format!("fake_proc_{nanos}")) } fn setup_fake_proc(tmp: &Path, entries: &[(&str, &str)]) { @@ -224,7 +230,7 @@ mod tests { fs::create_dir_all(&proc_dir).unwrap(); let comm_path = proc_dir.join("comm"); let mut f = File::create(comm_path).unwrap(); - writeln!(f, "{}", comm).unwrap(); + writeln!(f, "{comm}").unwrap(); } } @@ -255,7 +261,49 @@ mod tests { pgrp: 1, starttime: 0.0, }; - assert_eq!(check_time(&dummy_stat, None, None).unwrap(), true); + assert!(check_time(&dummy_stat, None, None).unwrap()); + } + + #[test] + fn test_check_stat_self() { + // Test that check_stat works for the current process + #[allow(clippy::cast_possible_wrap)] + let pid = std::process::id() as i32; + let result = check_stat(pid); + assert!( + result.is_some(), + "check_stat should succeed for current process" + ); + + let stat = result.expect("check_stat should return Some for current process"); + assert!(stat.pgrp > 0, "Process group should be positive"); + assert!(stat.starttime >= 0.0, "Start time should be non-negative"); + } + + #[test] + fn test_check_stat_init() { + // Test that check_stat works for PID 1 (init/systemd) + let result = check_stat(1); + assert!(result.is_some(), "check_stat should succeed for PID 1"); + + let stat = result.expect("check_stat should return Some for PID 1"); + assert!(stat.pgrp > 0, "Process group should be positive"); + assert!(stat.starttime >= 0.0, "Start time should be non-negative"); + } + + #[test] + fn test_list_pids_basic() { + // Test that list_pids can find init/systemd + let opts = OptionsPids { + use_group: false, + younger_than: None, + older_than: None, + ignore_case: false, + }; + + let result = list_pids("systemd", &opts); + // systemd might not exist on all systems, so we just verify the function runs + assert!(result.is_ok(), "list_pids should succeed"); } #[test] diff --git a/crates/procedre/src/process.rs b/crates/procedre/src/process.rs index 19ea6a1..7b18403 100644 --- a/crates/procedre/src/process.rs +++ b/crates/procedre/src/process.rs @@ -63,17 +63,20 @@ pub fn build_process_tree() -> Result, Box> .filter_map(|entry| check_entry(&entry)) .collect(); - let mut tree: HashMap = - pids.into_iter().map(|proc| (proc.pid, proc)).collect(); + // Pre-allocate HashMap with capacity to reduce rehashing + let mut tree: HashMap = HashMap::with_capacity(pids.len()); - let relationships: Vec<(i32, i32)> = { - let iter = tree.values(); + // Collect relationships while building tree to avoid second iteration + let mut relationships: Vec<(i32, i32)> = Vec::with_capacity(pids.len()); - iter.filter(|proc| proc.ppid != 0) - .map(|proc| (proc.ppid, proc.pid)) - .collect() - }; + for proc in pids { + if proc.ppid != 0 { + relationships.push((proc.ppid, proc.pid)); + } + tree.insert(proc.pid, proc); + } + // Build parent-child relationships for (ppid, pid) in relationships { tree.get_mut(&ppid) .ok_or("Failed to get parent process")? @@ -89,35 +92,52 @@ fn check_entry(entry: &fs::DirEntry) -> Option { } fn parse_process(pid: i32) -> Result> { + const REQUIRED_FIELDS: u8 = 7; // Name (1) | State (2) | PPid (4) = 7 + let mut proc = ProcessNode::new(); proc.pid = pid; let status_file = fs::read_to_string(format!("{PROC}/{pid}/status"))?; + let mut found_fields = 0u8; + for line in status_file.lines() { - let mut parts = line.split_whitespace(); - match parts.next() { - Some("Name:") => { - if let Some(name) = parts.next() { + // Optimization: check first char before full string comparison + let Some(&first_char) = line.as_bytes().first() else { + continue; + }; + + match first_char { + b'N' if line.starts_with("Name:") => { + // starts_with ensures we can safely slice after "Name:" + if let Some(name) = line["Name:".len()..].split_whitespace().next() { proc.name = name.to_string(); + found_fields |= 1; } } - Some("PPid:") => { - if let Some(ppid_str) = parts.next() { - proc.ppid = ppid_str.parse::()?; - break; - } - } - Some("State:") => { - if let Some(state_str) = parts.next() { - proc.state = match state_str { - "R" => ProcessState::Running, - "S" => ProcessState::Sleeping, - "Z" => ProcessState::Zombie, - "T" => ProcessState::TracingStop, - "X" => ProcessState::Dead, + b'S' if line.starts_with("State:") => { + // starts_with ensures we can safely slice after "State:" + if let Some(state_str) = line["State:".len()..].split_whitespace().next() { + proc.state = match state_str.as_bytes().first() { + Some(&b'R') => ProcessState::Running, + Some(&b'S') => ProcessState::Sleeping, + Some(&b'Z') => ProcessState::Zombie, + Some(&b'T') => ProcessState::TracingStop, + Some(&b'X') => ProcessState::Dead, _ => ProcessState::Idle, }; + found_fields |= 2; + } + } + b'P' if line.starts_with("PPid:") => { + // starts_with ensures we can safely slice after "PPid:" + if let Some(ppid_str) = line["PPid:".len()..].split_whitespace().next() { + proc.ppid = ppid_str.parse::()?; + found_fields |= 4; + // Early exit if we've found all required fields + if found_fields == REQUIRED_FIELDS { + break; + } } } _ => {}