Version 0.4.0 by lemmih · Pull Request #496 · bheisler/criterion.rs

lemmih · 2021-07-26T07:18:56Z

Changes:

The Criterion::can_plot function has been removed.
The Criterion::bench_function_over_inputs function has been removed.
The Criterion::bench_functions function has been removed.
The Criterion::bench function has been removed.
--feature csv_output is now required for csv output.
--feature html_reports is now required for html output.
Added a --discard-baseline flag.
rayon and plotters are optional (and default) dependencies.
Status messages ('warming up', 'analyzing', etc) are printed to stderr, benchmark results are printed to stdout.
Terminal escape codes are handled by the anes crate.
Accept subsecond durations as command-line options.
Formally support WASI (and automatically test for wasi regressions).
Add --quiet flag (for printing a single line per benchmark).
Replace serde_cbor with ciborium.
Upgrade to clap v3.
Bump regex to version 1.5.
Bump MSRV to 1.56.1.

* Disable warnings about HTML reports and standalone support. * Include 'html_reports' in the set of stable features. * Suppress html and csv if the feature flags are missing. * Make the CSV dependency optional. * Group optional dependencies in cargo manifest. * Test both with all features and no features.

Some tools (e.g., rhysd/github-actions-benchmark) that parse the output of criterion do not correctly parse the standard output if it contains an unexpected string, such as an error message. The fix in this commit ensures that the tool will work correctly in environments where gnuplot is not installed (e.g., GitHub Actions runners). Also, from the conventions of the command line interface, it is preferable that such error messages be output to stderr.

* Make the plotters dependency optional. * Turn rayon into an optional dependency. * Add cli flag for discarding baselines rather than saving them to disk. * Disable plotting if html_reports is disabled. * Support testing with and without the html_reports flag.

) * Use terminal code to clear the line. * Print status messages to stderr. Status messages such as 'Warming up', 'Analyzing', etc. * Use the 'anes' library for handling terminal escape codes.

* Accept subsecond durations from the command line. * Replace 'to_nanos' with 'as_nanos'. 'as_nanos' stabilized in 1.33.

…505)

* Add --quiet flag. When enabled, only a single line is printed per benchmark. * Print warnings and errors to stderr.

briansmith · 2021-08-26T22:38:35Z

@@ -17,6 +17,7 @@ license     = "Apache-2.0/MIT"
 exclude     = ["book/*"]


I think it would be good to set version = "0.4.0" now.

Would that make it easier for people to test this branch?

It was easier for me to test with it left as-is, because I could use [patch.crates-io] to insert it into other dependencies like criterion-cycles-per-byte and pprof.

briansmith · 2021-08-26T22:45:38Z

@@ -26,21 +27,24 @@ serde_derive   = "1.0"
 serde_cbor     = "0.11"
 atty           = "0.2"
 clap           = { version = "2.33", default-features = false }


My initial feedback: This branch with default-features=false and opting into no optional features is a drop-in replacement for 0.3.4 for me insofar as my benchmarks build and run using this version, simply with this change:

[dev-dependencies] -criterion = "0.3.5" +criterion = { git = "https://github.com/bheisler/criterion.rs", branch = "version-0.4", commit = "7b8030b1223adacebf81b8e1cb3286a83e7a2470", default-features=false}

How practical would it be to move the anes, atty, clap, criterion-plot, and serde_cbor (at least) dependencies out of the "no default features" configuration?

Here's the cargo-tree of dependencies with default-features=false:

│ ├── anes v0.1.6 │ ├── atty v0.2.14 │ │ └── winapi v0.3.9 │ ├── cast v0.2.7 │ │ [build-dependencies] │ │ └── rustc_version v0.4.0 │ │ └── semver v1.0.4 │ ├── clap v2.33.3 │ │ ├── bitflags v1.3.2 │ │ ├── textwrap v0.11.0 │ │ │ └── unicode-width v0.1.8 │ │ └── unicode-width v0.1.8 │ ├── criterion-plot v0.4.3 (https://github.com/bheisler/criterion.rs?branch=version-0.4#7b8030b1) │ │ ├── cast v0.2.7 (*) │ │ └── itertools v0.10.1 │ │ └── either v1.6.1 │ ├── itertools v0.10.1 (*) │ ├── lazy_static v1.4.0 │ ├── num-traits v0.2.14 │ │ [build-dependencies] │ │ └── autocfg v1.0.1 │ ├── oorandom v11.1.3 │ ├── regex v1.5.4 │ │ └── regex-syntax v0.6.25 │ ├── serde v1.0.128 │ ├── serde_cbor v0.11.2 │ │ ├── half v1.7.1 │ │ └── serde v1.0.128 │ ├── serde_derive v1.0.128 (proc-macro) │ │ ├── proc-macro2 v1.0.28 │ │ │ └── unicode-xid v0.2.2 │ │ ├── quote v1.0.9 │ │ │ └── proc-macro2 v1.0.28 (*) │ │ └── syn v1.0.75 │ │ ├── proc-macro2 v1.0.28 (*) │ │ ├── quote v1.0.9 (*) │ │ └── unicode-xid v0.2.2 │ ├── serde_json v1.0.66 │ │ ├── itoa v0.4.8 │ │ ├── ryu v1.0.5 │ │ └── serde v1.0.128 │ ├── tinytemplate v1.2.1 │ │ ├── serde v1.0.128 │ │ └── serde_json v1.0.66 (*) │ └── walkdir v2.3.2 │ ├── same-file v1.0.6 │ │ └── winapi-util v0.1.5 │ │ └── winapi v0.3.9 │ ├── winapi v0.3.9 │ └── winapi-util v0.1.5 (*)

Some comments:

│ ├── cast v0.2.7 │ │ [build-dependencies] │ │ └── rustc_version v0.4.0 │ │ └── semver v1.0.4

Not sure that cast is really carrying its weight considering the overhead it adds to the build. Either we could try to remove cast or we could work with the cast developers to simplify its build dependencies; I filed japaric/cast.rs#35 to that effect.

│ ├── clap v2.33.3 │ │ ├── bitflags v1.3.2 │ │ ├── textwrap v0.11.0 │ │ │ └── unicode-width v0.1.8 │ │ └── unicode-width v0.1.8

I don't fully understand how important clap::value_t is or how difficult it would be to replace, but I noticed if we could avoid using clap::value_t then we wouldn't need to depend on clap when the cargo-clippy feature isn't selected. So I think it is worth trying to find an alternative to value_t.

│ ├── serde v1.0.128 │ ├── serde_cbor v0.11.2 │ │ ├── half v1.7.1 │ │ └── serde v1.0.128 │ ├── serde_derive v1.0.128 (proc-macro) │ │ ├── proc-macro2 v1.0.28 │ │ │ └── unicode-xid v0.2.2 │ │ ├── quote v1.0.9 │ │ │ └── proc-macro2 v1.0.28 (*) │ │ └── syn v1.0.75 │ │ ├── proc-macro2 v1.0.28 (*) │ │ ├── quote v1.0.9 (*) │ │ └── unicode-xid v0.2.2 │ ├── serde_json v1.0.66 │ │ ├── itoa v0.4.8 │ │ ├── ryu v1.0.5 │ │ └── serde v1.0.128

Serde is a very heavy dependency. Granted, most of my projects use it anyway, so eliminating serde and serde-derive wouldn't have as wide an impact as removing other dependencies. However, I think finding a way to remove serde_cbor or serde_json (perhaps just default to one or the other) or both (In my CI jobs, I don't want any serializatoin at all since I don't do anything with serialized output; I just want to verify that the benchmarks still build and run) would be a good win.

│ └── walkdir v2.3.2 │ ├── same-file v1.0.6 │ │ └── winapi-util v0.1.5 │ │ └── winapi v0.3.9 │ ├── winapi v0.3.9 │ └── winapi-util v0.1.5 (*)

Again, in the pretty common situation where in CI where we're only verifying that the benchmarks compile and run correctly, and where we don't want any persistence anyway, we should be able to eliminate all the functionality that does file I/O and then eliminate this pretty heavy dependency.

It should be possible to make serde an optional dependency and have functionality degrade gracefully (ie. running/testing benchmarks should still work but saving/loading baselines would print an error message). I'll gladly review such a PR.

How practical would it be to move the anes, atty, clap, criterion-plot, and serde_cbor (at least) dependencies out of the "no default features" configuration?

criterion-plot is scheduled to be removed entirely (either in version 0.4 or 0.5). serde could be optional but it would take a fair bit of work/refactoring. I guess anes and atty could be optional but they're so tiny that I don't really see the point. clap is quite vital, though, and I don't see how criterion could work without it.

I'll gladly review such a PR.

OK, when I have time, I will try that. Will you be merging this PR (#496) soon? It would be easier for me to make my PRs on top of this one.

I'll gladly review such a PR.

OK, when I have time, I will try that. Will you be merging this PR (#496) soon? It would be easier for me to make my PRs on top of this one.

Probably won't be merged any time soon. You can submit your PR directly against this branch: Checkout version-0.4, push a new branch, and create a PR (or draft PR) with the target being version-0.4 from the drop-down menu.

I had intended for command-line parsing and output and the like to start shifting over to cargo-criterion with the 0.4 release, so I think clap and anes and related crates should be behind the cargo-bench-support feature. That code would need to still be available in criterion-rs for the time being to give users time to migrate over to cargo-criterion (that's the purpose of the cargo-bench-support feature).

briansmith · 2021-08-26T22:53:56Z

With the PR as it is today, here is the effect on one of my projects:

+name = "anes"
-name = "bstr"
-name = "crossbeam-channel"
-name = "crossbeam-deque"
-name = "crossbeam-epoch"
-name = "crossbeam-utils"
-name = "csv"
-name = "csv-core"
-name = "memchr"
-name = "memoffset"
-name = "num_cpus"
-name = "plotters"
-name = "plotters-backend"
-name = "plotters-svg"
-name = "rayon"
-name = "rayon-core"
-name = "regex-automata"
-name = "scopeguard"

A net reduction of 16 dependencies.

briansmith · 2021-09-05T18:48:53Z

cast.rs 0.3.0 has been released with fewer dependencies.

str4d · 2021-12-27T17:43:52Z

I tested db880ab on several of my projects, by adding the following patch:

[patch.crates-io]
criterion = { git = "https://github.com/bheisler/criterion.rs", rev = "db880abe2ff8b3c1c208d2e8ead4e21fbe1e05a4" }

No issues at all; it's a drop-in replacement and I didn't have to alter any code. Even the projects that use criterion-cycles-per-byte, or the criterion feature flag of pprof, continue to work fine.

I also confirmed (after removing criterion-cycles-per-byte and pprof integrations) that after replacing my criterion 0.3 dependency with a direct git revision and default-features = false, the benchmarks continued to compile and run, and I saw a similar drop in dependencies as above.

str4d · 2021-12-27T18:34:52Z

#497 confuses me: it says that the cargo_bench_support feature flag is now honored, but all the PR does is remove the warning. cargo_bench_support has never done anything (it was added for forward-compatibility purposes), so the necessary logic still needs to be implemented.

I noticed this while looking into making serde optional: serde_cbor is a necessary component of cargo-criterion support, so a user like @briansmith who doesn't want serde therefore can't use cargo-criterion, and I guess would need to have cargo_bench_support enabled.

Since feature flags should always be additive, I think that implies we should also have a cargo_criterion_support feature flag, and then require that at least one of the two is enabled (otherwise there is no way to run benchmarks). I don't have the criterion repo knowledge to implement cargo_bench_support, but I'll look at adding cargo_criterion_support.

str4d · 2021-12-27T19:08:54Z

Opened #541 to add cargo_criterion_support to this PR.

… not be found

* Enable --quick mode. * Add basic user guide for the new 'quick mode'.

* Document WASM support with wasmer, wasmtime, nodejs, and browsers.

* Bump MSRV to 1.56 as required by ciborium

Clap v3 is a new major version of clap with several deprecations and breaking changes, all of which are addressed within this commit.

bheisler · 2022-07-06T23:11:47Z

OK, so I've taken a look over this. Most of it looks great.

I'm a bit skeptical about the quick mode, but I could be convinced if people find it useful and reliable. If it's going to be added to criterion-rs support should also be added to cargo-criterion as well.

I'm a lot skeptical about vendoring critcmp, especially into this repo as opposed to cargo-criterion. Do we really want to take on the burden of maintaining and supporting critcmp at all? I suppose that we can use critcmp's source under its license, but BurntSushi seems to still be maintaining it and we'd presumably have to vendor in all of his changes forevermore. Additionally, one of my longer-term goals was to move as much functionality as practical out of criterion-rs and into cargo-criterion to reduce the compile- and link-time burden on running benchmarks. None of this is core to the process of doing the measurements, so it seems to fit better in cargo-criterion (whose job is storing and reporting the measurements) than it does here.

If we are going to add this feature, I'd rather work with BurntSushi to have just one copy of that code. Would he be open to providing it as a library crate we can pull in? Or, a hacky sort of solution, could we have cargo-criterion compare just shell out to critcmp instead of vendoring it?

Many of these changes should be made to cargo-criterion as well, like the changes to command-line output, but that doesn't need to block merging them into criterion-rs.

lemmih · 2022-07-08T19:14:55Z

BurntSushi doesn't want to maintain the code (neither as a library or as an executable). If we want the functionality to be part of the documented workflow for criterion then we have to maintain it ourselves. I'm volunteering to be responsible for it.

You're completely right that it doesn't belong in criterion.rs. How would you feel about me maintaining it as part of cargo-criterion? I think it's either that or we delete it and don't mention it in the documentation.

bheisler · 2022-07-14T21:50:21Z

Hm, I didn't realize it was abandoned.

One of the long-term problems I've had in maintaining Criterion-rs is that - since I haven't been writing (or benchmarking) much Rust code aside from sporadically maintaining Criterion - I don't really feel like I have a strong vision or opinion on how the workflow should evolve. Well, aside from introducing cargo-criterion to reduce compile- and link-time, as previously mentioned.

If you think critcmp is a worthy addition to that workflow, I'd be okay with merging it into cargo-criterion.

CosmicHorrorDev

Just a couple of things I noticed when working on #599

CosmicHorrorDev · 2022-07-17T02:22:51Z

+                .help("Limit the baselines used in tabulated results.")
+                .help(""))


Suggested change

.help("Limit the baselines used in tabulated results.")

.help(""))

.help("Limit the baselines used in tabulated results."))

CosmicHorrorDev · 2022-07-17T02:29:10Z

+            };
+
+            let args = critcmp::app::Args {
+                baselines: matches.values_of_lossy("baselines").unwrap_or_default(),


Parsing this causes a panic at runtime

thread 'main' panicked at 'Must use `Arg::allow_invalid_utf8` with `_os` lookups at `[hash: 570E2D9D9139F734]`', /.../cargo/registry/src/github.com-1ecc6299db9ec823/clap-3.2.12/src/parser/matches/arg_matches.rs:1732:13 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

This may be one of the errors that gets caught by debug_assert()

* Delete critcmp code (it belongs in cargo-criterion) * Bump MSRV to 1.57 due to os_str_bytes. * Mention MSRV bump in CHANGELOG.

lemmih · 2022-08-23T13:38:49Z

@bheisler I removed the critcmp code. Let me know if there's anything else we need to look at before 0.4 can be released.

bheisler · 2022-08-29T23:56:39Z

Only nice-to-haves, I think. I'll try and check over everything and hopefully publish 0.4.0 this weekend.

bheisler · 2022-09-06T03:11:20Z

Unfortunately a bunch of things came up and I was not able to get to it during the weekend, but I haven't forgotten.

jhpratt · 2022-09-06T03:39:32Z

No worries! Things happen — I'm sure everyone can understand that.

bheisler · 2022-09-10T17:53:14Z

Thanks for your patience, everyone. I've merged and published 0.4.0.

mati865 · 2022-09-10T20:01:43Z

 [0.3.4]: https://github.com/bheisler/criterion.rs/compare/0.3.3...0.3.4
 [0.3.5]: https://github.com/bheisler/criterion.rs/compare/0.3.4...0.3.5
-[0.3.5]: https://github.com/bheisler/criterion.rs/compare/0.3.5...0.3.6
+[0.3.5]: https://github.com/bheisler/criterion.rs/compare/0.3.5...0.3.6


This should be [0.3.6]

lemmih and others added 4 commits July 26, 2021 15:16

Remove deprecated functions.

f013a5d

Run CI for pull requests to the version-0.4 branch.

0ef6c82

lemmih mentioned this pull request Jul 26, 2021

Delete deprecated items #462

Open

lemmih added 9 commits July 26, 2021 23:14

Don't try to copy CSV files that weren't generated. (#499)

e8bb66d

Use terminal code, send results to stdout, and messages to stderr. (#500

4b80187

) * Use terminal code to clear the line. * Print status messages to stderr. Status messages such as 'Warming up', 'Analyzing', etc. * Use the 'anes' library for handling terminal escape codes.

Accept subsecond durations from the command line. (#501)

43f9751

* Accept subsecond durations from the command line. * Replace 'to_nanos' with 'as_nanos'. 'as_nanos' stabilized in 1.33.

Remove the deprecated 'iter_with_large_setup'. (#502)

c1b3466

Test WASI functionality in CI run. (#503)

6d3161e

Remove a few unsafe ops since the compiler can prove they are safe. (#…

f862a1b

…505)

Fix trailing semicolon in macros.

791a97f

Add --quiet flag for printing a single line per benchmark. (#510)

7b8030b

* Add --quiet flag. When enabled, only a single line is printed per benchmark. * Print warnings and errors to stderr.

briansmith reviewed Aug 26, 2021

View reviewed changes

Merge branch 'master' into version-0.4

db880ab

crepererum mentioned this pull request Nov 30, 2021

serde_cbor is unmaintained (RUSTSEC-2021-0127) #534

Closed

pmnoxx mentioned this pull request Dec 15, 2021

feat: Use criterion instead of bencher near/nearcore#5791

Merged

rukai and others added 3 commits December 28, 2021 11:53

Add flag to allow baseline comparisons to succeed when a baseline can…

77f1b38

… not be found

Merge branch 'master' into version-0.4

6d4eff5

Enable --quick mode. (#535)

412591e

* Enable --quick mode. * Add basic user guide for the new 'quick mode'.

conorbros mentioned this pull request Jan 10, 2022

Fix CI failures when a new benchmark is added shotover/shotover-proxy#403

Merged

lemmih added 2 commits February 13, 2022 12:11

Compare baselines with critcmp. (#540)

a9c42f0

Improved WASI support (#550)

08bdb9a

* Document WASM support with wasmer, wasmtime, nodejs, and browsers.

lemmih added 2 commits February 13, 2022 19:13

WASI: Update missing firefox results.

6929f48

Merge branch 'master' into version-0.4

4b37d8a

teoxoy reviewed Mar 1, 2022

View reviewed changes

Comment thread Cargo.toml Outdated

crepererum and others added 10 commits July 2, 2022 12:35

replace serde_cbor with ciborium (#560)

d4593f4

* Bump MSRV to 1.56 as required by ciborium

Upgrade to clap v3 (#543)

a00c592

Clap v3 is a new major version of clap with several deprecations and breaking changes, all of which are addressed within this commit.

Merge branch 'master' into version-0.4

f73aebb

Merge branch 'master' into version-0.4

c334339

Bump MSRV to 1.56.1

1a66bb7

Set required clap parameter.

5cb91ab

bump the regex to 1.5 (#525)

6127032

Merge branch 'master' into version-0.4

77e4ae2

Bump minimum version of 'anes' to 0.1.4

758b324

Update CHANGELOG with v0.4 changes.

ecfd1d1

lemmih marked this pull request as ready for review July 2, 2022 14:49

lemmih changed the title ~~[WIP] Version 0.4.0~~ Version 0.4.0 Jul 2, 2022

Add missing black_box for bench_with_input parameters. Fixes 566.

2934163

CosmicHorrorDev suggested changes Jul 17, 2022

View reviewed changes

lemmih added 2 commits August 23, 2022 15:00

Merge branch 'master' into version-0.4

a18d080

Remove critcmp code (it belongs in cargo-criterion) (#610)

f82ce59

* Delete critcmp code (it belongs in cargo-criterion) * Bump MSRV to 1.57 due to os_str_bytes. * Mention MSRV bump in CHANGELOG.

Add Throughput::BytesDecimal. Fixes #581.

935c632

bheisler merged commit 935c632 into master Sep 10, 2022

mati865 reviewed Sep 10, 2022

View reviewed changes

		@@ -17,6 +17,7 @@ license = "Apache-2.0/MIT"
		exclude = ["book/*"]

		.help("Limit the baselines used in tabulated results.")
		.help(""))

Conversation

lemmih commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briansmith commented Aug 26, 2021

Uh oh!

briansmith commented Sep 5, 2021

Uh oh!

str4d commented Dec 27, 2021

Uh oh!

str4d commented Dec 27, 2021

Uh oh!

str4d commented Dec 27, 2021

Uh oh!

Uh oh!

bheisler commented Jul 6, 2022

Uh oh!

lemmih commented Jul 8, 2022

Uh oh!

bheisler commented Jul 14, 2022

Uh oh!

CosmicHorrorDev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lemmih commented Aug 23, 2022

Uh oh!

bheisler commented Aug 29, 2022

Uh oh!

bheisler commented Sep 6, 2022

Uh oh!

jhpratt commented Sep 6, 2022

Uh oh!

bheisler commented Sep 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

lemmih commented Jul 26, 2021 •

edited

Loading