Use criterion.rs instead of libtest for benchmarks.
#119
Conversation
Since Criterion can only benchmark public API, these changes just drop all internal benchmarks (e.g., benchmarks for field operations). But those are usually microbenchmarks whose meaning is kind of questionable anyways, so I don't think this is a big loss. The `bench` feature disappears, since Criterion works on stable Rust.
|
Everything else looks good though. |
| @@ -60,7 +65,6 @@ default = ["std"] | |||
| std = ["rand", "subtle/std"] | |||
| alloc = [] | |||
| yolocrypto = ["avx2_backend"] | |||
| bench = [] | |||
There was a problem hiding this comment.
If we remove the "bench" feature, will all the dev-dependencies being pulled in by criterion also be required when we run cargo test? Also this is going to be a lot of code to pull in, it'd be nice to have a way to turn it off?
There was a problem hiding this comment.
Yes, all the dev dependencies are pulled in for tests and benchmarks, but are excluded from library users. I would rather not have a bench feature -- I'm glad to be able to get rid of it.
There was a problem hiding this comment.
As long as someone is using curve25519-dalek as a library, they won't be using any of its dev dependencies, only its actual dependencies. So feature-gating them would only save code for people who want to make changes to curve25519-dalek itself, but don't want to run benchmarks. Since the dependencies are cached, this is basically a one-time savings.
isislovecruft
dismissed
their stale review
Doesn't apply when running cargo test.
Removing double 'does'
Fix `no std` by moving `get_random` feat dev-deps
Since Criterion can only benchmark public API, these changes just drop
all internal benchmarks (e.g., benchmarks for field operations). But
those are usually microbenchmarks whose meaning is kind of questionable
anyways, so I don't think this is a big loss.
The
benchfeature disappears, since Criterion works on stable Rust.