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

Constant benchmark #70

Merged
merged 10 commits into from
Nov 13, 2023
Merged

Constant benchmark #70

merged 10 commits into from
Nov 13, 2023

Conversation

joshuabvarghese
Copy link
Contributor

  • Added additional benchmarks for the "constant" compressor.
  • Added benchmark functions for constant compression, decompression, optimization, compression vs. decompression, compression ratio, compression vs. data size, decompression vs. data size, memory usage, and compression vs. residuals.
  • Provided documentation and comments explaining each benchmark function and its purpose.
  • Utilized the Criterion crate for benchmarking and measuring performance.

Screenshot 2023-11-08 at 9 42 44 am

@joshuabvarghese joshuabvarghese marked this pull request as ready for review November 12, 2023 22:23
@rukai
Copy link
Contributor

rukai commented Nov 12, 2023

When I run cargo bench the benches dont actually run:
image

I believe this is because this PR is missing the [[bench]] section in your Cargo.toml, refer to https://bheisler.github.io/criterion.rs/book/getting_started.html for more information.

This PR adds criterion dependency under [dependencies] instead of [dev-dependencies].
This is a problem because it means any users of the brro-compressor crate will also depend on the criterion crate.
Making their builds take longer and using up disk space.
Whereas if we make criterion just a dev dependency then it is only used for our tests and benchmarks and dependent crates will not pull it in.

@joshuabvarghese
Copy link
Contributor Author

When I run cargo bench the benches dont actually run: image

I believe this is because this PR is missing the [[bench]] section in your Cargo.toml, refer to https://bheisler.github.io/criterion.rs/book/getting_started.html for more information.

This PR adds criterion dependency under [dependencies] instead of [dev-dependencies]. This is a problem because it means any users of the brro-compressor crate will also depend on the criterion crate. Making their builds take longer and using up disk space. Whereas if we make criterion just a dev dependency then it is only used for tests and benchmarks and dependent crates will not pull it in.

brro-compressor/Cargo.toml Outdated Show resolved Hide resolved
brro-compressor/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@rukai rukai left a comment

Choose a reason for hiding this comment

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

Generating random inputs can contribute to noisy benchmarks, a way to avoid this while still having "random" data is to set a seed so that every run of the benchmark uses the same "random" data.
Something to investigate if you want to bring down the noise.

I'm not familiar enough with the functionality we are actually benchmarking to comment on how well we cover it.
But I've given all the generic benchmark advice I can, so, looks good to me!

Copy link
Collaborator

@cjrolo cjrolo left a comment

Choose a reason for hiding this comment

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

Nice work!

@rukai rukai merged commit 53a28fb into main Nov 13, 2023
2 checks passed
@joshuabvarghese joshuabvarghese deleted the Constant_Bench branch November 14, 2023 10:40
@joshuabvarghese joshuabvarghese restored the Constant_Bench branch November 14, 2023 10:41
@joshuabvarghese joshuabvarghese deleted the Constant_Bench branch November 14, 2023 10:41
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