ronantakizawa/codereview-bench huggingface dataset:
Repository: sharkdp/hyperfine
File: src/hyperfine/error.rs (Rust)

Code before:
    }
}

impl Error for ParameterScanError {}

#[derive(Debug)]
pub enum OptionsError<'a> {
    RunsBelowTwo,
    EmptyRunsRange,
    TooManyCommandNames(usize),
    DifferentCommandNameCountWithParameters(usize, usize),
    NumericParsingError(&'a str, ParseIntError),
}

impl<'a> fmt::Display for OptionsError<'a> {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        match *self {
            OptionsError::EmptyRunsRange => write!(f, "Empty runs range"),
            OptionsError::RunsBelowTwo => write!(f, "Number of runs below two"),
            OptionsError::TooManyCommandNames(n) => {
                write!(f, "Too many --command-name options: expected {} at most", n)
            }
            OptionsError::DifferentCommandNameCountWithParameters(real, expected) => {
                write!(
                    f,
                    "Current --command-name count is {}: expected {} as parameters",
                    real, expected
                )
            }
            OptionsError::NumericParsingError(cmd, ref err) => write!(
                f,
                "Could not read numeric argument to '--{cmd}': {err}",
                cmd = cmd,
                err = err
            ),
        }
    }
}

impl<'a> Error for OptionsError<'a> {}


Review comment (suggestion):
This will be shown to the user, so let's try to make it a bit more descriptive. Maybe:

"'--command-name' has been specified {} times. It has to appear exactly once, or exactly {} times (number of benchmarks)"

Code after:
    }
}

impl Error for ParameterScanError {}

#[derive(Debug)]
pub enum OptionsError<'a> {
    RunsBelowTwo,
    EmptyRunsRange,
    TooManyCommandNames(usize),
    UnexpectedCommandNameCount(usize, usize),
    NumericParsingError(&'a str, ParseIntError),
}

impl<'a> fmt::Display for OptionsError<'a> {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        match *self {
            OptionsError::EmptyRunsRange => write!(f, "Empty runs range"),
            OptionsError::RunsBelowTwo => write!(f, "Number of runs below two"),
            OptionsError::TooManyCommandNames(n) => {
                write!(f, "Too many --command-name options: expected {} at most", n)
            }
            OptionsError::UnexpectedCommandNameCount(real, expected) => {
                write!(
                    f,
                    "'--command-name' has been specified {} times. It has to appear exactly once, or exactly {} times (number of benchmarks)",
                    real, expected
                )
            }
            OptionsError::NumericParsingError(cmd, ref err) => write!(
                f,
                "Could not read numeric argument to '--{cmd}': {err}",
                cmd = cmd,
                err = err
            ),
        }
    }
}

impl<'a> Error for OptionsError<'a> {}
Repository: sharkdp/hyperfine
File: src/benchmark.rs (Rust)

Code before:
    let progress_bar = if options.output_style != OutputStyleOption::Disabled {
        Some(get_progress_bar(
            options.runs.min,
            "Initial time measurement",
            options.output_style,
        ))
    } else {
        None
    };

    let prepare_res = run_preparation_command(&options.shell, &prepare_cmd, options.show_output)?;

    // Initial timing run
    let (res, status) = time_shell_command(
        &options.shell,
        cmd,
        options.show_output,
        options.failure_action,
        Some(shell_spawning_time),
    )?;
    let success = status.success();

    // Determine number of benchmark runs
    let runs_in_min_time = (options.min_time_sec
        / (res.time_real
            + setup_res.time_real
            + prepare_res.time_real
            + shell_spawning_time.time_real)) as u64;

    let count = {
        let min = cmp::max(runs_in_min_time, options.runs.min);

        options
            .runs
            .max
            .as_ref()
            .map(|max| cmp::min(min, *max))
            .unwrap_or(min)
    };

    let count_remaining = count - 1;

    // Save the first result
    times_real.push(res.time_real);
    times_user.push(res.time_user);
    times_system.push(res.time_system);
    exit_codes.push(extract_exit_code(status));

    all_succeeded = all_succeeded && success;

    // Re-configure the progress bar

Review comment (suggestion):
I don't think `setup_res.time_real` should be added here. It's ran only once before each series of benchmarks, so it shouldn't influence how many benchmark runs we can perform in the "min. time" that is reserved for benchmarks.

Setup time will obviously be there in addition, but shouldn't influence how many benchmarks we perform.

Code after:
        ))
    } else {
        None
    };

    let prepare_res = run_preparation_command(&options.shell, &prepare_cmd, options.show_output)?;

    // Initial timing run
    let (res, status) = time_shell_command(
        &options.shell,
        cmd,
        options.show_output,
        options.failure_action,
        Some(shell_spawning_time),
    )?;
    let success = status.success();

    // Determine number of benchmark runs
    let runs_in_min_time = (options.min_time_sec
        / (res.time_real + prepare_res.time_real + shell_spawning_time.time_real))
        as u64;

    let count = {
        let min = cmp::max(runs_in_min_time, options.runs.min);

        options
            .runs
            .max
            .as_ref()
            .map(|max| cmp::min(min, *max))
            .unwrap_or(min)
    };

    let count_remaining = count - 1;

    // Save the first result
    times_real.push(res.time_real);
    times_user.push(res.time_user);
    times_system.push(res.time_system);
    exit_codes.push(extract_exit_code(status));

    all_succeeded = all_succeeded && success;

    // Re-configure the progress bar
    if let Some(bar) = progress_bar.as_ref() {
        bar.set_length(count)
    }
    if let Some(bar) = progress_bar.as_ref() {
        bar.inc(1)
    }
Repository: sharkdp/hyperfine
File: src/options.rs (Rust)

Code before:
    /// Maximum number of benchmark runs
    pub max: Option<u64>,
}

impl Default for Runs {
    fn default() -> Runs {
        Runs { min: 10, max: None }
    }
}

/// A set of options for hyperfine
pub struct HyperfineOptions {
    /// Number of warmup runs
    pub warmup_count: u64,

    /// Number of benchmark runs
    pub runs: Runs,

    /// Minimum benchmarking time
    pub min_time_sec: Second,

    /// Whether or not to ignore non-zero exit codes
    pub failure_action: CmdFailureAction,

    /// Command to run before each batch of timing runs
    pub setup_command: Option<Vec<String>>,

    /// Command to run before each timing run
    pub preparation_command: Option<Vec<String>>,

    /// Command to run after each benchmark
    pub cleanup_command: Option<String>,

    /// What color mode to use for output
    pub output_style: OutputStyleOption,

    /// The shell to use for executing commands.
    pub shell: Shell,

    /// Forward benchmark's stdout to hyperfine's stdout
    pub show_output: bool,

    /// Which time unit to use for CLI & Markdown output
    pub time_unit: Option<Unit>,

    /// A list of custom command names that, if defined,
    /// will be used instead of the command itself in
    /// benchmark outputs.
    pub names: Option<Vec<String>>,
}


Review comment (suggestion):
This should be just a `Option<String>`, since only one setup command can be provided.

Code after:
    /// Maximum number of benchmark runs
    pub max: Option<u64>,
}

impl Default for Runs {
    fn default() -> Runs {
        Runs { min: 10, max: None }
    }
}

/// A set of options for hyperfine
pub struct HyperfineOptions {
    /// Number of warmup runs
    pub warmup_count: u64,

    /// Number of benchmark runs
    pub runs: Runs,

    /// Minimum benchmarking time
    pub min_time_sec: Second,

    /// Whether or not to ignore non-zero exit codes
    pub failure_action: CmdFailureAction,

    /// Command to run before each batch of timing runs
    pub setup_command: Option<String>,

    /// Command to run before each timing run
    pub preparation_command: Option<Vec<String>>,

    /// Command to run after each benchmark
    pub cleanup_command: Option<String>,

    /// What color mode to use for output
    pub output_style: OutputStyleOption,

    /// The shell to use for executing commands.
    pub shell: Shell,

    /// Forward benchmark's stdout to hyperfine's stdout
    pub show_output: bool,

    /// Which time unit to use for CLI & Markdown output
    pub time_unit: Option<Unit>,

    /// A list of custom command names that, if defined,
    /// will be used instead of the command itself in
    /// benchmark outputs.
    pub names: Option<Vec<String>>,
}
Repository: sharkdp/hyperfine
File: tests/integration_tests.rs (Rust)

Code before:
}

#[test]
fn warmup_runs_are_executed_before_benchmarking_runs() {
    ExecutionOrderTest::new()
        .arg("--runs=2")
        .arg("--warmup=3")
        .command("command 1")
        .expect_output("command 1")
        .expect_output("command 1")
        .expect_output("command 1")
        .expect_output("command 1")
        .expect_output("command 1")
        .run();
}

#[test]
fn setup_can_only_be_provided_once() {
    hyperfine()
        .arg("--runs=2")
        .arg("--setup=setup")
        .arg("--setup=setup-again")
        .arg("true")
        .assert()
        .failure();
}

#[test]
fn setup_commands_are_executed_before_each_series_of_timing_runs() {
    ExecutionOrderTest::new()
        .arg("--runs=2")
        .setup("setup")
        .command("command 1")
        .command("command 2")
        .expect_output("setup")
        .expect_output("command 1")
        .expect_output("command 1")
        .expect_output("setup")
        .expect_output("command 2")
        .expect_output("command 2")
        .run();
}

#[test]
fn prepare_commands_are_executed_before_each_timing_run() {
    ExecutionOrderTest::new()
        .arg("--runs=2")
        .prepare("prepare")
        .command("command 1")
        .command("command 2")
        .expect_output("prepare")

Review comment (suggestion):
this can probably be removed once you remove `.multiple(true)`. We don't need to test whether or not the command line parser works.

Code after:
#[test]
fn benchmarks_are_executed_sequentially() {
    ExecutionOrderTest::new()
        .arg("--runs=2")
        .command("command 1")
        .command("command 2")
        .expect_output("command 1")
        .expect_output("command 1")
        .expect_output("command 2")
        .expect_output("command 2")
        .run();
}

#[test]
fn warmup_runs_are_executed_before_benchmarking_runs() {
    ExecutionOrderTest::new()
        .arg("--runs=2")
        .arg("--warmup=3")
        .command("command 1")
        .expect_output("command 1")
        .expect_output("command 1")
        .expect_output("command 1")
        .expect_output("command 1")
        .expect_output("command 1")
        .run();
}

#[test]
fn setup_commands_are_executed_before_each_series_of_timing_runs() {
    ExecutionOrderTest::new()
        .arg("--runs=2")
        .setup("setup")
        .command("command 1")
        .command("command 2")
        .expect_output("setup")
        .expect_output("command 1")
        .expect_output("command 1")
        .expect_output("setup")
        .expect_output("command 2")
        .expect_output("command 2")
        .run();
}

#[test]
fn prepare_commands_are_executed_before_each_timing_run() {
    ExecutionOrderTest::new()
        .arg("--runs=2")
        .prepare("prepare")
        .command("command 1")
        .command("command 2")
        .expect_output("prepare")
Repository: sharkdp/hyperfine
File: src/error.rs (Rust)

Code before:
    }
}

impl From<DecimalError> for ParameterScanError {
    fn from(e: DecimalError) -> ParameterScanError {
        ParameterScanError::ParseDecimalError(e)
    }
}

#[derive(Debug, Error)]
pub enum OptionsError<'a> {
    #[error(
        "Conflicting requirements for the number of runs (empty range, min is larger than max)"
    )]
    EmptyRunsRange,
    #[error("Too many --command-name options: Expected {0} at most")]
    TooManyCommandNames(usize),
    #[error("'--command-name' has been specified {0} times. It has to appear exactly once, or exactly {1} times (number of benchmarks)")]
    UnexpectedCommandNameCount(usize, usize),
    #[error("Could not read numeric argument to '--{0}': {1}")]
    NumericParsingError(&'a str, ParseIntError),
    #[error("An empty command has been specified for the '--shell <command>' option")]
    EmptyShell,
    #[error("Failed to parse '--shell <command>' expression as command line: {0}")]
    ShellParseError(shell_words::ParseError),
    #[error("Unknown output policy '{0}'. Use ./{0} to output to a file named {0}.")]
    UnknownOutputPolicy(String),
}


Review comment (suggestion):
Minor:
```suggestion
    #[error("Unknown output policy '{0}'. Use './{0}' to output to a file named '{0}'.")]
```

Code after:
    }
}

impl From<DecimalError> for ParameterScanError {
    fn from(e: DecimalError) -> ParameterScanError {
        ParameterScanError::ParseDecimalError(e)
    }
}

#[derive(Debug, Error)]
pub enum OptionsError<'a> {
    #[error(
        "Conflicting requirements for the number of runs (empty range, min is larger than max)"
    )]
    EmptyRunsRange,
    #[error("Too many --command-name options: Expected {0} at most")]
    TooManyCommandNames(usize),
    #[error("'--command-name' has been specified {0} times. It has to appear exactly once, or exactly {1} times (number of benchmarks)")]
    UnexpectedCommandNameCount(usize, usize),
    #[error("Could not read numeric argument to '--{0}': {1}")]
    NumericParsingError(&'a str, ParseIntError),
    #[error("An empty command has been specified for the '--shell <command>' option")]
    EmptyShell,
    #[error("Failed to parse '--shell <command>' expression as command line: {0}")]
    ShellParseError(shell_words::ParseError),
    #[error("Unknown output policy '{0}'. Use './{0}' to output to a file named '{0}'.")]
    UnknownOutputPolicy(String),
}
Repository: sharkdp/hyperfine
File: src/timer/mod.rs (Rust)

Code before:

use std::process::{ChildStdout, Command, ExitStatus};

use anyhow::Result;

#[derive(Debug, Copy, Clone)]
struct CPUTimes {
    /// Total amount of time spent executing in user mode
    pub user_usec: i64,

    /// Total amount of time spent executing in kernel mode
    pub system_usec: i64,
}

/// Used to indicate the result of running a command
#[derive(Debug, Copy, Clone)]
pub struct TimerResult {
    pub time_real: Second,
    pub time_user: Second,
    pub time_system: Second,

    /// The exit status of the process
    pub status: ExitStatus,
}

/// Discard the output of a child process.
fn discard(output: ChildStdout) {
    const CHUNK_SIZE: usize = 64 << 10;

    #[cfg(target_os = "linux")]
    {
        if let Ok(file) = File::create("/dev/null") {
            while let Ok(bytes) = splice(
                output.as_raw_fd(),
                None,
                file.as_raw_fd(),
                None,
                CHUNK_SIZE,
                SpliceFFlags::empty(),
            ) {
                if bytes == 0 {
                    break;
                }
            }
        }
    }

    #[cfg(not(target_os = "linux"))]
    {
        let mut output = output;
        let mut buf = [0; CHUNK_SIZE];

Review comment (performance):
Just out of interest: For `splice`, I assume this needs to be larger than a page size... but performance probably levels off at some point, so there's no gain in making this even larger?

Code after:

use anyhow::Result;

#[derive(Debug, Copy, Clone)]
struct CPUTimes {
    /// Total amount of time spent executing in user mode
    pub user_usec: i64,

    /// Total amount of time spent executing in kernel mode
    pub system_usec: i64,
}

/// Used to indicate the result of running a command
#[derive(Debug, Copy, Clone)]
pub struct TimerResult {
    pub time_real: Second,
    pub time_user: Second,
    pub time_system: Second,

    /// The exit status of the process
    pub status: ExitStatus,
}

/// Discard the output of a child process.
fn discard(output: ChildStdout) {
    const CHUNK_SIZE: usize = 64 << 10;

    #[cfg(target_os = "linux")]
    {
        if let Ok(file) = File::create("/dev/null") {
            while let Ok(bytes) = splice(
                output.as_raw_fd(),
                None,
                file.as_raw_fd(),
                None,
                CHUNK_SIZE,
                SpliceFFlags::empty(),
            ) {
                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;
        }
Repository: sharkdp/hyperfine
File: src/benchmark/executor.rs (Rust)

Code before:
use std::process::{ExitStatus, Stdio};

use crate::command::Command;
use crate::options::{
    CmdFailureAction, CommandInputPolicy, CommandOutputPolicy, Options, OutputStyleOption, Shell,
};
use crate::output::progress_bar::get_progress_bar;
use crate::timer::{execute_and_measure, TimerResult};
use crate::util::randomized_environment_offset;
use crate::util::units::Second;

use super::timing_result::TimingResult;

use anyhow::{bail, Context, Result};
use statistical::mean;

pub trait Executor {
    /// Run the given command and measure the execution time
    fn run_command_and_measure(
        &self,
        command: &Command<'_>,
        command_failure_action: Option<CmdFailureAction>,
    ) -> Result<(TimingResult, ExitStatus)>;

    /// Perform a calibration of this executor. For example,
    /// when running commands through a shell, we need to

Review comment (suggestion):
Can we please fie warning here (unused import: `Stdio`)

Code after:
use std::process::ExitStatus;

use crate::command::Command;
use crate::options::{
    CmdFailureAction, CommandInputPolicy, CommandOutputPolicy, Options, OutputStyleOption, Shell,
};
use crate::output::progress_bar::get_progress_bar;
use crate::timer::{execute_and_measure, TimerResult};
use crate::util::randomized_environment_offset;
use crate::util::units::Second;

use super::timing_result::TimingResult;

use anyhow::{bail, Context, Result};
use statistical::mean;

pub trait Executor {
    /// Run the given command and measure the execution time
    fn run_command_and_measure(
        &self,
        command: &Command<'_>,
        command_failure_action: Option<CmdFailureAction>,
    ) -> Result<(TimingResult, ExitStatus)>;

    /// Perform a calibration of this executor. For example,
    /// when running commands through a shell, we need to
Repository: sharkdp/hyperfine
File: src/options.rs (Rust)

Code before:

    /// Disable all the output
    Disabled,
}

/// Bounds for the number of benchmark runs
pub struct RunBounds {
    /// Minimum number of benchmark runs
    pub min: u64,

    /// Maximum number of benchmark runs
    pub max: Option<u64>,
}

impl Default for RunBounds {
    fn default() -> Self {
        RunBounds { min: 10, max: None }
    }
}

#[derive(Debug, Clone, PartialEq)]
pub enum CommandInputPolicy {
    /// Redirect from the null device
    Null,

    /// Redirect input from a file
    File(PathBuf),
}

impl Default for CommandInputPolicy {
    fn default() -> Self {
        CommandInputPolicy::Null
    }
}

impl CommandInputPolicy {
    pub fn get_stdin(&self) -> io::Result<Stdio> {
        let stream: Stdio = match self {
            CommandInputPolicy::Null => Stdio::null(),

            CommandInputPolicy::File(path) => {
                let file: File = File::open(&path)?;
                Stdio::from(file)
            }
        };

        Ok(stream)
    }
}

/// How to handle the output of benchmarked commands

Review comment (suggestion):
```suggestion
    /// Read input from a file
```

Code after:

    /// Disable all the output
    Disabled,
}

/// Bounds for the number of benchmark runs
pub struct RunBounds {
    /// Minimum number of benchmark runs
    pub min: u64,

    /// Maximum number of benchmark runs
    pub max: Option<u64>,
}

impl Default for RunBounds {
    fn default() -> Self {
        RunBounds { min: 10, max: None }
    }
}

#[derive(Debug, Clone, PartialEq)]
pub enum CommandInputPolicy {
    /// Read from the null device
    Null,

    /// Read input from a file
    File(PathBuf),
}

impl Default for CommandInputPolicy {
    fn default() -> Self {
        CommandInputPolicy::Null
    }
}

impl CommandInputPolicy {
    pub fn get_stdin(&self) -> io::Result<Stdio> {
        let stream: Stdio = match self {
            CommandInputPolicy::Null => Stdio::null(),

            CommandInputPolicy::File(path) => {
                let file: File = File::open(&path)?;
                Stdio::from(file)
            }
        };

        Ok(stream)
    }
}

/// How to handle the output of benchmarked commands
Repository: sharkdp/hyperfine
File: src/options.rs (Rust)

Code before:

    /// Keep coloring, but use no progress bar
    Color,

    /// Disable all the output
    Disabled,
}

/// Bounds for the number of benchmark runs
pub struct RunBounds {
    /// Minimum number of benchmark runs
    pub min: u64,

    /// Maximum number of benchmark runs
    pub max: Option<u64>,
}

impl Default for RunBounds {
    fn default() -> Self {
        RunBounds { min: 10, max: None }
    }
}

#[derive(Debug, Clone, PartialEq)]
pub enum CommandInputPolicy {
    /// Redirect from the null device
    Null,

    /// Redirect input from a file
    File(PathBuf),
}

impl Default for CommandInputPolicy {
    fn default() -> Self {
        CommandInputPolicy::Null
    }
}

impl CommandInputPolicy {
    pub fn get_stdin(&self) -> io::Result<Stdio> {
        let stream: Stdio = match self {
            CommandInputPolicy::Null => Stdio::null(),

            CommandInputPolicy::File(path) => {
                let file: File = File::open(&path)?;
                Stdio::from(file)
            }
        };

        Ok(stream)
    }

Review comment (suggestion):
```suggestion
    /// Read from the null device
```

Code after:

    /// Keep coloring, but use no progress bar
    Color,

    /// Disable all the output
    Disabled,
}

/// Bounds for the number of benchmark runs
pub struct RunBounds {
    /// Minimum number of benchmark runs
    pub min: u64,

    /// Maximum number of benchmark runs
    pub max: Option<u64>,
}

impl Default for RunBounds {
    fn default() -> Self {
        RunBounds { min: 10, max: None }
    }
}

#[derive(Debug, Clone, PartialEq)]
pub enum CommandInputPolicy {
    /// Read from the null device
    Null,

    /// Read input from a file
    File(PathBuf),
}

impl Default for CommandInputPolicy {
    fn default() -> Self {
        CommandInputPolicy::Null
    }
}

impl CommandInputPolicy {
    pub fn get_stdin(&self) -> io::Result<Stdio> {
        let stream: Stdio = match self {
            CommandInputPolicy::Null => Stdio::null(),

            CommandInputPolicy::File(path) => {
                let file: File = File::open(&path)?;
                Stdio::from(file)
            }
        };

        Ok(stream)
    }