-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add perf walltime support #13
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
base: main
Are you sure you want to change the base?
Conversation
@not-matthias can you fix CI please before I review ? 👼 Bonus question: any idea why the diff is so huge in gh display? |
Yeah, of course!
I think it's due to the clang-format pre-commit-hook (mostly valgrind.h). Should I add it to the blacklist? It's not really created by us |
This comment was marked as outdated.
This comment was marked as outdated.
CodSpeed WallTime Performance ReportMerging #13 will degrade performances by 33.33%Comparing Summary
Benchmarks breakdown
|
7e397c3
to
a105afb
Compare
Absolutely, rule of thumb should be that we never format external files like this, and if possible when we edit external files we make it so our formatter matches the initial style of the file to avoid unneccessary diffs, same for callgrind.h. |
@not-matthias could you make sure to rebase this on top of the windows PR once it's merged ? And then I'll review for real to avoid rework |
a105afb
to
014fcee
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.
Looks good overall! I think we'll need to be extra careful to make sure this still builds on Windows after #15; especially since we didn't test the instrument hooks on windows yet. I'll let @GuillaumeLagrange do the more thorough review once rebased on the initial PR
190aec9
to
c2aa8d6
Compare
c2aa8d6
to
da25a57
Compare
Depends on CodSpeedHQ/runner#94
Currently blocked by: CodSpeedHQ/instrument-hooks#6