-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat(criterion_compat): fork criterion and add walltime support #85
Conversation
CodSpeed Instrumentation Performance ReportMerging #85 will degrade performances by 16.51%Comparing Summary
Benchmarks breakdown
|
CodSpeed Walltime Performance ReportMerging #85 will degrade performances by 72.66%Comparing Summary
Benchmarks breakdown
|
1a4962d
to
1cfeccb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Biggest change is how we handle the fork which feels a bit uncomfortable, I would like us to be able to have more control over clippy to make diff completely minimal with original criterion
a6e0a7c
to
f5ec4c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very small comments, we can merge after this !
crates/criterion_compat/criterion-0.5.1/tests/criterion_tests.rs
Outdated
Show resolved
Hide resolved
crates/criterion_compat/criterion-0.5.1/tests/criterion_tests.rs
Outdated
Show resolved
Hide resolved
31adf46
to
4abfae8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Let's wait for the display issue with 0 time benchmarks before merging, I just want to sanity check that the perf regression reported by codspeed is expected/known and not caused by changes here, but can't really do it without the report page
Verified execution time of benchmarks with 0s, by comparing the measured walltime with local results:
Looks good, so I don't think we have a bug in there. |
Just some small structure details. Otherwise looks good |
e553cb8
to
2bf7839
Compare
Link for the uv-walltime-criterion integration: https://codspeed.io/AvalancheHQ/uv-criterion-walltime/runs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets go!
Signed-off-by: not-matthias <matthias@codspeed.io>
Signed-off-by: not-matthias <matthias@codspeed.io>
Signed-off-by: not-matthias <matthias@codspeed.io>
Signed-off-by: not-matthias <matthias@codspeed.io>
Signed-off-by: not-matthias <matthias@codspeed.io>
This is needed so that the submodule can be cloned on CI without having to setup SSH keys. Signed-off-by: not-matthias <matthias@codspeed.io>
2bf7839
to
9102482
Compare
No description provided.