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

rand_distr: Port benchmarks to Criterion #1116

Merged
merged 1 commit into from May 25, 2021
Merged

Conversation

vks
Copy link
Collaborator

@vks vks commented Apr 25, 2021

  • The benchmarks are now living in their own crate. Therefore, this does
    not add any dev-dependencies to rand_distr.
  • Instead of bytes per seconds, we now measure cycles per byte.

Refs #1039.

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Nice!

Maybe benches should be a top-level crate instead of under rand_distr?

@dhardy
Copy link
Member

dhardy commented Apr 26, 2021

This is how you're supposed to use it? $ cargo run --release

$ cargo +nightly bench
<snip>
exp/exp                 time:   [14380.2573 cycles 14551.8306 cycles 14750.6125 cycles]
                        thrpt:  [1.8438 cpb 1.8190 cpb 1.7975 cpb]
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild
exp/exp1_specialized    time:   [13641.4057 cycles 13718.3708 cycles 13800.0997 cycles]          
                        thrpt:  [1.7250 cpb 1.7148 cpb 1.7052 cpb]
Found 11 outliers among 100 measurements (11.00%)
  4 (4.00%) high mild
  7 (7.00%) high severe
...

@newpavlov
Copy link
Member

Note that criterion is a quite heavy dependency, so I also think it's worth to keep these benchmarks in a separate crate. Otherwise, CI times may rise significantly.

@vks
Copy link
Collaborator Author

vks commented Apr 26, 2021

@dhardy Yes, this could be a top-level crate, but I wanted to get feedback before converting the other benchmarks. (There are top-level rand benchmarks already occupying the benches folder.)

As you already figured out, you can run them with cargo bench. Nightly is only required because of #![rustfmt::skip].

@newpavlov They are already in a separate crate, and not checked by CI. In fact, I implemented the benchmarks based on the way it was done in the RustCrypto crates. The next version of Criterion will make the HTML reports optional, which hopefully turns it into a lightweight dependency. If we are still worried about CI runtime, I think caching can solve this issue.

@vks vks added the D-review Do: needs review label Apr 30, 2021
Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Sorry, I guess I should approve this.

- The benchmarks are now living in their own crate. Therefore, this does
  not add any dev-dependencies to rand_distr.
- Instead of bytes per seconds, we now measure cycles per byte.

Refs rust-random#1039.
@vks vks merged commit 2732f2d into rust-random:master May 25, 2021
@vks vks deleted the criterion branch May 25, 2021 01:42
@vks
Copy link
Collaborator Author

vks commented May 25, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D-review Do: needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants