From e4b814fd0b628b8c883b986aeefeec61266472d6 Mon Sep 17 00:00:00 2001 From: Clement Tsang <34804052+ClementTsang@users.noreply.github.com> Date: Sun, 14 Sep 2025 23:47:49 -0400 Subject: [PATCH] refactor: optimize username cloning for Unix processes (#1816) * refactor: rename/update comments for uid_to_username * use arc instead of cloning user repeatedly * clippy * oop * oop2 * hmmm hopefully that works? * ugh --- src/collection/processes.rs | 4 ++-- src/collection/processes/linux/mod.rs | 9 +-------- src/collection/processes/unix/process_ext.rs | 9 +-------- src/collection/processes/unix/user_table.rs | 15 ++++++++++----- src/collection/processes/windows.rs | 3 +-- src/widgets/process_table.rs | 4 ++-- src/widgets/process_table/process_columns.rs | 9 +++++---- src/widgets/process_table/process_data.rs | 17 +++++++++++++---- src/widgets/process_table/query.rs | 5 ++++- 9 files changed, 39 insertions(+), 36 deletions(-) diff --git a/src/collection/processes.rs b/src/collection/processes.rs index 4c3b0196..8a15073a 100644 --- a/src/collection/processes.rs +++ b/src/collection/processes.rs @@ -32,7 +32,7 @@ cfg_if! { } } -use std::{borrow::Cow, time::Duration}; +use std::{sync::Arc, time::Duration}; use super::{DataCollector, error::CollectionResult}; @@ -128,7 +128,7 @@ pub struct ProcessHarvest { pub uid: Option, /// This is the process' user. - pub user: Cow<'static, str>, + pub user: Option>, /// Gpu memory usage as bytes. #[cfg(feature = "gpu")] diff --git a/src/collection/processes/linux/mod.rs b/src/collection/processes/linux/mod.rs index cffdbe38..f8e7bf72 100644 --- a/src/collection/processes/linux/mod.rs +++ b/src/collection/processes/linux/mod.rs @@ -201,14 +201,7 @@ fn read_proc( (0, 0, 0, 0) }; - let user = uid - .and_then(|uid| { - user_table - .get_uid_to_username_mapping(uid) - .map(Into::into) - .ok() - }) - .unwrap_or_else(|| "N/A".into()); + let user = uid.and_then(|uid| user_table.uid_to_username(uid).ok()); let time = if let Ok(ticks_per_sec) = u32::try_from(rustix::param::clock_ticks_per_second()) { if ticks_per_sec == 0 { diff --git a/src/collection/processes/unix/process_ext.rs b/src/collection/processes/unix/process_ext.rs index d303323b..aa9127b5 100644 --- a/src/collection/processes/unix/process_ext.rs +++ b/src/collection/processes/unix/process_ext.rs @@ -88,14 +88,7 @@ pub(crate) trait UnixProcessExt { total_write: disk_usage.total_written_bytes, process_state, uid, - user: uid - .and_then(|uid| { - user_table - .get_uid_to_username_mapping(uid) - .map(Into::into) - .ok() - }) - .unwrap_or_else(|| "N/A".into()), + user: uid.and_then(|uid| user_table.uid_to_username(uid).ok()), time: if process_val.start_time() == 0 { // Workaround for sysinfo occasionally returning a start time equal to UNIX // epoch, giving a run time in the range of 50+ years. We just diff --git a/src/collection/processes/unix/user_table.rs b/src/collection/processes/unix/user_table.rs index 036fe60e..7e6ff29b 100644 --- a/src/collection/processes/unix/user_table.rs +++ b/src/collection/processes/unix/user_table.rs @@ -1,28 +1,33 @@ +use std::sync::Arc; + use hashbrown::HashMap; use crate::collection::error::{CollectionError, CollectionResult}; #[derive(Debug, Default)] pub struct UserTable { - pub uid_user_mapping: HashMap, + pub uid_user_mapping: HashMap>, } impl UserTable { - pub fn get_uid_to_username_mapping(&mut self, uid: libc::uid_t) -> CollectionResult { + /// Get the username associated with a UID. On first access of a name, it will + /// be cached for future accesses. + pub fn uid_to_username(&mut self, uid: libc::uid_t) -> CollectionResult> { if let Some(user) = self.uid_user_mapping.get(&uid) { Ok(user.clone()) } else { - // SAFETY: getpwuid returns a null pointer if no passwd entry is found for the uid. + // SAFETY: getpwuid returns a null pointer if no passwd entry is found for the uid which we check. let passwd = unsafe { libc::getpwuid(uid) }; if passwd.is_null() { Err("passwd is inaccessible".into()) } else { // SAFETY: We return early if passwd is null. - let username = unsafe { std::ffi::CStr::from_ptr((*passwd).pw_name) } + let username: Arc = unsafe { std::ffi::CStr::from_ptr((*passwd).pw_name) } .to_str() .map_err(|err| CollectionError::General(err.into()))? - .to_string(); + .into(); + self.uid_user_mapping.insert(uid, username.clone()); Ok(username) diff --git a/src/collection/processes/windows.rs b/src/collection/processes/windows.rs index 0f9f8d37..e62d6bc7 100644 --- a/src/collection/processes/windows.rs +++ b/src/collection/processes/windows.rs @@ -108,8 +108,7 @@ pub fn sysinfo_process_data( process_state, user: process_val .user_id() - .and_then(|uid| users.get_user_by_id(uid)) - .map_or_else(|| "N/A".into(), |user| user.name().to_owned().into()), + .and_then(|uid| users.get_user_by_id(uid).map(|user| user.name().into())), time: if process_val.start_time() == 0 { // Workaround for sysinfo occasionally returning a start time equal to UNIX // epoch, giving a run time in the range of 50+ years. We just diff --git a/src/widgets/process_table.rs b/src/widgets/process_table.rs index 1b003cc7..bf3a244c 100644 --- a/src/widgets/process_table.rs +++ b/src/widgets/process_table.rs @@ -1188,9 +1188,9 @@ mod test { process_state: "N/A", process_char: '?', #[cfg(target_family = "unix")] - user: "root".to_string(), + user: Some("root".into()), #[cfg(not(target_family = "unix"))] - user: "N/A".to_string(), + user: Some("N/A".into()), num_similar: 0, disabled: false, time: Duration::from_secs(0), diff --git a/src/widgets/process_table/process_columns.rs b/src/widgets/process_table/process_columns.rs index 83ede3ea..97691e41 100644 --- a/src/widgets/process_table/process_columns.rs +++ b/src/widgets/process_table/process_columns.rs @@ -151,16 +151,17 @@ impl SortsRow for ProcColumn { } ProcColumn::State => { if descending { - data.sort_by_cached_key(|pd| Reverse(pd.process_state.to_lowercase())); + data.sort_by_cached_key(|pd| Reverse(pd.process_state)); } else { - data.sort_by_cached_key(|pd| pd.process_state.to_lowercase()); + data.sort_by_cached_key(|pd| pd.process_state); } } ProcColumn::User => { + // FIXME: Is there a better way here to keep the to_lowercase? Usually it shouldn't matter but... if descending { - data.sort_by_cached_key(|pd| Reverse(pd.user.to_lowercase())); + data.sort_by_cached_key(|pd| Reverse(pd.user.clone())); } else { - data.sort_by_cached_key(|pd| pd.user.to_lowercase()); + data.sort_by_cached_key(|pd| pd.user.clone()); } } ProcColumn::Time => { diff --git a/src/widgets/process_table/process_data.rs b/src/widgets/process_table/process_data.rs index b200a3ea..f1e27585 100644 --- a/src/widgets/process_table/process_data.rs +++ b/src/widgets/process_table/process_data.rs @@ -3,6 +3,7 @@ use std::{ cmp::{Ordering, max}, fmt::Display, num::NonZeroU16, + sync::Arc, time::Duration, }; @@ -206,7 +207,7 @@ pub struct ProcWidgetData { pub total_write: u64, pub process_state: &'static str, pub process_char: char, - pub user: String, + pub user: Option>, pub num_similar: u64, pub disabled: bool, pub time: Duration, @@ -249,7 +250,7 @@ impl ProcWidgetData { total_write: process.total_write, process_state: process.process_state.0, process_char: process.process_state.1, - user: process.user.to_string(), + user: process.user.clone(), num_similar: 1, disabled: false, time: process.time, @@ -318,7 +319,11 @@ impl ProcWidgetData { ProcColumn::TotalRead => dec_bytes_string(self.total_read), ProcColumn::TotalWrite => dec_bytes_string(self.total_write), ProcColumn::State => self.process_char.to_string(), - ProcColumn::User => self.user.clone(), + ProcColumn::User => self + .user + .as_ref() + .map(|user| user.to_string()) + .unwrap_or_else(|| "N/A".to_string()), ProcColumn::Time => format_time(self.time), #[cfg(feature = "gpu")] ProcColumn::GpuMemValue | ProcColumn::GpuMemPercent => self.gpu_mem_usage.to_string(), @@ -355,7 +360,11 @@ impl DataToCell for ProcWidgetData { self.process_state.into() } } - ProcColumn::User => self.user.clone().into(), + ProcColumn::User => self + .user + .as_ref() + .map(|user| user.to_string().into()) + .unwrap_or_else(|| "N/A".into()), ProcColumn::Time => format_time(self.time).into(), #[cfg(feature = "gpu")] ProcColumn::GpuMemValue | ProcColumn::GpuMemPercent => { diff --git a/src/widgets/process_table/query.rs b/src/widgets/process_table/query.rs index 01fe6e57..3f408af0 100644 --- a/src/widgets/process_table/query.rs +++ b/src/widgets/process_table/query.rs @@ -815,7 +815,10 @@ impl Prefix { }), PrefixType::Pid => r.is_match(process.pid.to_string().as_str()), PrefixType::State => r.is_match(process.process_state.0), - PrefixType::User => r.is_match(process.user.as_ref()), + PrefixType::User => match process.user.as_ref() { + Some(user) => r.is_match(user), + None => r.is_match("N/A"), + }, _ => true, } } else {