From 3df63ba2bb14e75775e0e1fae744043e7ad79a5f Mon Sep 17 00:00:00 2001 From: Tavian Barnes Date: Tue, 17 May 2022 14:21:16 -0400 Subject: [PATCH] Address review comments --- src/error.rs | 2 +- src/options.rs | 30 +++++++++++++++--------------- src/timer/mod.rs | 27 ++++++++++++--------------- 3 files changed, 28 insertions(+), 31 deletions(-) diff --git a/src/error.rs b/src/error.rs index 72701c4fd..c24e9ecc7 100644 --- a/src/error.rs +++ b/src/error.rs @@ -49,6 +49,6 @@ pub enum OptionsError<'a> { EmptyShell, #[error("Failed to parse '--shell ' expression as command line: {0}")] ShellParseError(shell_words::ParseError), - #[error("Unknown output policy '{0}'. Use ./{0} to output to a file named {0}.")] + #[error("Unknown output policy '{0}'. Use './{0}' to output to a file named '{0}'.")] UnknownOutputPolicy(String), } diff --git a/src/options.rs b/src/options.rs index aed0156b4..7281a6201 100644 --- a/src/options.rs +++ b/src/options.rs @@ -113,8 +113,8 @@ impl Default for RunBounds { /// How to handle the output of benchmarked commands #[derive(Debug, Clone, PartialEq)] pub enum CommandOutputPolicy { - /// Discard all output - Discard, + /// Redirect output to the null device + Null, /// Feed output through a pipe before discarding it Pipe, @@ -123,19 +123,19 @@ pub enum CommandOutputPolicy { File(PathBuf), /// Show command output on the terminal - Forward, + Inherit, } impl Default for CommandOutputPolicy { fn default() -> Self { - CommandOutputPolicy::Discard + CommandOutputPolicy::Null } } impl CommandOutputPolicy { pub fn get_stdout_stderr(&self) -> io::Result<(Stdio, Stdio)> { let streams = match self { - CommandOutputPolicy::Discard => (Stdio::null(), Stdio::null()), + CommandOutputPolicy::Null => (Stdio::null(), Stdio::null()), // Typically only stdout is performance-relevant, so just pipe that CommandOutputPolicy::Pipe => (Stdio::piped(), Stdio::null()), @@ -145,7 +145,7 @@ impl CommandOutputPolicy { (file.into(), Stdio::null()) } - CommandOutputPolicy::Forward => (Stdio::inherit(), Stdio::inherit()), + CommandOutputPolicy::Inherit => (Stdio::inherit(), Stdio::inherit()), }; Ok(streams) @@ -212,7 +212,7 @@ impl Default for Options { cleanup_command: None, output_style: OutputStyleOption::Full, executor_kind: ExecutorKind::default(), - command_output_policy: CommandOutputPolicy::Discard, + command_output_policy: CommandOutputPolicy::Null, time_unit: None, } } @@ -269,12 +269,12 @@ impl Options { options.cleanup_command = matches.value_of("cleanup").map(String::from); options.command_output_policy = if matches.is_present("show-output") { - CommandOutputPolicy::Forward + CommandOutputPolicy::Inherit } else if let Some(output) = matches.value_of("output") { match output { - "null" => CommandOutputPolicy::Discard, + "null" => CommandOutputPolicy::Null, "pipe" => CommandOutputPolicy::Pipe, - "inherit" => CommandOutputPolicy::Forward, + "inherit" => CommandOutputPolicy::Inherit, arg => { let path = PathBuf::from(arg); if path.components().count() <= 1 { @@ -284,7 +284,7 @@ impl Options { } } } else { - CommandOutputPolicy::Discard + CommandOutputPolicy::Null }; options.output_style = match matches.value_of("style") { @@ -294,12 +294,12 @@ impl Options { Some("color") => OutputStyleOption::Color, Some("none") => OutputStyleOption::Disabled, _ => { - if options.command_output_policy != CommandOutputPolicy::Forward - && atty::is(Stream::Stdout) + if options.command_output_policy == CommandOutputPolicy::Inherit + || !atty::is(Stream::Stdout) { - OutputStyleOption::Full - } else { OutputStyleOption::Basic + } else { + OutputStyleOption::Full } } }; diff --git a/src/timer/mod.rs b/src/timer/mod.rs index 4a87c8cf3..fd6b88a89 100644 --- a/src/timer/mod.rs +++ b/src/timer/mod.rs @@ -13,12 +13,10 @@ use std::fs::File; #[cfg(target_os = "linux")] use std::os::unix::io::AsRawFd; -#[cfg(not(target_os = "linux"))] -use std::io::Read; - use crate::util::units::Second; use wall_clock_timer::WallClockTimer; +use std::io::Read; use std::process::{ChildStdout, Command, ExitStatus}; use anyhow::Result; @@ -65,14 +63,11 @@ fn discard(output: ChildStdout) { } } - #[cfg(not(target_os = "linux"))] - { - let mut output = output; - let mut buf = [0; CHUNK_SIZE]; - while let Ok(bytes) = output.read(&mut buf) { - if bytes == 0 { - break; - } + let mut output = output; + let mut buf = [0; CHUNK_SIZE]; + while let Ok(bytes) = output.read(&mut buf) { + if bytes == 0 { + break; } } } @@ -81,17 +76,19 @@ fn discard(output: ChildStdout) { pub fn execute_and_measure(mut command: Command) -> Result { let wallclock_timer = WallClockTimer::start(); - let mut child = command.spawn()?; - let output = child.stdout.take(); - #[cfg(not(windows))] let cpu_timer = self::unix_timer::CPUTimer::start(); + + let mut child = command.spawn()?; + #[cfg(windows)] let cpu_timer = self::windows_timer::CPUTimer::start_for_process(&child); - if let Some(output) = output { + if let Some(output) = child.stdout.take() { + // Handle CommandOutputPolicy::Pipe discard(output); } + let status = child.wait()?; let (time_user, time_system) = cpu_timer.stop();