From 0ce464dc062a589768bde6a3252ef151e301f093 Mon Sep 17 00:00:00 2001 From: mattsu Date: Fri, 31 Oct 2025 20:47:42 +0900 Subject: [PATCH 1/4] feat(timeout): add benchmarking support and optimize signal handling - Add dev-dependencies for divan and uucore with benchmark features to Cargo.toml - Include a new benchmark harness for timeout_bench - Optimize signal lookups using OnceLock for KILL and CONT signals to avoid repeated computations - Introduce preserved_child_exit function for accurate exit status handling - Update send_signal and wait_or_kill_process for improved efficiency and correctness - Update Cargo.lock with new dependency codspeed-divan-compat This enhances performance testing capabilities and reduces overhead in signal operations for the timeout utility. --- Cargo.lock | 1 + src/uu/timeout/Cargo.toml | 8 ++ src/uu/timeout/benches/timeout_bench.rs | 121 ++++++++++++++++++++++++ src/uu/timeout/src/timeout.rs | 75 ++++++++++----- src/uucore/src/lib/features/process.rs | 25 +++-- 5 files changed, 201 insertions(+), 29 deletions(-) create mode 100644 src/uu/timeout/benches/timeout_bench.rs diff --git a/Cargo.lock b/Cargo.lock index ad6f228d69c..168d508e6a6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4114,6 +4114,7 @@ name = "uu_timeout" version = "0.3.0" dependencies = [ "clap", + "codspeed-divan-compat", "fluent", "libc", "nix", diff --git a/src/uu/timeout/Cargo.toml b/src/uu/timeout/Cargo.toml index c6b795628f7..cb87a6b70e9 100644 --- a/src/uu/timeout/Cargo.toml +++ b/src/uu/timeout/Cargo.toml @@ -27,3 +27,11 @@ fluent = { workspace = true } [[bin]] name = "timeout" path = "src/main.rs" + +[dev-dependencies] +divan = { workspace = true } +uucore = { workspace = true, features = ["benchmark"] } + +[[bench]] +name = "timeout_bench" +harness = false diff --git a/src/uu/timeout/benches/timeout_bench.rs b/src/uu/timeout/benches/timeout_bench.rs new file mode 100644 index 00000000000..3cfd968cf4e --- /dev/null +++ b/src/uu/timeout/benches/timeout_bench.rs @@ -0,0 +1,121 @@ +// This file is part of the uutils coreutils package. +// +// For the full copyright and license information, please view the LICENSE +// file that was distributed with this source code. + +use std::env; +use std::process; +use std::time::Duration; + +const CHILD_FLAG: &str = "--timeout-bench-child"; + +fn maybe_run_child_mode() { + let mut args = env::args(); + let _ = args.next(); // skip executable path + + while let Some(arg) = args.next() { + if arg == CHILD_FLAG { + let mode = args + .next() + .unwrap_or_else(|| panic!("missing child mode after {CHILD_FLAG}")); + run_child(mode); + } + } +} + +#[cfg(unix)] +fn run_child(mode: String) -> ! { + match mode.as_str() { + "quick-exit" => process::exit(0), + "short-sleep" => { + std::thread::sleep(Duration::from_millis(5)); + process::exit(0); + } + "long-sleep" => { + std::thread::sleep(Duration::from_secs(2)); + process::exit(0); + } + "ignore-term" => { + use nix::sys::signal::{SigHandler, Signal, signal}; + + unsafe { + signal(Signal::SIGTERM, SigHandler::SigIgn) + .expect("failed to ignore SIGTERM in bench child"); + } + + loop { + std::thread::sleep(Duration::from_millis(100)); + } + } + other => { + eprintln!("unknown child mode: {other}"); + process::exit(1); + } + } +} + +#[cfg(not(unix))] +fn run_child(_: String) -> ! { + // The timeout benchmarks are Unix-only, but ensure child invocations still terminate. + process::exit(0); +} + +#[cfg(unix)] +mod unix { + use super::*; + use divan::{Bencher, black_box}; + use uu_timeout::uumain; + use uucore::benchmark::run_util_function; + + fn bench_timeout_with_mode(bencher: Bencher, args: &[&str], child_mode: &str) { + let child_path = env::current_exe() + .expect("failed to locate timeout bench executable") + .into_os_string() + .into_string() + .expect("bench executable path must be valid UTF-8"); + + let mut owned_args: Vec = args.iter().map(|s| (*s).to_string()).collect(); + owned_args.push(child_path); + owned_args.push(CHILD_FLAG.into()); + owned_args.push(child_mode.to_string()); + + let arg_refs: Vec<&str> = owned_args.iter().map(|s| s.as_str()).collect(); + + bencher.bench(|| { + black_box(run_util_function(uumain, &arg_refs)); + }); + } + + /// Benchmark the fast path where the command exits immediately. + #[divan::bench] + fn timeout_quick_exit(bencher: Bencher) { + bench_timeout_with_mode(bencher, &["0.1"], "quick-exit"); + } + + /// Benchmark a command that runs longer than the threshold and receives the default signal. + #[divan::bench] + fn timeout_enforced(bencher: Bencher) { + bench_timeout_with_mode(bencher, &["0.05"], "long-sleep"); + } + + /// Benchmark the `-k/--kill-after` flow where the child ignores SIGTERM. + #[divan::bench] + fn timeout_kill_after(bencher: Bencher) { + bench_timeout_with_mode(bencher, &["-k", "0.1", "0.05"], "ignore-term"); + } + + pub fn run() { + divan::main(); + } +} + +#[cfg(unix)] +fn main() { + maybe_run_child_mode(); + unix::run(); +} + +#[cfg(not(unix))] +fn main() { + maybe_run_child_mode(); +} diff --git a/src/uu/timeout/src/timeout.rs b/src/uu/timeout/src/timeout.rs index d0fba88ca71..4834d040b23 100644 --- a/src/uu/timeout/src/timeout.rs +++ b/src/uu/timeout/src/timeout.rs @@ -11,7 +11,10 @@ use clap::{Arg, ArgAction, Command}; use std::io::ErrorKind; use std::os::unix::process::ExitStatusExt; use std::process::{self, Child, Stdio}; -use std::sync::atomic::{self, AtomicBool}; +use std::sync::{ + OnceLock, + atomic::{self, AtomicBool}, +}; use std::time::Duration; use uucore::display::Quotable; use uucore::error::{UResult, USimpleError, UUsageError}; @@ -183,6 +186,22 @@ fn unblock_sigchld() { /// We should terminate child process when receiving TERM signal. static SIGNALED: AtomicBool = AtomicBool::new(false); +static SIGNAL_KILL: OnceLock = OnceLock::new(); +static SIGNAL_CONT: OnceLock = OnceLock::new(); + +#[inline] +fn signal_kill() -> usize { + *SIGNAL_KILL.get_or_init(|| { + signal_by_name_or_value("KILL").expect("KILL signal must be available on this platform") + }) +} + +#[inline] +fn signal_cont() -> usize { + *SIGNAL_CONT.get_or_init(|| { + signal_by_name_or_value("CONT").expect("CONT signal must be available on this platform") + }) +} fn catch_sigterm() { use nix::sys::signal; @@ -209,6 +228,16 @@ fn report_if_verbose(signal: usize, cmd: &str, verbose: bool) { } } +fn preserved_child_exit(status: std::process::ExitStatus) -> i32 { + if let Some(code) = status.code() { + code + } else if let Some(signal) = status.signal() { + ExitStatus::SignalSent(signal.try_into().unwrap()).into() + } else { + ExitStatus::CommandTimedOut.into() + } +} + fn send_signal(process: &mut Child, signal: usize, foreground: bool) { // NOTE: GNU timeout doesn't check for errors of signal. // The subprocess might have exited just after the timeout. @@ -217,10 +246,10 @@ fn send_signal(process: &mut Child, signal: usize, foreground: bool) { let _ = process.send_signal(signal); } else { let _ = process.send_signal_group(signal); - let kill_signal = signal_by_name_or_value("KILL").unwrap(); - let continued_signal = signal_by_name_or_value("CONT").unwrap(); + let kill_signal = signal_kill(); + let continued_signal = signal_cont(); if signal != kill_signal && signal != continued_signal { - _ = process.send_signal_group(continued_signal); + let _ = process.send_signal_group(continued_signal); } } } @@ -236,10 +265,11 @@ fn send_signal(process: &mut Child, signal: usize, foreground: bool) { /// If the child process terminates within the given time period and /// `preserve_status` is `true`, then the status code of the child /// process is returned. If the child process terminates within the -/// given time period and `preserve_status` is `false`, then 124 is +/// given time period and `preserve_status` is `false`, then 125 is /// returned. If the child does not terminate within the time period, -/// then 137 is returned. Finally, if there is an error while waiting -/// for the child process to terminate, then 124 is returned. +/// then 137 is returned (or 143 if `timeout` itself receives +/// `SIGTERM`). Finally, if there is an error while waiting for the +/// child process to terminate, then 124 is returned. /// /// # Errors /// @@ -252,22 +282,27 @@ fn wait_or_kill_process( preserve_status: bool, foreground: bool, verbose: bool, + signaled: &AtomicBool, ) -> std::io::Result { // ignore `SIGTERM` here - match process.wait_or_timeout(duration, None) { + match process.wait_or_timeout(duration, Some(signaled)) { Ok(Some(status)) => { if preserve_status { - Ok(status.code().unwrap_or_else(|| status.signal().unwrap())) + Ok(preserved_child_exit(status)) } else { Ok(ExitStatus::TimeoutFailed.into()) } } Ok(None) => { - let signal = signal_by_name_or_value("KILL").unwrap(); + let signal = signal_kill(); report_if_verbose(signal, cmd, verbose); send_signal(process, signal, foreground); process.wait()?; - Ok(ExitStatus::SignalSent(signal).into()) + if signaled.load(atomic::Ordering::Relaxed) { + Ok(ExitStatus::Terminated.into()) + } else { + Ok(ExitStatus::SignalSent(signal).into()) + } } Err(_) => Ok(ExitStatus::WaitingFailed.into()), } @@ -358,19 +393,14 @@ fn timeout( match kill_after { None => { let status = process.wait()?; - if SIGNALED.load(atomic::Ordering::Relaxed) { - Err(ExitStatus::Terminated.into()) + let exit_code = if SIGNALED.load(atomic::Ordering::Relaxed) { + ExitStatus::Terminated.into() } else if preserve_status { - if let Some(ec) = status.code() { - Err(ec.into()) - } else if let Some(sc) = status.signal() { - Err(ExitStatus::SignalSent(sc.try_into().unwrap()).into()) - } else { - Err(ExitStatus::CommandTimedOut.into()) - } + preserved_child_exit(status) } else { - Err(ExitStatus::CommandTimedOut.into()) - } + ExitStatus::CommandTimedOut.into() + }; + Err(exit_code.into()) } Some(kill_after) => { match wait_or_kill_process( @@ -380,6 +410,7 @@ fn timeout( preserve_status, foreground, verbose, + &SIGNALED, ) { Ok(status) => Err(status.into()), Err(e) => Err(USimpleError::new( diff --git a/src/uucore/src/lib/features/process.rs b/src/uucore/src/lib/features/process.rs index 55e8c36482b..4e118604071 100644 --- a/src/uucore/src/lib/features/process.rs +++ b/src/uucore/src/lib/features/process.rs @@ -129,22 +129,33 @@ impl ChildExt for Child { // .try_wait() doesn't drop stdin, so we do it manually drop(self.stdin.take()); + const MAX_SLEEP: Duration = Duration::from_millis(50); + const MIN_SLEEP: Duration = Duration::from_micros(500); + let start = Instant::now(); loop { if let Some(status) = self.try_wait()? { return Ok(Some(status)); } - if start.elapsed() >= timeout - || signaled.is_some_and(|signaled| signaled.load(atomic::Ordering::Relaxed)) - { + if signaled.is_some_and(|signaled| signaled.load(atomic::Ordering::Relaxed)) { + break; + } + + let Some(remaining) = timeout.checked_sub(start.elapsed()) else { break; + }; + if remaining.is_zero() { + break; + } + + // For tiny remaining durations, yield so we do not oversleep. + if remaining < MIN_SLEEP { + std::thread::yield_now(); + continue; } - // XXX: this is kinda gross, but it's cleaner than starting a thread just to wait - // (which was the previous solution). We might want to use a different duration - // here as well - thread::sleep(Duration::from_millis(100)); + thread::sleep(remaining.min(MAX_SLEEP)); } Ok(None) From 9780d795df553615993fd6be7364349030f73fe3 Mon Sep 17 00:00:00 2001 From: mattsu Date: Sat, 1 Nov 2025 15:06:08 +0900 Subject: [PATCH 2/4] feat: add uu_timeout to benchmark packages - Include uu_timeout utility in the GitHub Actions benchmarks workflow - Ensures performance metrics are collected for the newly added timeout command --- .github/workflows/benchmarks.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/benchmarks.yml b/.github/workflows/benchmarks.yml index 7485555e0cb..873fdb10ba3 100644 --- a/.github/workflows/benchmarks.yml +++ b/.github/workflows/benchmarks.yml @@ -40,6 +40,7 @@ jobs: - { package: uu_sort } - { package: uu_split } - { package: uu_tsort } + - { package: uu_timeout } - { package: uu_unexpand } - { package: uu_uniq } - { package: uu_wc } From b97cbb99eac2ef35d53bcb67dcef3ce54e838ba2 Mon Sep 17 00:00:00 2001 From: mattsu Date: Sat, 1 Nov 2025 16:11:18 +0900 Subject: [PATCH 3/4] perf(timeout): optimize benchmarks by reducing durations and removing unused test - Reduced sleep time in "long-sleep" mode from 2s to 200ms for faster execution - Shortened timeout values in quick_exit and enforced benchmarks from 0.1s/0.05s to 0.02s - Removed timeout_kill_after benchmark to streamline test suite and improve performance --- src/uu/timeout/benches/timeout_bench.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/uu/timeout/benches/timeout_bench.rs b/src/uu/timeout/benches/timeout_bench.rs index 3cfd968cf4e..99332f71a80 100644 --- a/src/uu/timeout/benches/timeout_bench.rs +++ b/src/uu/timeout/benches/timeout_bench.rs @@ -32,7 +32,7 @@ fn run_child(mode: String) -> ! { process::exit(0); } "long-sleep" => { - std::thread::sleep(Duration::from_secs(2)); + std::thread::sleep(Duration::from_millis(200)); process::exit(0); } "ignore-term" => { @@ -89,20 +89,16 @@ mod unix { /// Benchmark the fast path where the command exits immediately. #[divan::bench] fn timeout_quick_exit(bencher: Bencher) { - bench_timeout_with_mode(bencher, &["0.1"], "quick-exit"); + bench_timeout_with_mode(bencher, &["0.02"], "quick-exit"); } /// Benchmark a command that runs longer than the threshold and receives the default signal. #[divan::bench] fn timeout_enforced(bencher: Bencher) { - bench_timeout_with_mode(bencher, &["0.05"], "long-sleep"); + bench_timeout_with_mode(bencher, &["0.02"], "long-sleep"); } - /// Benchmark the `-k/--kill-after` flow where the child ignores SIGTERM. - #[divan::bench] - fn timeout_kill_after(bencher: Bencher) { - bench_timeout_with_mode(bencher, &["-k", "0.1", "0.05"], "ignore-term"); - } + pub fn run() { divan::main(); From de103b1d2e6071a27d1baa0542144f537ee1aa25 Mon Sep 17 00:00:00 2001 From: mattsu Date: Sat, 1 Nov 2025 16:12:08 +0900 Subject: [PATCH 4/4] style: remove extra blank lines in timeout benchmark Removed unnecessary blank lines after the bench_timeout_with_mode call in src/uu/timeout/benches/timeout_bench.rs to improve code readability and consistency. --- src/uu/timeout/benches/timeout_bench.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/uu/timeout/benches/timeout_bench.rs b/src/uu/timeout/benches/timeout_bench.rs index 99332f71a80..3ed0e5dc6e9 100644 --- a/src/uu/timeout/benches/timeout_bench.rs +++ b/src/uu/timeout/benches/timeout_bench.rs @@ -98,8 +98,6 @@ mod unix { bench_timeout_with_mode(bencher, &["0.02"], "long-sleep"); } - - pub fn run() { divan::main(); }