Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a cargo_criterion_support feature flag #541

Open
wants to merge 1 commit into
base: version-0.4
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 7 additions & 2 deletions Cargo.toml
Expand Up @@ -24,7 +24,6 @@ itertools = "0.10"
serde = "1.0"
serde_json = "1.0"
serde_derive = "1.0"
serde_cbor = "0.11"
atty = "0.2"
clap = { version = "2.33", default-features = false }
walkdir = "2.3"
Expand All @@ -34,6 +33,9 @@ num-traits = { version = "0.2", default-features = false, features = ["std"]
oorandom = "11.1"
regex = { version = "1.3", default-features = false, features = ["std"] }

# cargo-criterion support
serde_cbor = { version = "0.11", optional = true }

# Optional dependencies
rayon = { version = "1.3", optional = true }
csv = { version = "1.1", optional = true }
Expand Down Expand Up @@ -67,7 +69,7 @@ stable = [
"async_tokio",
"async_std",
]
default = ["rayon", "plotters", "cargo_bench_support"]
default = ["rayon", "plotters", "cargo_bench_support", "cargo_criterion_support"]

# Enable use of the nightly-only test::black_box function to discourage compiler optimizations.
real_blackbox = []
Expand All @@ -90,6 +92,9 @@ html_reports = []
# required in order to have Criterion.rs be usable outside of cargo-criterion.
cargo_bench_support = []

# This feature enables Criterion.rs to be usable with cargo-criterion.
cargo_criterion_support = ["serde_cbor"]

# This feature _currently_ does nothing, but in 0.4.0 it will be
# required in order to have Criterion.rs generate CSV files. This feature is deprecated in favor of
# cargo-criterion's --message-format=json option.
Expand Down
4 changes: 3 additions & 1 deletion src/analysis/mod.rs
Expand Up @@ -7,6 +7,7 @@ use crate::stats::univariate::Sample;
use crate::stats::{Distribution, Tails};

use crate::benchmark::BenchmarkConfig;
#[cfg(feature = "cargo_criterion_support")]
use crate::connection::OutgoingMessage;
use crate::estimate::{
build_estimates, ConfidenceInterval, Distributions, Estimate, Estimates, PointEstimates,
Expand Down Expand Up @@ -92,6 +93,7 @@ pub(crate) fn common<M: Measurement, T: ?Sized>(
iters = sample.1;
times = sample.2;

#[cfg(feature = "cargo_criterion_support")]
if let Some(conn) = &criterion.connection {
conn.send(&OutgoingMessage::MeasurementComplete {
id: id.into(),
Expand Down Expand Up @@ -247,7 +249,7 @@ pub(crate) fn common<M: Measurement, T: ?Sized>(
});
}

if criterion.connection.is_none() {
if !criterion.has_connection() {
if let Baseline::Save = criterion.baseline {
copy_new_dir_to_base(
id.as_directory_name(),
Expand Down
3 changes: 3 additions & 0 deletions src/benchmark_group.rs
@@ -1,5 +1,6 @@
use crate::analysis;
use crate::benchmark::PartialBenchmarkConfig;
#[cfg(feature = "cargo_criterion_support")]
use crate::connection::OutgoingMessage;
use crate::measurement::Measurement;
use crate::report::BenchmarkId as InternalBenchmarkId;
Expand Down Expand Up @@ -306,6 +307,7 @@ impl<'a, M: Measurement> BenchmarkGroup<'a, M> {

match self.criterion.mode {
Mode::Benchmark => {
#[cfg(feature = "cargo_criterion_support")]
if let Some(conn) = &self.criterion.connection {
if do_run {
conn.send(&OutgoingMessage::BeginningBenchmark { id: (&id).into() })
Expand Down Expand Up @@ -369,6 +371,7 @@ impl<'a, M: Measurement> Drop for BenchmarkGroup<'a, M> {
fn drop(&mut self) {
// I don't really like having a bunch of non-trivial code in drop, but this is the only way
// to really write linear types like this in Rust...
#[cfg(feature = "cargo_criterion_support")]
if let Some(conn) = &mut self.criterion.connection {
conn.send(&OutgoingMessage::FinishedBenchmarkGroup {
group: &self.group_name,
Expand Down
47 changes: 40 additions & 7 deletions src/lib.rs
Expand Up @@ -27,6 +27,11 @@
)
)]

#[cfg(not(any(feature = "cargo_bench_support", feature = "cargo_criterion_support")))]
compile_error!(
"At least one of [`cargo_bench_support`, `cargo_criterion_support`] must be enabled"
);

#[cfg(test)]
extern crate approx;

Expand Down Expand Up @@ -56,6 +61,7 @@ mod benchmark;
mod benchmark_group;
pub mod async_executor;
mod bencher;
#[cfg(feature = "cargo_criterion_support")]
mod connection;
#[cfg(feature = "csv_output")]
mod csv_report;
Expand All @@ -77,17 +83,19 @@ use std::cell::RefCell;
use std::collections::HashSet;
use std::default::Default;
use std::env;
#[cfg(feature = "cargo_criterion_support")]
use std::net::TcpStream;
use std::path::{Path, PathBuf};
use std::process::Command;
#[cfg(feature = "cargo_criterion_support")]
use std::sync::{Mutex, MutexGuard};
use std::time::Duration;

use criterion_plot::{Version, VersionError};

use crate::benchmark::BenchmarkConfig;
use crate::connection::Connection;
use crate::connection::OutgoingMessage;
#[cfg(feature = "cargo_criterion_support")]
use crate::connection::{Connection, OutgoingMessage};
use crate::html::Html;
use crate::measurement::{Measurement, WallTime};
#[cfg(feature = "plotters")]
Expand Down Expand Up @@ -123,6 +131,9 @@ lazy_static! {
PlottingBackend::None
}
};
}
#[cfg(feature = "cargo_criterion_support")]
lazy_static! {
static ref CARGO_CRITERION_CONNECTION: Option<Mutex<Connection>> = {
match std::env::var("CARGO_CRITERION_PORT") {
Comment on lines +135 to 138
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, when cargo_criterion_support is not enabled, criterion behaves as if cargo-criterion does not exist. It might be useful to check somewhere whether the CARGO_CRITERION_PORT env variable is set, and print a warning or error if cargo_criterion_support is not enabled. This wouldn't be the right place; it would need to go in the benchmark's main function (similar to the previous warnings when cargo_bench_support was not enabled).

Ok(port_str) => {
Expand All @@ -133,6 +144,8 @@ lazy_static! {
Err(_) => None,
}
};
}
lazy_static! {
static ref DEFAULT_OUTPUT_DIRECTORY: PathBuf = {
// Set criterion home to (in descending order of preference):
// - $CRITERION_HOME (cargo-criterion sets this, but other users could as well)
Expand Down Expand Up @@ -344,6 +357,7 @@ pub struct Criterion<M: Measurement = WallTime> {
all_titles: HashSet<String>,
measurement: M,
profiler: Box<RefCell<dyn Profiler>>,
#[cfg(feature = "cargo_criterion_support")]
connection: Option<MutexGuard<'static, Connection>>,
mode: Mode,
}
Expand Down Expand Up @@ -411,13 +425,14 @@ impl Default for Criterion {
all_titles: HashSet::new(),
measurement: WallTime,
profiler: Box::new(RefCell::new(ExternalProfiler)),
#[cfg(feature = "cargo_criterion_support")]
connection: CARGO_CRITERION_CONNECTION
.as_ref()
.map(|mtx| mtx.lock().unwrap()),
mode: Mode::Benchmark,
};

if criterion.connection.is_some() {
if criterion.has_connection() {
// disable all reports when connected to cargo-criterion; it will do the reporting.
criterion.report.cli_enabled = false;
criterion.report.bencher_enabled = false;
Expand All @@ -429,6 +444,21 @@ impl Default for Criterion {
}

impl<M: Measurement> Criterion<M> {
/// Private helper that returns true if we are running under `cargo-criterion`.
///
/// Use this function if you have logic that is conditional on `cargo-criterion` being
/// used (or not), but that should always be compiled in. For code that should only be
/// conditionally compiled, use `#[cfg(feature = "cargo_criterion_support")]`.
fn has_connection(&self) -> bool {
#[cfg(not(feature = "cargo_criterion_support"))]
{
false
}

#[cfg(feature = "cargo_criterion_support")]
self.connection.is_some()
}

/// Changes the measurement for the benchmarks run with this runner. See the
/// Measurement trait for more details
pub fn with_measurement<M2: Measurement>(self, m: M2) -> Criterion<M2> {
Expand All @@ -445,6 +475,7 @@ impl<M: Measurement> Criterion<M> {
all_titles: self.all_titles,
measurement: m,
profiler: self.profiler,
#[cfg(feature = "cargo_criterion_support")]
connection: self.connection,
mode: self.mode,
}
Expand Down Expand Up @@ -611,7 +642,7 @@ impl<M: Measurement> Criterion<M> {
/// Enables plotting
pub fn with_plots(mut self) -> Criterion<M> {
// If running under cargo-criterion then don't re-enable the reports; let it do the reporting.
if self.connection.is_none() && self.report.html.is_none() {
if !self.has_connection() && self.report.html.is_none() {
let default_backend = DEFAULT_PLOTTING_BACKEND.create_plotter();
if let Some(backend) = default_backend {
self.report.html = Some(Html::new(backend));
Expand Down Expand Up @@ -827,7 +858,7 @@ https://bheisler.github.io/criterion.rs/book/faq.html
")
.get_matches();

if self.connection.is_some() {
if self.has_connection() {
if let Some(color) = matches.value_of("color") {
if color != "auto" {
eprintln!("Warning: --color will be ignored when running with cargo-criterion. Use `cargo criterion --color {} -- <args>` instead.", color);
Expand Down Expand Up @@ -889,6 +920,7 @@ https://bheisler.github.io/criterion.rs/book/faq.html
};

// This is kind of a hack, but disable the connection to the runner if we're not benchmarking.
#[cfg(feature = "cargo_criterion_support")]
if !self.mode.is_benchmark() {
self.connection = None;
}
Expand Down Expand Up @@ -921,7 +953,7 @@ https://bheisler.github.io/criterion.rs/book/faq.html
self.baseline_directory = dir.to_owned();
}

if self.connection.is_some() {
if self.has_connection() {
// disable all reports when connected to cargo-criterion; it will do the reporting.
self.report.cli_enabled = false;
self.report.bencher_enabled = false;
Expand Down Expand Up @@ -1057,7 +1089,7 @@ https://bheisler.github.io/criterion.rs/book/faq.html
/// Returns true iff we should save the benchmark results in
/// json files on the local disk.
fn should_save_baseline(&self) -> bool {
self.connection.is_none()
!self.has_connection()
&& self.load_baseline.is_none()
&& !matches!(self.baseline, Baseline::Discard)
}
Expand Down Expand Up @@ -1089,6 +1121,7 @@ https://bheisler.github.io/criterion.rs/book/faq.html
let group_name = group_name.into();
assert!(!group_name.is_empty(), "Group name must not be empty.");

#[cfg(feature = "cargo_criterion_support")]
if let Some(conn) = &self.connection {
conn.send(&OutgoingMessage::BeginningBenchmarkGroup { group: &group_name })
.unwrap();
Expand Down
5 changes: 4 additions & 1 deletion src/routine.rs
@@ -1,4 +1,5 @@
use crate::benchmark::BenchmarkConfig;
#[cfg(feature = "cargo_criterion_support")]
use crate::connection::OutgoingMessage;
use crate::measurement::Measurement;
use crate::report::{BenchmarkId, Report, ReportContext};
Expand Down Expand Up @@ -37,7 +38,7 @@ pub(crate) trait Routine<M: Measurement, T: ?Sized> {
.profile(id, report_context, time.as_nanos() as f64);

let mut profile_path = report_context.output_directory.clone();
if (*crate::CARGO_CRITERION_CONNECTION).is_some() {
if criterion.has_connection() {
// If connected to cargo-criterion, generate a cargo-criterion-style path.
// This is kind of a hack.
profile_path.push("profile");
Expand Down Expand Up @@ -95,6 +96,7 @@ pub(crate) trait Routine<M: Measurement, T: ?Sized> {
.report
.warmup(id, report_context, wu.as_nanos() as f64);

#[cfg(feature = "cargo_criterion_support")]
if let Some(conn) = &criterion.connection {
conn.send(&OutgoingMessage::Warmup {
id: id.into(),
Expand Down Expand Up @@ -140,6 +142,7 @@ pub(crate) trait Routine<M: Measurement, T: ?Sized> {
.report
.measurement_start(id, report_context, n, expected_ns, total_iters);

#[cfg(feature = "cargo_criterion_support")]
if let Some(conn) = &criterion.connection {
conn.send(&OutgoingMessage::MeasurementStart {
id: id.into(),
Expand Down