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
bench: Prevent thread oversubscription and decreases the variance of result values #19710
Conversation
Hmmmm... I'd maybe rather just abort the test and return early if only 1 thread gets made because we shouldn't ever be running with the checkqueue on a single core machine... |
We do not know for sure who many CPU cores/threads users dedicate to Bitcoin Core :) With this PR benchmark results are quite consistent:
And early return do not disable the test, right? |
If it's a benchmark that benchmarks thread synchronization/interaction (which is, I think, the only valid reason to run a benchmark multi-threaded) then I agree running it without threads makes little sense. (And if so you could even see the "two threads on a single-core machine" case as a realistic case, because bitcoind will create threads on such a machine) |
Co-authored-by: Martin Ankerl <Martin.Ankerl@gmail.com>
Updated d786765 -> a65c1ad (pr19710.01 -> pr19710.02, diff). Addressed:
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
cr ACK a65c1ad |
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.
ACK a65c1ad
Updated a65c1ad -> 4f91643 (pr19710.02 -> pr19710.03, diff). Addressed @vasild's comments:
|
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.
Concept ACK.
Updated 4f91643 -> 5e7acd6 (pr19710.03 -> pr19710.04, diff).
|
Code review ACK 5e7acd6. |
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.
ACK 5e7acd6
Thanks for the comments!
@MarcoFalke Mind having another look at this PR? |
This change decreases the variance of benchmark results.
Updated 5e7acd6 -> 3edc4e3 (pr19710.04 -> pr19710.05, diff).
|
Code review ACK 3edc4e3 |
ACK |
…ases the variance of result values 3edc4e3 bench: Prevent thread oversubscription (Hennadii Stepanov) ce3e6a7 bench: Allow skip benchmark (Hennadii Stepanov) Pull request description: Split out from bitcoin#18710. Some results (borrowed from bitcoin#18710): ![89121718-a3329800-d4c1-11ea-8bd1-66da20619696](https://user-images.githubusercontent.com/32963518/90146614-ecb89800-dd89-11ea-80fe-bac0e46e735e.png) ACKs for top commit: fjahr: Code review ACK 3edc4e3 Tree-SHA512: df7413ec9ea326564a8e8de54752c9d1444ff7de34edb03e1e0c2120fc333e4640767fdbe3e87eab6a7b389a4863c02e22ad2ae0dbf139fad6a9b85e00f563b4
Split out from #18710.
Some results (borrowed from #18710):