-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Implementation of random interleaving. #1105
Conversation
http://github.com/google/benchmark/issues/1051 for the feature requests. Committer: Hai Huang (http://github.com/haih-g) On branch fr-1051 Changes to be committed: modified: include/benchmark/benchmark.h modified: src/benchmark.cc new file: src/benchmark_adjust_repetitions.cc new file: src/benchmark_adjust_repetitions.h modified: src/benchmark_api_internal.cc modified: src/benchmark_api_internal.h modified: src/benchmark_register.cc modified: src/benchmark_runner.cc modified: src/benchmark_runner.h modified: test/CMakeLists.txt new file: test/benchmark_random_interleaving_gtest.cc
Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark.cc modified: src/benchmark_runner.cc modified: test/benchmark_random_interleaving_gtest.cc
Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark_api_internal.cc modified: src/benchmark_api_internal.h modified: src/benchmark_runner.cc
Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark_runner.cc
…-1051 Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: test/benchmark_random_interleaving_gtest.cc
Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: test/benchmark_random_interleaving_gtest.cc
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.
there's a lot here but i've left some initial comments.
it would also be good to see the results of a run of something like the basic tests with and without interleaving enabled.
Without Random Interleaving
With Random Interleaving
|
Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark.cc modified: src/benchmark_api_internal.cc modified: src/benchmark_api_internal.h modified: test/benchmark_random_interleaving_gtest.cc
tests are showing a lot of 0 CPU result (which is then causing reporter output tests to fail because 'inf' is not a number). i don't see anything obvious in how this has been triggered by this change, but it's worth taking a look. windows only too, which is also odd. |
…fr-1051. Also change sentinel of random_interleaving_repetitions to -1. Hopefully it fixes the failures on Windows. Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark.cc modified: src/benchmark_api_internal.cc modified: src/benchmark_api_internal.h
Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark_api_internal.cc modified: src/benchmark_runner.cc
i think this looks good, but i'm biased because i've seen it before. i'd like @LebedevRI to also take a look. |
Missing documentation changes. |
Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark_adjust_repetitions.cc modified: src/benchmark_adjust_repetitions.h
This PR introduces a new paradigm to run benchmarks and it has to touch multiple files at the same time. The only piece I can split off is the benchmark_adjust_repetitions.h|cc. I can have a dummy |
src/benchmark.cc
Outdated
std::vector<RunResults> run_results_vector(benchmarks.size()); | ||
for (size_t i = 0; i < outer_repetitions; i++) { | ||
if (FLAGS_benchmark_enable_random_interleaving) { | ||
std::random_shuffle(benchmark_indices.begin(), benchmark_indices.end()); |
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.
std::random_shuffle
uses std::rand
and was deprecated in C++11 and removed in C++17. std::shuffle
is its replacement. See https://en.cppreference.com/w/cpp/algorithm/random_shuffle
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.
Done
Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark.cc
src/benchmark_adjust_repetitions.cc
Outdated
|
||
namespace { | ||
|
||
constexpr double kNanosecondInSecond = 0.000000001; |
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.
= 1e-9
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.
Done
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 still believe this can be split up at least into 3 parts:
- BenchmarkInstance rework (rewrite to getters)
- Native interleaving just of the repetitions as they are defined now
- Introduction of automatic repetitions for interleaving
As is it's a bit too big to review. I guess it looks fine.
Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark_adjust_repetitions.cc
CI failure is real:
you need to tweak the name to something like |
Done |
Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark_api_internal.cc modified: src/benchmark_api_internal.h modified: src/benchmark_runner.cc
One last thing :) Can you add a section to the |
Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: README.md new file: docs/random_interleaving.md
Done |
thank you! |
Ok. Now that i have actually tried this locally, i have the very same thought i voiced originally: Because this was too big, two pretty minor bugs slipped through:
If i was at the helm, this is where i'd back this out, and ask for a complete redesign, I am talking about redesign, because we can't just divide min time by the desired number of fractions, :/ |
I've reverted this in #1161. @haih-g, if we want to try this again then I think following the approach outlined by @LebedevRI is worthwhile. We need to make sure that expectations of interactions between different flags are honoured. |
To help me better understand the bug, could you please provide a few run examples? From what you described, I think this is actually the "repetition auto-adjustment" feature. Random interleaving will introduce some overhead and the overhead is controlled by the flag One thing we learned from the Google internal implementation is that benchmarks may timeout simply because the overhead of random interleaving may be too much. So we have to have "repetition auto-adjustment" because we cannot afford breaking anyone in a mono-repo environment. But maybe this is not needed in the world of OSS, as everyone is doing version control by themselves. So if "repetition auto-adjustment" is causing troubles, we can either:
Thoughts? |
…le#1051) Based on the original implementation by Hai Huang @haih-g) from google#1105.
Based on original implementation by Hai Huang @haih-g in #1105
Based on original implementation by Hai Huang @haih-g in #1105
…le#1051) Based on the original implementation by Hai Huang @haih-g) from google#1105.
…le#1051) Inspired by the original implementation by Hai Huang @haih-g from google#1105. The original implementation had design deficiencies that weren't really addressable without redesign, so it was reverted. In essence, the original implementation consisted of two separateable parts: * reducing the amount time each repetition is run for, and symmetrically increasing repetition count * running the repetitions in random order While it worked fine for the usual case, it broke down when user would specify repetitions (it would completely ignore that request), or specified per-repetition min time (while it would still adjust the repetition count, it would not adjust the per-repetition time, leading to much greater run times) Here, like i was originally suggesting in the original review, i'm separating the features, and only dealing with a single one - running repetitions in random order. Now that the runs/repetitions are no longer in-order, the tooling may wish to sort the output, and indeed `compare.py` has been updated to do that: google#1168.
…le#1051) Inspired by the original implementation by Hai Huang @haih-g from google#1105. The original implementation had design deficiencies that weren't really addressable without redesign, so it was reverted. In essence, the original implementation consisted of two separateable parts: * reducing the amount time each repetition is run for, and symmetrically increasing repetition count * running the repetitions in random order While it worked fine for the usual case, it broke down when user would specify repetitions (it would completely ignore that request), or specified per-repetition min time (while it would still adjust the repetition count, it would not adjust the per-repetition time, leading to much greater run times) Here, like i was originally suggesting in the original review, i'm separating the features, and only dealing with a single one - running repetitions in random order. Now that the runs/repetitions are no longer in-order, the tooling may wish to sort the output, and indeed `compare.py` has been updated to do that: google#1168.
… (#1163) Inspired by the original implementation by Hai Huang @haih-g from #1105. The original implementation had design deficiencies that weren't really addressable without redesign, so it was reverted. In essence, the original implementation consisted of two separateable parts: * reducing the amount time each repetition is run for, and symmetrically increasing repetition count * running the repetitions in random order While it worked fine for the usual case, it broke down when user would specify repetitions (it would completely ignore that request), or specified per-repetition min time (while it would still adjust the repetition count, it would not adjust the per-repetition time, leading to much greater run times) Here, like i was originally suggesting in the original review, i'm separating the features, and only dealing with a single one - running repetitions in random order. Now that the runs/repetitions are no longer in-order, the tooling may wish to sort the output, and indeed `compare.py` has been updated to do that: #1168.
* Refactor BenchmarkInstance (precursor to google#1105) * fix bazel (debug) build * clang-format on header * fix build error on g++-4.8
* Implementation of random interleaving. See http://github.com/google/benchmark/issues/1051 for the feature requests. Committer: Hai Huang (http://github.com/haih-g) On branch fr-1051 Changes to be committed: modified: include/benchmark/benchmark.h modified: src/benchmark.cc new file: src/benchmark_adjust_repetitions.cc new file: src/benchmark_adjust_repetitions.h modified: src/benchmark_api_internal.cc modified: src/benchmark_api_internal.h modified: src/benchmark_register.cc modified: src/benchmark_runner.cc modified: src/benchmark_runner.h modified: test/CMakeLists.txt new file: test/benchmark_random_interleaving_gtest.cc * Fix benchmark_random_interleaving_gtest.cc for fr-1051 Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark.cc modified: src/benchmark_runner.cc modified: test/benchmark_random_interleaving_gtest.cc * Fix macos build for fr-1051 Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark_api_internal.cc modified: src/benchmark_api_internal.h modified: src/benchmark_runner.cc * Fix macos and windows build for fr-1051. Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark_runner.cc * Fix benchmark_random_interleaving_test.cc for macos and windows in fr-1051 Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: test/benchmark_random_interleaving_gtest.cc * Fix int type benchmark_random_interleaving_gtest for macos in fr-1051 Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: test/benchmark_random_interleaving_gtest.cc * Address dominichamon's comments 03/29 for fr-1051 Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark.cc modified: src/benchmark_api_internal.cc modified: src/benchmark_api_internal.h modified: test/benchmark_random_interleaving_gtest.cc * Address dominichamon's comment on default min_time / repetitions for fr-1051. Also change sentinel of random_interleaving_repetitions to -1. Hopefully it fixes the failures on Windows. Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark.cc modified: src/benchmark_api_internal.cc modified: src/benchmark_api_internal.h * Fix windows test failures for fr-1051 Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark_api_internal.cc modified: src/benchmark_runner.cc * Add license blurb for fr-1051. Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark_adjust_repetitions.cc modified: src/benchmark_adjust_repetitions.h * Switch to std::shuffle() for fr-1105. Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark.cc * Change to 1e-9 in fr-1105 Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark_adjust_repetitions.cc * Fix broken build caused by bad merge for fr-1105. Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark_api_internal.cc modified: src/benchmark_runner.cc * Fix build breakage for fr-1051. Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark.cc modified: src/benchmark_api_internal.cc modified: src/benchmark_api_internal.h modified: src/benchmark_register.cc modified: src/benchmark_runner.cc * Print out reports as they come in if random interleaving is disabled (fr-1051) Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark.cc * size_t, int64_t --> int in benchmark_runner for fr-1051. Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark_runner.cc modified: src/benchmark_runner.h * Address comments from dominichamon for fr-1051 Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark.cc modified: src/benchmark_adjust_repetitions.cc modified: src/benchmark_adjust_repetitions.h modified: src/benchmark_api_internal.cc modified: src/benchmark_api_internal.h modified: test/benchmark_random_interleaving_gtest.cc * benchmar_indices --> size_t to make CI pass: fr-1051 Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark.cc * Fix min_time not initialized issue for fr-1051. Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark_api_internal.cc modified: src/benchmark_api_internal.h * min_time --> MinTime in fr-1051. Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: src/benchmark_api_internal.cc modified: src/benchmark_api_internal.h modified: src/benchmark_runner.cc * Add doc for random interleaving for fr-1051 Committer: Hai Huang <haih@google.com> On branch fr-1051 Your branch is up to date with 'origin/fr-1051'. Changes to be committed: modified: README.md new file: docs/random_interleaving.md Co-authored-by: Dominic Hamon <dominichamon@users.noreply.github.com>
Based on original implementation by Hai Huang @haih-g in google#1105
…le#1051) (google#1163) Inspired by the original implementation by Hai Huang @haih-g from google#1105. The original implementation had design deficiencies that weren't really addressable without redesign, so it was reverted. In essence, the original implementation consisted of two separateable parts: * reducing the amount time each repetition is run for, and symmetrically increasing repetition count * running the repetitions in random order While it worked fine for the usual case, it broke down when user would specify repetitions (it would completely ignore that request), or specified per-repetition min time (while it would still adjust the repetition count, it would not adjust the per-repetition time, leading to much greater run times) Here, like i was originally suggesting in the original review, i'm separating the features, and only dealing with a single one - running repetitions in random order. Now that the runs/repetitions are no longer in-order, the tooling may wish to sort the output, and indeed `compare.py` has been updated to do that: google#1168.
See http://github.com/google/benchmark/issues/1051 for the feature requests.
Committer: Hai Huang (http://github.com/haih-g)
On branch fr-1051
Changes to be committed:
modified: include/benchmark/benchmark.h
modified: src/benchmark.cc
new file: src/benchmark_adjust_repetitions.cc
new file: src/benchmark_adjust_repetitions.h
modified: src/benchmark_api_internal.cc
modified: src/benchmark_api_internal.h
modified: src/benchmark_register.cc
modified: src/benchmark_runner.cc
modified: src/benchmark_runner.h
modified: test/CMakeLists.txt
new file: test/benchmark_random_interleaving_gtest.cc