Merge pull request #1324 from GuillaumeGomez/fix-succession-refresh_cpu

Fix invalid CPU computation when multiple calls to `refresh_process` are done in succession on macOS

src/common/system.rs

@@ -367,6 +367,9 @@ impl System {
     /// exist (it will **NOT** be removed from the processes if it doesn't exist anymore). If it
     /// isn't listed yet, it'll be added.
     ///
+    /// ⚠️ If you need to refresh multiple processes at once, use [`refresh_pids`] instead! It has
+    /// much better performance.
+    ///
     /// It is the same as calling:
     ///
     /// ```no_run
@@ -394,6 +397,8 @@ impl System {
     /// let mut s = System::new_all();
     /// s.refresh_process(Pid::from(1337));
     /// ```
+    ///
+    /// [`refresh_pids`]: #method.refresh_pids
     pub fn refresh_process(&mut self, pid: Pid) -> bool {
         self.refresh_process_specifics(
             pid,
@@ -409,6 +414,9 @@ impl System {
     /// exist (it will **NOT** be removed from the processes if it doesn't exist anymore). If it
     /// isn't listed yet, it'll be added.
     ///
+    /// ⚠️ If you need to refresh multiple processes at once, use [`refresh_pids_specifics`]
+    /// instead! It has much better performance.
+    ///
     /// ⚠️ On Linux, `sysinfo` keeps the `stat` files open by default. You can change this behaviour
     /// by using [`set_open_files_limit`][crate::set_open_files_limit].
     ///
@@ -418,6 +426,8 @@ impl System {
     /// let mut s = System::new_all();
     /// s.refresh_process_specifics(Pid::from(1337), ProcessRefreshKind::new());
     /// ```
+    ///
+    /// [`refresh_pids_specifics`]: #method.refresh_pids_specifics
     pub fn refresh_process_specifics(
         &mut self,
         pid: Pid,

src/unix/apple/macos/system.rs

@@ -8,6 +8,7 @@ use libc::{
     processor_cpu_load_info_t, sysconf, vm_page_size, PROCESSOR_CPU_LOAD_INFO, _SC_CLK_TCK,
 };
 use std::ptr::null_mut;
+use std::time::Instant;
 
 struct ProcessorCpuLoadInfo {
     cpu_load: processor_cpu_load_info_t,
@@ -55,6 +56,8 @@ pub(crate) struct SystemTimeInfo {
     timebase_to_ns: f64,
     clock_per_sec: f64,
     old_cpu_info: ProcessorCpuLoadInfo,
+    last_update: Option<Instant>,
+    prev_time_interval: f64,
 }
 
 unsafe impl Send for SystemTimeInfo {}
@@ -97,40 +100,52 @@ impl SystemTimeInfo {
                 timebase_to_ns: info.numer as f64 / info.denom as f64,
                 clock_per_sec: nano_per_seconds / clock_ticks_per_sec as f64,
                 old_cpu_info,
+                last_update: None,
+                prev_time_interval: 0.,
             })
         }
     }
 
     pub fn get_time_interval(&mut self, port: mach_port_t) -> f64 {
-        let mut total = 0;
-        let new_cpu_info = match ProcessorCpuLoadInfo::new(port) {
-            Some(cpu_info) => cpu_info,
-            None => return 0.,
-        };
-        let cpu_count = std::cmp::min(self.old_cpu_info.cpu_count, new_cpu_info.cpu_count);
-        unsafe {
-            for i in 0..cpu_count {
-                let new_load: &processor_cpu_load_info = &*new_cpu_info.cpu_load.offset(i as _);
-                let old_load: &processor_cpu_load_info =
-                    &*self.old_cpu_info.cpu_load.offset(i as _);
-                for (new, old) in new_load.cpu_ticks.iter().zip(old_load.cpu_ticks.iter()) {
-                    if new > old {
-                        total += new.saturating_sub(*old);
+        let need_cpu_usage_update = self
+            .last_update
+            .map(|last_update| last_update.elapsed() > crate::MINIMUM_CPU_UPDATE_INTERVAL)
+            .unwrap_or(true);
+        if need_cpu_usage_update {
+            let mut total = 0;
+            let new_cpu_info = match ProcessorCpuLoadInfo::new(port) {
+                Some(cpu_info) => cpu_info,
+                None => return 0.,
+            };
+            let cpu_count = std::cmp::min(self.old_cpu_info.cpu_count, new_cpu_info.cpu_count);
+            unsafe {
+                for i in 0..cpu_count {
+                    let new_load: &processor_cpu_load_info = &*new_cpu_info.cpu_load.offset(i as _);
+                    let old_load: &processor_cpu_load_info =
+                        &*self.old_cpu_info.cpu_load.offset(i as _);
+                    for (new, old) in new_load.cpu_ticks.iter().zip(old_load.cpu_ticks.iter()) {
+                        if new > old {
+                            total += new.saturating_sub(*old);
+                        }
                     }
                 }
             }
 
             self.old_cpu_info = new_cpu_info;
+            self.last_update = Some(Instant::now());
 
             // Now we convert the ticks to nanoseconds (if the interval is less than
             // `MINIMUM_CPU_UPDATE_INTERVAL`, we replace it with it instead):
             let base_interval = total as f64 / cpu_count as f64 * self.clock_per_sec;
             let smallest = crate::MINIMUM_CPU_UPDATE_INTERVAL.as_secs_f64() * 1_000_000_000.0;
-            if base_interval < smallest {
+            self.prev_time_interval = if base_interval < smallest {
                 smallest
             } else {
                 base_interval / self.timebase_to_ns
-            }
+            };
+            self.prev_time_interval
+        } else {
+            self.prev_time_interval
         }
     }
 }

tests/process.rs

@@ -813,3 +813,47 @@ fn test_parent_change() {
     // We kill the child to clean up.
     child.kill();
 }
+
+// We want to ensure that if `System::refresh_process*` methods are called
+// one after the other, it won't impact the CPU usage computation badly.
+#[test]
+fn test_multiple_single_process_refresh() {
+    if !sysinfo::IS_SUPPORTED_SYSTEM || cfg!(feature = "apple-sandbox") || cfg!(windows) {
+        // Windows never updates its parent PID so no need to check anything.
+        return;
+    }
+
+    let file_name = "target/test_binary3";
+    build_test_binary(file_name);
+    let mut p_a = std::process::Command::new(format!("./{file_name}"))
+        .arg("1")
+        .spawn()
+        .unwrap();
+    let mut p_b = std::process::Command::new(format!("./{file_name}"))
+        .arg("1")
+        .spawn()
+        .unwrap();
+
+    let pid_a = Pid::from_u32(p_a.id() as _);
+    let pid_b = Pid::from_u32(p_b.id() as _);
+
+    let mut s = System::new();
+    let process_refresh_kind = ProcessRefreshKind::new().with_cpu();
+    s.refresh_process_specifics(pid_a, process_refresh_kind);
+    s.refresh_process_specifics(pid_b, process_refresh_kind);
+
+    std::thread::sleep(std::time::Duration::from_secs(1));
+    s.refresh_process_specifics(pid_a, process_refresh_kind);
+    s.refresh_process_specifics(pid_b, process_refresh_kind);
+
+    let cpu_a = s.process(pid_a).unwrap().cpu_usage();
+    let cpu_b = s.process(pid_b).unwrap().cpu_usage();
+
+    p_a.kill().expect("failed to kill process a");
+    p_b.kill().expect("failed to kill process b");
+
+    let _ = p_a.wait();
+    let _ = p_b.wait();
+
+    assert!(cpu_b - 5. < cpu_a && cpu_b + 5. > cpu_a);
+}