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

Switch benchmarks to criterion #145

Merged
merged 3 commits into from
Apr 25, 2018
Merged

Switch benchmarks to criterion #145

merged 3 commits into from
Apr 25, 2018

Conversation

robinst
Copy link
Collaborator

@robinst robinst commented Apr 21, 2018

Advantages over the previous system:

  • Allows running on stable Rust
  • Compares benchmarks between runs using statistics

See https://github.com/japaric/criterion.rs for more.

Advantages over the previous system:

* Allows running on stable Rust
* Compares benchmarks between runs using statistics

See https://github.com/japaric/criterion.rs for more.

criterion_group!{
name = benches;
config = Criterion::default().sample_size(10);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that more samples is better, but it takes a much longer time to run. But this can be adjusted if the measured runtime varies too much between runs of cargo bench.

Copy link
Owner

@trishume trishume left a comment

Choose a reason for hiding this comment

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

Tried it out locally, it's pretty neat, I like it. One of the sample sizes needs fixing though.

And also for the highlighting and parsing tests, there's a bug in criterion where it writes the results to paths like target/criterion/parse/"testdata/highlight_test.erb"/new/sample.json expecting the quoted part to be one directory but it actually creates it as a "testdata directory with the rest inside it. I'm not sure if this can only be fixed in criterion, or if there's a workaround to not put slashes in the benchmark names.


criterion_group! {
name = benches;
config = Criterion::default().sample_size(100);
Copy link
Owner

Choose a reason for hiding this comment

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

This sample size is too high, the load_syntaxes benchmark has an estimated time of 10 minutes, and others are pretty long.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed!

@robinst
Copy link
Collaborator Author

robinst commented Apr 24, 2018

Good pickup with the results (I only looked at the output, not the files).

I'll look into it tomorrow and either raise an issue or a fix. To work around it in syntect we could instead use numerical indexes or simpler strings and then look up the test file in a Map. What do you think?

@trishume
Copy link
Owner

Yah a string map seems like a fine workaround, numerical indexes would probably lead to the files being numbers and thus harder to navigate.

So that the file paths in target are a bit less weird, see
bheisler/criterion.rs#142.
@robinst
Copy link
Collaborator Author

robinst commented Apr 25, 2018

Done using a match, and raised an issue for criterion: bheisler/criterion.rs#142

@trishume trishume merged commit 7243993 into master Apr 25, 2018
@trishume trishume deleted the criterion-benchmark branch April 25, 2018 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants