Skip to content

Set up criterion for benchmarking #197

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

Merged
merged 4 commits into from
May 19, 2025
Merged

Set up criterion for benchmarking #197

merged 4 commits into from
May 19, 2025

Conversation

friendlymatthew
Copy link
Member

@friendlymatthew friendlymatthew commented May 8, 2025

This commit uses criterion for its benchmarking suite. Also contains a README about benchmarking instructions.

Copy link
Contributor

hyperlint-ai bot commented May 8, 2025

PR Change Summary

Set up criterion for benchmarking and implemented std::error::Error for jiter errors.

  • Migrated to criterion for the benchmarking suite
  • Implemented std::error::Error for jiter error types
  • Added a README for benchmark instructions

Added Files

  • crates/jiter/benches/README.md

How can I customize these reviews?

Check out the Hyperlint AI Reviewer docs for more information on how to customize the review.

If you just want to ignore it on this PR, you can add the hyperlint-ignore label to the PR. Future changes won't trigger a Hyperlint review.

Note specifically for link checks, we only check the first 30 links in a file and we cache the results for several hours (for instance, if you just added a page, you might experience this). Our recommendation is to add hyperlint-ignore to the PR to ignore the link check for this PR.

Copy link

codecov bot commented May 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

Copy link

codspeed-hq bot commented May 8, 2025

CodSpeed Performance Report

Merging #197 will create unknown performance changes

Comparing criterion (a10651b) with main (961d830)

Summary

🆕 70 new benchmarks
⁉️ 70 dropped benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
🆕 big_jiter_iter N/A 36.9 ms N/A
🆕 big_jiter_skip N/A 19.2 ms N/A
🆕 big_jiter_value N/A 49.9 ms N/A
🆕 big_serde_value N/A 204.3 ms N/A
🆕 bigints_array_jiter_iter N/A 125.3 µs N/A
🆕 bigints_array_jiter_skip N/A 94.1 µs N/A
🆕 bigints_array_jiter_value N/A 208.9 µs N/A
🆕 bigints_array_serde_value N/A 719.3 µs N/A
🆕 floats_array_jiter_iter N/A 184.7 µs N/A
🆕 floats_array_jiter_skip N/A 106.2 µs N/A
🆕 floats_array_jiter_value N/A 308.3 µs N/A
🆕 floats_array_serde_value N/A 1.1 ms N/A
🆕 massive_ints_array_jiter_iter N/A 573 µs N/A
🆕 massive_ints_array_jiter_skip N/A 223.4 µs N/A
🆕 massive_ints_array_jiter_value N/A 764.8 µs N/A
🆕 massive_ints_array_serde_value N/A 2.2 ms N/A
🆕 medium_response_jiter_skip N/A 16.9 µs N/A
🆕 medium_response_jiter_value N/A 37.8 µs N/A
🆕 medium_response_jiter_value_owned N/A 50.7 µs N/A
🆕 medium_response_serde_value N/A 84.3 µs N/A
... ... ... ... ...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

Copy link
Collaborator

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks, overall looks good though we need to fix the codspeed measurement in CI.

Also, maybe split the error impls into their own PR for sake of making those changes more visible in their own right?

@@ -1,5 +1,4 @@
use codspeed_bencher_compat::{benchmark_group, benchmark_main, Bencher};
use std::hint::black_box;
use criterion::{black_box, criterion_group, criterion_main, Criterion};
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should use codspeed_criterion_compat here so that the benchmarks continue to be measured in CI.

@friendlymatthew friendlymatthew changed the title Set up criterion and impl std::error::Error for error types Set up criterion for benchmarking May 19, 2025
@friendlymatthew friendlymatthew force-pushed the criterion branch 3 times, most recently from a6ebbcf to 67ca987 Compare May 19, 2025 13:09
@davidhewitt
Copy link
Collaborator

Looks like this introduces a dependency on half that is not MSRV compatible. It might be possible to tune things to make that not the case.

Copy link
Collaborator

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Super, thanks! 👍

@davidhewitt davidhewitt merged commit e6b28c2 into main May 19, 2025
25 of 26 checks passed
@davidhewitt davidhewitt deleted the criterion branch May 19, 2025 16:27
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