Respect min_time per thread in multi-threaded benchmarks (#2117)#2256
Open
devtejasx wants to merge 2 commits into
Open
Respect min_time per thread in multi-threaded benchmarks (#2117)#2256devtejasx wants to merge 2 commits into
devtejasx wants to merge 2 commits into
Conversation
real_time and manual_time (and whole-process CPU time) are accumulated across all threads in RunInThread. google#1836 removed the per-thread scaling of these values so that the reported Time is correct wall-clock time, but the same values also drive the min-time stopping decision. As a result a benchmark running N threads stopped after only min_time/N of wall-clock time per thread, and UseRealTime() no longer had any effect. Introduce GetTimeForDecision(), which divides the accumulated real/manual/ process-CPU time by the thread count for the stopping decision only. The values stored in IterationResults::results stay summed so reporting and counter rates are unaffected. Also scale the real_time runaway guard in ShouldReportIterationResults the same way. Add min_time_threads_gtest, which checks that the per-thread wall-clock time does not collapse as the thread count grows.
Collaborator
|
Duplicate of #2118. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2117.
Problem
real_timeandmanual_time(and whole-process CPU time) are accumulated across all threads inRunInThread(they are summed under the manager lock). #1836 removed the per-thread scaling of those values so that the reportedTimeis correct wall-clock time. However, the very same values also feed the min-time stopping decision viaIterationResults::seconds.The consequence: a benchmark running
Nthreads stops as soon as the summed time across threads reachesmin_time, i.e. after onlymin_time / Nof wall-clock time per thread. This is exactly what #2117 reports — the iteration count doesn't grow with more threads,--benchmark_min_timeisn't honored, andUseRealTime()no longer fixes it (it did before #1836).Fix
Add
BenchmarkRunner::GetTimeForDecision(), which returns the per-thread duration by dividing the accumulated real/manual/process-CPU time by the thread count — but only for the stopping decision (PredictNumItersNeededandShouldReportIterationResults). The summed values stored inIterationResults::resultsare left untouched, so reported times and global counter rates are unaffected. Thereal_time_used >= 5 * min_timerunaway guard is scaled the same way.This restores the pre-#1836 decision behavior without reintroducing #1834.
Test
test/min_time_threads_gtest.ccruns the same benchmark at 1 and 4 threads withUseRealTime()and asserts that the per-thread wall-clock time does not collapse as the thread count grows. It fails before this change (4-thread ≈ ¼ of 1-thread) and passes after.