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

Benchmarking with Criterion.rs #156

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

RobertJacobsonCDC
Copy link
Collaborator

  • Added Criterion.rs and criterion.toml for cargo-criterion.
  • Created benches directory, example_births_deaths, example_basic_infection benchmarks.
  • Wrote a README.md explaining how to run benchmarks and profile with samply.

@RobertJacobsonCDC
Copy link
Collaborator Author

Could someone verify whether the pre-commit failure to compile issue is spurious? It works on my machine, so I wonder if it just doesn't know what to do with the Criterion benchmark code.

@k88hudson-cfa
Copy link
Collaborator

@RobertJacobsonCDC I can reproduce locally running clippy with --all-targets

@RobertJacobsonCDC RobertJacobsonCDC force-pushed the RobertJacobsonCDC_benchmarks branch 2 times, most recently from b6a865b to fc85f08 Compare January 22, 2025 22:19
Copy link
Collaborator

@ekr-cfa ekr-cfa left a comment

Choose a reason for hiding this comment

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

I'm kind of suspicious of this overall structure where we have to incorporate all the files from the programs we're benchmarking. Is there instead some way to make them a library and then pull in the library separately from the criterion driver and the example main? Open to other alternatives, but I don't think linking to all the individual files is good practice.

Cargo.toml Outdated Show resolved Hide resolved
benches/README.md Outdated Show resolved Hide resolved
benches/README.md Outdated Show resolved Hide resolved
criterion.toml Outdated Show resolved Hide resolved
examples/births-deaths/infection_manager.rs Show resolved Hide resolved
@RobertJacobsonCDC RobertJacobsonCDC self-assigned this Jan 29, 2025
@RobertJacobsonCDC RobertJacobsonCDC force-pushed the RobertJacobsonCDC_benchmarks branch from fc85f08 to 6f86acc Compare February 3, 2025 23:01
.vscode
.idea

# Generated by Cargo
# will have compiled files and executables
debug/
target/
**/target/
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The libraries for the examples can now have their own target directory.

@RobertJacobsonCDC
Copy link
Collaborator Author

The examples are largely unchanged. The paths to input and output files were adjusted for them.

Running benchmarks:

cargo bench --bench example_births_deaths
cargo bench --bench example_basic_infection
cargo bench -- --bench example_benches
cargo bench

Running examples:

cargo run --example births-deaths
cargo run --example basic-infection

@RobertJacobsonCDC RobertJacobsonCDC force-pushed the RobertJacobsonCDC_benchmarks branch 2 times, most recently from f761f60 to 32c3e4f Compare February 4, 2025 13:58
…enches` directory, `example_births_deaths`, `example_basic_infection` benchmarks, README.md explaining how to run benchmarks and profile with samply.
@RobertJacobsonCDC RobertJacobsonCDC force-pushed the RobertJacobsonCDC_benchmarks branch from 32c3e4f to d726163 Compare February 4, 2025 14:39
Copy link
Collaborator

@ekr-cfa ekr-cfa left a comment

Choose a reason for hiding this comment

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

This structure still doesn't feel right. Added a comment inline, but happy to discuss live.

static SEED: u64 = 123;
static MAX_TIME: f64 = 303.0;

fn basic_infection() -> Context {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code appears to largely duplicate basic-infection/lib.rs, which isn't great.

The way I expected this to work is that each example lib exposes a single function that encapsulates the entire simulation and is called something like main_inner() and is effectively main. And then you would call main_inner() from either the true main or from the benchmarking framework.

Is there a reason that can't work?

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.

3 participants