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

Introduce benchmarking using Criterion. #110

Merged
merged 4 commits into from
Sep 6, 2018
Merged

Introduce benchmarking using Criterion. #110

merged 4 commits into from
Sep 6, 2018

Conversation

Hoverbear
Copy link
Contributor

Starts on #109.

This demonstrates Criterion, ways to bench cleanly, and documentation how to use it.

@Hoverbear Hoverbear added the Enhancement An improvement to existing code. label Aug 27, 2018
@Hoverbear Hoverbear added this to the 0.4.0 milestone Aug 27, 2018
@Hoverbear Hoverbear self-assigned this Aug 27, 2018
@Hoverbear Hoverbear force-pushed the add-benches branch 8 times, most recently from a0238b0 to 58246d2 Compare August 28, 2018 06:11
mod suites;
use suites::*;

criterion_main!(
Copy link
Contributor

Choose a reason for hiding this comment

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

the default sample size for the criterion is 100, maybe we can use a smaller sample size like 10 to reduce the benchmark time.

@siddontang
Copy link
Contributor

CI failed @Hoverbear

@Hoverbear Hoverbear force-pushed the add-benches branch 4 times, most recently from cea2462 to 75625ba Compare September 4, 2018 19:59
@Hoverbear Hoverbear force-pushed the add-benches branch 2 times, most recently from e9a2340 to ecc28dd Compare September 5, 2018 00:13
@siddontang
Copy link
Contributor

PTAL @breeswish

@siddontang
Copy link
Contributor

LGTM

Copy link
Member

@breezewish breezewish left a comment

Choose a reason for hiding this comment

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

Nice job! Your code arrangement is pretty well :) Mostly LGTM!


fn main() {
criterion::init_logging();
let _ = env_logger::try_init();
Copy link
Member

Choose a reason for hiding this comment

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

Seems to be duplicated? criterion::init_logging will use a SimpleLogger to initialize log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh nice catch. :)

src/eraftpb.rs Outdated Show resolved Hide resolved
@Hoverbear
Copy link
Contributor Author

@breeswish Thanks, I resolved your comment. PTAL again?

Copy link
Member

@breezewish breezewish left a comment

Choose a reason for hiding this comment

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

LGTM!

@siddontang siddontang merged commit 70beb2c into master Sep 6, 2018
@siddontang siddontang deleted the add-benches branch September 6, 2018 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement An improvement to existing code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants