-
Notifications
You must be signed in to change notification settings - Fork 973
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
Apply time benchmarking #4402
Apply time benchmarking #4402
Conversation
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 haven't looked too deeply into implementation, but looks good overall! (and seems already usable, would be interesting to see the results for our current configuration) Just left a few nit-picks/questions.
63f0d25
to
84d55fe
Compare
681343f
to
c2ac746
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, but I'm not sure if we want to include this in p22 build. On one hand, this is not risky from the protocol standpoint, on the other hand we're modifying loadgen and that has a bit of risk for supercluster tests. I think we still have a bit of time before cutting the release, so we should have a run with just these changes. WDYT?
Actually there are some test failures, so we need to address these first. |
Yeah that's a good point. The good thing is that if this change did break loadgen, we'd find out pretty quickly in SSC, so I think it's fine to merge. Worst case we'll just have to revert it. |
Description
Resolves #4391.
Checklist
clang-format
v8.0.0 (viamake format
or the Visual Studio extension)