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

Code before:
    RaiseError,

    /// Simply ignore the non-zero exit code
    Ignore,
}

/// Output style type option
#[derive(Debug, Clone, Copy, PartialEq)]
pub enum OutputStyleOption {
    /// Do not output with colors or any special formatting
    Basic,

    /// Output with full color and formatting
    Full,

    /// Keep elements such as progress bar, but use no coloring
    NoColor,
}

/// Number of runs for a benchmark
pub struct Runs {
    /// Minimum number of benchmark runs
    pub min: u64,

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

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

/// 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,


Review comment (suggestion):
It would be great if we could use `Option<u64>` here to represent the possibility to have no maximum in the types (instead of using `u64::MAX` below - which also works, of course).

Code after:
    RaiseError,

    /// Simply ignore the non-zero exit code
    Ignore,
}

/// Output style type option
#[derive(Debug, Clone, Copy, PartialEq)]
pub enum OutputStyleOption {
    /// Do not output with colors or any special formatting
    Basic,

    /// Output with full color and formatting
    Full,

    /// Keep elements such as progress bar, but use no coloring
    NoColor,
}

/// Number of runs for a benchmark
pub struct Runs {
    /// Minimum number of benchmark runs
    pub min: u64,

    /// 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,
Repository: sharkdp/hyperfine
File: src/hyperfine/export/markdown.rs (Rust)

Code before:
use super::Exporter;

use hyperfine::format::{Unit, format_duration_value};
use hyperfine::types::BenchmarkResult;

use std::io::Result;

macro_rules! TABLE_HEADER {
    ($unit:expr) => {
        format!("| Command | Mean [{unit}] | Min…Max [{unit}] |\n|:---|---:|---:|\n", unit=$unit) };
}

#[derive(Default)]
pub struct MarkdownExporter {}

impl Exporter for MarkdownExporter {
    fn serialize(&self, results: &Vec<BenchmarkResult>) -> Result<Vec<u8>> {
        // Default to `Second`.
        let mut unit = Unit::Second;

        if !results.is_empty() {
            // Use the first BenchmarkResult entry to determine the unit for all entries.
            unit = format_duration_value(results[0].mean, None).1;
        }

        let mut destination = start_table(unit);

        for result in results {
            add_table_row(&mut destination, result, unit);
        }

        Ok(destination)
    }
}

fn start_table(unit: Unit) -> Vec<u8> {
    TABLE_HEADER!(unit.short_name()).bytes().collect()
}

fn add_table_row(dest: &mut Vec<u8>, entry: &BenchmarkResult, unit: Unit) {
    let mean_str = format_duration_value(entry.mean, Some(unit)).0;
    let stddev_str = format_duration_value(entry.stddev, Some(unit)).0;
    let min_str = format_duration_value(entry.min, Some(unit)).0;
    let max_str = format_duration_value(entry.max, Some(unit)).0;

    dest.extend(

Review comment (suggestion):
You can use
``` rust
if Some(first_result) = results.first() {
  // ..
}
```
here instead of relying on `is_empty()` and index-access.

Code after:
use super::Exporter;

use hyperfine::format::{Unit, format_duration_value};
use hyperfine::types::BenchmarkResult;

use std::io::Result;

#[derive(Default)]
pub struct MarkdownExporter {}

impl Exporter for MarkdownExporter {
    fn serialize(&self, results: &Vec<BenchmarkResult>) -> Result<Vec<u8>> {
        let unit = if let Some(first_result) = results.first() {
            // Use the first BenchmarkResult entry to determine the unit for all entries.
            format_duration_value(first_result.mean, None).1
        } else {
            // Default to `Second`.
            Unit::Second
        };

        let mut destination = start_table(unit);

        for result in results {
            add_table_row(&mut destination, result, unit);
        }

        Ok(destination)
    }
}

fn table_header(unit_short_name: String) -> String {
    format!("| Command | Mean [{unit}] | Min…Max [{unit}] |\n|:---|---:|---:|\n", unit=unit_short_name)
}

fn start_table(unit: Unit) -> Vec<u8> {
    table_header(unit.short_name()).bytes().collect()
}

fn add_table_row(dest: &mut Vec<u8>, entry: &BenchmarkResult, unit: Unit) {
    let mean_str = format_duration_value(entry.mean, Some(unit)).0;
    let stddev_str = format_duration_value(entry.stddev, Some(unit)).0;
    let min_str = format_duration_value(entry.min, Some(unit)).0;
    let max_str = format_duration_value(entry.max, Some(unit)).0;

    dest.extend(
        format!(
Repository: sharkdp/hyperfine
File: src/hyperfine/types.rs (Rust)

Code before:
/// This module contains common internal types.
use std::fmt;

/// Type alias for unit of time
pub type Second = f64;

/// Supported time units
#[derive(Debug, Clone, Copy, PartialEq)]
pub enum Unit {
    Second,
    MilliSecond,
}

impl Unit {
    /// The abbreviation of the Unit.
    pub fn short_name(&self) -> String {
        match *self {
            Unit::Second => String::from("s"),
            Unit::MilliSecond => String::from("ms"),
        }
    }

    /// Returns the Second value formatted for the Unit.
    pub fn format(&self, value: Second) -> String {
        match *self {
            Unit::Second => format!("{:.3}", value),
            Unit::MilliSecond => format!("{:.1}", value * 1e3),
        }
    }
}

/// A command that should be benchmarked.

Review comment (suggestion):
Good idea to move the `Unit`-related code. We could even consider moving it to a separate new module(?).

Code after:
/// This module contains common internal types.
use std::fmt;

use hyperfine::units::{Second, Unit};

#[cfg(not(windows))]
pub const DEFAULT_SHELL: &str = "sh";

#[cfg(windows)]
pub const DEFAULT_SHELL: &str = "cmd.exe";

/// A command that should be benchmarked.
#[derive(Debug, Clone)]
pub struct Command<'a> {
    /// The command that should be executed (without parameter substitution)
    expression: &'a str,

    /// A possible parameter value.
    parameter: Option<(&'a str, i32)>,
}

impl<'a> Command<'a> {
    pub fn new(expression: &'a str) -> Command<'a> {
        Command {
            expression: expression,
            parameter: None,
        }
    }

    pub fn new_parametrized(expression: &'a str, parameter: &'a str, value: i32) -> Command<'a> {
        Command {
            expression: expression,
Repository: sharkdp/hyperfine
File: src/hyperfine/types.rs (Rust)

Code before:
    pub parameter: Option<i32>,
}

impl BenchmarkResult {
    /// Create a new entry with the given values.
    pub fn new(
        command: String,
        mean: Second,
        stddev: Second,
        user: Second,
        system: Second,
        min: Second,
        max: Second,
        times: Vec<Second>,
        parameter: Option<i32>,
    ) -> Self {
        BenchmarkResult {
            command,
            mean,
            stddev,
            user,
            system,
            min,
            max,
            times: Some(times),
            parameter: parameter,
        }
    }
}


Review comment (suggestion):
I think this could just be `parameter` instead of `parameter: parameter`.

Code after:
    pub parameter: Option<i32>,
}

impl BenchmarkResult {
    /// Create a new entry with the given values.
    pub fn new(
        command: String,
        mean: Second,
        stddev: Second,
        user: Second,
        system: Second,
        min: Second,
        max: Second,
        times: Vec<Second>,
        parameter: Option<i32>,
    ) -> Self {
        BenchmarkResult {
            command,
            mean,
            stddev,
            user,
            system,
            min,
            max,
            times: Some(times),
            parameter,
        }
    }
}
Repository: sharkdp/hyperfine
File: src/hyperfine/types.rs (Rust)

Code before:
        NumericType::Decimal(self)
    }
}

/// A command that should be benchmarked.
#[derive(Debug, Clone)]
pub struct Command<'a> {
    /// The command that should be executed (without parameter substitution)
    expression: &'a str,

    /// A possible parameter value.
    parameter: Option<(&'a str, String)>,
}

impl<'a> Command<'a> {
    pub fn new(expression: &'a str) -> Command<'a> {
        Command {
            expression,
            parameter: None,
        }
    }

    pub fn new_parametrized(
        expression: &'a str,
        parameter: &'a str,
        value: String,
    ) -> Command<'a> {
        Command {
            expression,
            parameter: Some((parameter, value)),
        }
    }

    pub fn get_shell_command(&self) -> String {
        match &self.parameter {
            Some((param_name, param_value)) => self.expression.replace(
                &format!("{{{param_name}}}", param_name = param_name),
                &param_value.to_string(),
            ),
            None => self.expression.into(),
        }
    }

    pub fn get_parameter(&self) -> &Option<(&'a str, String)> {
        &self.parameter
    }
}

impl<'a> fmt::Display for Command<'a> {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        write!(f, "{}", self.get_shell_command())

Review comment (suggestion):
If you take a `&str` here instead of a `String`, you can avoid many of the `.clone` calls.

Code after:
        NumericType::Decimal(self)
    }
}

#[derive(Debug, Clone)]
pub enum ParameterValue {
    Text(String),
    Numeric(NumericType),
}

impl<'a> ToString for ParameterValue {
    fn to_string(&self) -> String {
        match self {
            ParameterValue::Text(ref value) => value.clone(),
            ParameterValue::Numeric(value) => value.to_string(),
        }
    }
}

/// A command that should be benchmarked.
#[derive(Debug, Clone)]
pub struct Command<'a> {
    /// The command that should be executed (without parameter substitution)
    expression: &'a str,

    /// A possible parameter value.
    parameter: Option<(&'a str, ParameterValue)>,
}

impl<'a> Command<'a> {
    pub fn new(expression: &'a str) -> Command<'a> {
        Command {
            expression,
            parameter: None,
        }
    }

    pub fn new_parametrized(
        expression: &'a str,
        parameter: &'a str,
        value: ParameterValue,
    ) -> Command<'a> {
        Command {
            expression,
            parameter: Some((parameter, value)),
        }
    }

    pub fn get_shell_command(&self) -> String {
        match &self.parameter {
            Some((param_name, param_value)) => self.expression.replace(
Repository: sharkdp/hyperfine
File: src/hyperfine/types.rs (Rust)

Code before:
            NumericType::Int(i) => fmt::Display::fmt(&i, f),
            NumericType::Decimal(i) => fmt::Display::fmt(&i, f),
        }
    }
}

impl Into<NumericType> for i32 {
    fn into(self) -> NumericType {
        NumericType::Int(self)
    }
}

impl Into<NumericType> for Decimal {
    fn into(self) -> NumericType {
        NumericType::Decimal(self)
    }
}

/// A command that should be benchmarked.
#[derive(Debug, Clone)]
pub struct Command<'a> {
    /// The command that should be executed (without parameter substitution)
    expression: &'a str,

    /// A possible parameter value.
    parameter: Option<(&'a str, String)>,
}

impl<'a> Command<'a> {
    pub fn new(expression: &'a str) -> Command<'a> {
        Command {
            expression,
            parameter: None,
        }
    }

    pub fn new_parametrized(
        expression: &'a str,
        parameter: &'a str,
        value: String,
    ) -> Command<'a> {
        Command {
            expression,
            parameter: Some((parameter, value)),
        }
    }

    pub fn get_shell_command(&self) -> String {
        match &self.parameter {
            Some((param_name, param_value)) => self.expression.replace(
                &format!("{{{param_name}}}", param_name = param_name),

Review comment (suggestion):
It might even be possible to use a `&'a str` reference here instead of a `String`.

Code after:
            NumericType::Int(i) => fmt::Display::fmt(&i, f),
            NumericType::Decimal(i) => fmt::Display::fmt(&i, f),
        }
    }
}

impl Into<NumericType> for i32 {
    fn into(self) -> NumericType {
        NumericType::Int(self)
    }
}

impl Into<NumericType> for Decimal {
    fn into(self) -> NumericType {
        NumericType::Decimal(self)
    }
}

#[derive(Debug, Clone)]
pub enum ParameterValue {
    Text(String),
    Numeric(NumericType),
}

impl<'a> ToString for ParameterValue {
    fn to_string(&self) -> String {
        match self {
            ParameterValue::Text(ref value) => value.clone(),
            ParameterValue::Numeric(value) => value.to_string(),
        }
    }
}

/// A command that should be benchmarked.
#[derive(Debug, Clone)]
pub struct Command<'a> {
    /// The command that should be executed (without parameter substitution)
    expression: &'a str,

    /// A possible parameter value.
    parameter: Option<(&'a str, ParameterValue)>,
}

impl<'a> Command<'a> {
    pub fn new(expression: &'a str) -> Command<'a> {
        Command {
            expression,
            parameter: None,
        }
    }
Repository: sharkdp/hyperfine
File: src/hyperfine/types.rs (Rust)

Code before:
            parameter: None,
        }
    }

    pub fn new_parametrized(
        expression: &'a str,
        parameter: &'a str,
        value: String,
    ) -> Command<'a> {
        Command {
            expression,
            parameter: Some((parameter, value)),
        }
    }

    pub fn get_shell_command(&self) -> String {
        match &self.parameter {
            Some((param_name, param_value)) => self.expression.replace(
                &format!("{{{param_name}}}", param_name = param_name),
                &param_value.to_string(),
            ),
            None => self.expression.into(),
        }
    }

    pub fn get_parameter(&self) -> &Option<(&'a str, String)> {
        &self.parameter
    }
}

impl<'a> fmt::Display for Command<'a> {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        write!(f, "{}", self.get_shell_command())
    }
}

/// Action to take when an executed command fails.
#[derive(Debug, Clone, Copy, PartialEq)]
pub enum CmdFailureAction {
    /// Exit with an error message
    RaiseError,

    /// Simply ignore the non-zero exit code
    Ignore,
}

/// Output style type option
#[derive(Debug, Clone, Copy, PartialEq)]
pub enum OutputStyleOption {
    /// Do not output with colors or any special formatting
    Basic,

Review comment (suggestion):
similar here, this could probably return a `&'a str` ref, instead of a `String`

Code after:

/// A command that should be benchmarked.
#[derive(Debug, Clone)]
pub struct Command<'a> {
    /// The command that should be executed (without parameter substitution)
    expression: &'a str,

    /// A possible parameter value.
    parameter: Option<(&'a str, ParameterValue)>,
}

impl<'a> Command<'a> {
    pub fn new(expression: &'a str) -> Command<'a> {
        Command {
            expression,
            parameter: None,
        }
    }

    pub fn new_parametrized(
        expression: &'a str,
        parameter: &'a str,
        value: ParameterValue,
    ) -> Command<'a> {
        Command {
            expression,
            parameter: Some((parameter, value)),
        }
    }

    pub fn get_shell_command(&self) -> String {
        match &self.parameter {
            Some((param_name, param_value)) => self.expression.replace(
                &format!("{{{param_name}}}", param_name = param_name),
                &param_value.to_string(),
            ),
            None => self.expression.into(),
        }
    }

    pub fn get_parameter(&self) -> &Option<(&'a str, ParameterValue)> {
        &self.parameter
    }
}

impl<'a> fmt::Display for Command<'a> {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        write!(f, "{}", self.get_shell_command())
    }
}
Repository: sharkdp/hyperfine
File: scripts/plot_whisker.py (Python)

Code before:

"""This program shows `hyperfine` benchmark results as a box and whisker plot.

Quoting from the matplotlib documentation:
    The box extends from the lower to upper quartile values of the data, with
    a line at the median. The whiskers extend from the box to show the range
    of the data. Flier points are those past the end of the whiskers.
"""

import argparse
import json
import numpy as np
import matplotlib.pyplot as plt
import matplotlib.cm as cm

parser = argparse.ArgumentParser(description=__doc__)
parser.add_argument("file", help="JSON file with benchmark results")
parser.add_argument("--title", help="Plot Title")
args = parser.parse_args()

with open(args.file) as f:
    results = json.load(f)["results"]

commands = [b["command"] for b in results]
times = [b["times"] for b in results]

boxplot = plt.boxplot(times, vert=True, patch_artist=True)
cmap = plt.cm.get_cmap("rainbow")
colors = [cmap(val / len(times)) for val in range(len(times))]

for patch, color in zip(boxplot["boxes"], colors):
    patch.set_facecolor(color)

plt.title(args.title)
plt.legend(handles=boxplot["boxes"], labels=commands, loc="best", fontsize="medium")
plt.ylabel("Time [s]")
plt.show()


Review comment (refactor):
Please do not remove this. I'm okay with adding a command-line option to specify this somehow, but I believe I would like to keep this the default.

Code after:

"""This program shows `hyperfine` benchmark results as a box and whisker plot.

Quoting from the matplotlib documentation:
    The box extends from the lower to upper quartile values of the data, with
    a line at the median. The whiskers extend from the box to show the range
    of the data. Flier points are those past the end of the whiskers.
"""

import argparse
import json
import matplotlib.pyplot as plt

parser = argparse.ArgumentParser(description=__doc__)
parser.add_argument("file", help="JSON file with benchmark results")
parser.add_argument("--title", help="Plot Title")
args = parser.parse_args()

with open(args.file) as f:
    results = json.load(f)["results"]

commands = [b["command"] for b in results]
times = [b["times"] for b in results]

boxplot = plt.boxplot(times, vert=True, patch_artist=True)
cmap = plt.cm.get_cmap("rainbow")
colors = [cmap(val / len(times)) for val in range(len(times))]

for patch, color in zip(boxplot["boxes"], colors):
    patch.set_facecolor(color)

plt.title(args.title)
plt.legend(handles=boxplot["boxes"], labels=commands, loc="best", fontsize="medium")
plt.ylabel("Time [s]")
plt.ylim(0, None)
plt.show()
Repository: sharkdp/hyperfine
File: scripts/plot_histogram.py (Python)

Code before:
#!/usr/bin/env python

"""This program shows `hyperfine` benchmark results as a histogram."""

import argparse
import json
import numpy as np
import matplotlib.pyplot as plt

parser = argparse.ArgumentParser(description=__doc__)
parser.add_argument("file", help="JSON file with benchmark results")
parser.add_argument("--title", help="Plot title")
parser.add_argument(
    "--labels", help="Comma-separated list of entries for the plot legend"
)
parser.add_argument("--bins", help="Number of bins (default: auto)")
parser.add_argument(
    "--type", help="Type of histogram (*bar*, barstacked, step, stepfilled)"
)
parser.add_argument("--imgname", help="Save image to the given filename.")
args = parser.parse_args()

with open(args.file) as f:
    results = json.load(f)["results"]

if args.labels:
    labels = args.labels.split(",")
else:
    labels = [b["command"] for b in results]
all_times = [b["times"] for b in results]

t_min = np.min(list(map(np.min, all_times)))
t_max = np.max(list(map(np.max, all_times)))

bins = int(args.bins) if args.bins else "auto"
histtype = args.type if args.type else "bar"

plt.hist(
    all_times, label=labels, bins=bins, histtype=histtype, range=(t_min, t_max),
)
plt.legend(prop={"family": ["Source Code Pro", "Fira Mono", "Courier New"]})

plt.xlabel("Time [s]")
if args.title:
    plt.title(args.title)

Review comment (suggestion):
I would prefer if we could name this `--output` and maybe provide a `-o` shortcut. That would be in analogy to programs like `curl` or `gcc`. 

Code after:
#!/usr/bin/env python

"""This program shows `hyperfine` benchmark results as a histogram."""

import argparse
import json
import numpy as np
import matplotlib.pyplot as plt

parser = argparse.ArgumentParser(description=__doc__)
parser.add_argument("file", help="JSON file with benchmark results")
parser.add_argument("--title", help="Plot title")
parser.add_argument(
    "--labels", help="Comma-separated list of entries for the plot legend"
)
parser.add_argument("--bins", help="Number of bins (default: auto)")
parser.add_argument(
    "--type", help="Type of histogram (*bar*, barstacked, step, stepfilled)"
)
parser.add_argument(
    "-o", "--output", help="Save image to the given filename."
)

args = parser.parse_args()

with open(args.file) as f:
    results = json.load(f)["results"]

if args.labels:
    labels = args.labels.split(",")
else:
    labels = [b["command"] for b in results]
all_times = [b["times"] for b in results]

t_min = np.min(list(map(np.min, all_times)))
t_max = np.max(list(map(np.max, all_times)))

bins = int(args.bins) if args.bins else "auto"
histtype = args.type if args.type else "bar"

plt.hist(
    all_times, label=labels, bins=bins, histtype=histtype, range=(t_min, t_max),
)
plt.legend(prop={"family": ["Source Code Pro", "Fira Mono", "Courier New"]})