-
Notifications
You must be signed in to change notification settings - Fork 646
Callgrind benchmarks #619
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
Callgrind benchmarks #619
Conversation
|
callgrind please |
5f690a0 to
0a2c52a
Compare
cloutiertyler
left a comment
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.
I feel under qualified to review this PR, but I don't see any red flags.
|
@cloutiertyler , here's example output: Callgrind Benchmark ReportThese benchmarks were run using callgrind, Of these, Note: there are currently a minimal number of Legend:
filter
insert bulk
iterate
serialize_product_value
empty transaction
|
|
The delta there is between two runs of the last commit on this branch, fwiw, it's just to show the formatting. You can also see that we still have about 1% variance in the module benchmarks, but it's a lot lower. I could add other metrics e.g. estimated cycles / cache misses there, but they're a lot higher variance. |
|
I'd be particularly interested in cache misses even if they're higher variance, if only because optimizing for instructions could incentivize people to optimize that at the expense of cache performance. |
kulakowski
left a comment
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.
This overall looks good to me. It's a pretty big PR so I defer to @joshua-spacetime about how to land it. I'll say how I broke this apart in my head and how I feel about each part:
- the CI machinery: seems fine, seems easy to iterate on and land separately, I'd like someone more knowledable than me to say whether it's risky: like, could this interfere with existing CI actions?
- the docker machinery: seems ok, seems linux specific and needs some guard rails for non-linux systems?
- the callgrind machinery: the forks are unfortunate, but I get it. I'd personally want to see some mechanism for knowing we keep them up to date. but I think that about all our dependencies in general, so. I'm personally fine with the
flagmagic, but someone else should probably agree with me about that. - the benchmarks themeselves. seems ok but I'd want @joshua-spacetime to be ok with how forward progress there is tracked.
- the serialization machinery: also seems ok for the reason I mentioned, it seems easy for someone else to add on to, and I think incentivizing creation of more benchmarks is what I would optimize for.
- the reporting machinery: again it seems fine, but it's a big chunk of code and I wonder if it can be split out?
I added some places I'd add tracking issues. But really I just want James and Joshua to be on the same page for tracking that work, I don't care if it's issues or something else.
|
I've slimmed this down a bit by ripping out everything related to generating reports (except the script to pack json files for uploading), that now lives in the benchmarks-viewer application: https://github.com/clockworklabs/benchmarks-viewer None of the stuff here should interfere with other CI. |
778a3e2 to
1a45e3e
Compare
|
These are still failing it would appear. |
67d8b7f to
6a82551
Compare
|
@cloutiertyler this is ready to merge |
|
Ah, didn't see Tyler wanted John to look over this, whoops. |
Description of Changes
At long last.