Skip to content

Commit

Permalink
Iteration counts should be uint64_t globally. (#817)
Browse files Browse the repository at this point in the history
This is a shameless rip-off of #646
I did promise to look into why that proposed PR was producing
so much worse assembly, and so i finally did.

The reason is - that diff changes `size_t` (unsigned) to `int64_t` (signed).

There is this nice little `assert`:
https://github.com/google/benchmark/blob/7a1c37028359ca9d386d719a6ad527743cf1b753/include/benchmark/benchmark.h#L744
It ensures that we didn't magically decide to advance our iterator
when we should have finished benchmarking.

When `cached_` was unsigned, the `assert` was `cached_ UGT 0`.
But we only ever get to that `assert` if `cached_ NE 0`,
and naturally if `cached_` is not `0`, then it is bigger than `0`,
so the `assert` is tautological, and gets folded away.

But now that `cached_` became signed, the assert became `cached_ SGT 0`.
And we still only know that `cached_ NE 0`, so the assert can't be
optimized out, or at least it doesn't currently.

Regardless of whether or not that is a bug in itself,
that particular diff would have regressed the normal 64-bit systems,
by halving the maximal iteration space (since we go from unsigned counter
to signed one, of the same bit-width), which seems like a bug.
And just so it happens, fixing *this* bug, fixes the other bug.

This produces fully (bit-by-bit) identical state_assembly_test.s
The filecheck change is actually needed regardless of this patch,
else this test does not pass for me even without this diff.
  • Loading branch information
LebedevRI committed May 13, 2019
1 parent 2e7203a commit f92903c
Show file tree
Hide file tree
Showing 15 changed files with 73 additions and 59 deletions.
38 changes: 19 additions & 19 deletions include/benchmark/benchmark.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,7 @@ static void BM_memcpy(benchmark::State& state) {
memset(src, 'x', state.range(0));
for (auto _ : state)
memcpy(dst, src, state.range(0));
state.SetBytesProcessed(int64_t(state.iterations()) *
int64_t(state.range(0)));
state.SetBytesProcessed(state.iterations() * state.range(0));
delete[] src; delete[] dst;
}
BENCHMARK(BM_memcpy)->Arg(8)->Arg(64)->Arg(512)->Arg(1<<10)->Arg(8<<10);
Expand Down Expand Up @@ -122,8 +121,7 @@ template <class Q> int BM_Sequential(benchmark::State& state) {
q.Wait(&v);
}
// actually messages, not bytes:
state.SetBytesProcessed(
static_cast<int64_t>(state.iterations())*state.range(0));
state.SetBytesProcessed(state.iterations() * state.range(0));
}
BENCHMARK_TEMPLATE(BM_Sequential, WaitQueue<int>)->Range(1<<0, 1<<10);
Expand Down Expand Up @@ -413,9 +411,11 @@ enum TimeUnit { kNanosecond, kMicrosecond, kMillisecond };
// calculated automatically to the best fit.
enum BigO { oNone, o1, oN, oNSquared, oNCubed, oLogN, oNLogN, oAuto, oLambda };

typedef uint64_t IterationCount;

// BigOFunc is passed to a benchmark in order to specify the asymptotic
// computational complexity for the benchmark.
typedef double(BigOFunc)(int64_t);
typedef double(BigOFunc)(IterationCount);

// StatisticsFunc is passed to a benchmark in order to compute some descriptive
// statistics over all the measurements of some type
Expand Down Expand Up @@ -488,7 +488,7 @@ class State {
// while (state.KeepRunningBatch(1000)) {
// // process 1000 elements
// }
bool KeepRunningBatch(size_t n);
bool KeepRunningBatch(IterationCount n);

// REQUIRES: timer is running and 'SkipWithError(...)' has not been called
// by the current thread.
Expand Down Expand Up @@ -627,7 +627,7 @@ class State {
int64_t range_y() const { return range(1); }

BENCHMARK_ALWAYS_INLINE
size_t iterations() const {
IterationCount iterations() const {
if (BENCHMARK_BUILTIN_EXPECT(!started_, false)) {
return 0;
}
Expand All @@ -638,15 +638,15 @@ class State {
: // items we expect on the first cache line (ie 64 bytes of the struct)
// When total_iterations_ is 0, KeepRunning() and friends will return false.
// May be larger than max_iterations.
size_t total_iterations_;
IterationCount total_iterations_;

// When using KeepRunningBatch(), batch_leftover_ holds the number of
// iterations beyond max_iters that were run. Used to track
// completed_iterations_ accurately.
size_t batch_leftover_;
IterationCount batch_leftover_;

public:
const size_t max_iterations;
const IterationCount max_iterations;

private:
bool started_;
Expand All @@ -667,14 +667,14 @@ class State {
const int threads;

private:
State(size_t max_iters, const std::vector<int64_t>& ranges, int thread_i,
int n_threads, internal::ThreadTimer* timer,
State(IterationCount max_iters, const std::vector<int64_t>& ranges,
int thread_i, int n_threads, internal::ThreadTimer* timer,
internal::ThreadManager* manager);

void StartKeepRunning();
// Implementation of KeepRunning() and KeepRunningBatch().
// is_batch must be true unless n is 1.
bool KeepRunningInternal(size_t n, bool is_batch);
bool KeepRunningInternal(IterationCount n, bool is_batch);
void FinishKeepRunning();
internal::ThreadTimer* timer_;
internal::ThreadManager* manager_;
Expand All @@ -686,11 +686,11 @@ inline BENCHMARK_ALWAYS_INLINE bool State::KeepRunning() {
return KeepRunningInternal(1, /*is_batch=*/false);
}

inline BENCHMARK_ALWAYS_INLINE bool State::KeepRunningBatch(size_t n) {
inline BENCHMARK_ALWAYS_INLINE bool State::KeepRunningBatch(IterationCount n) {
return KeepRunningInternal(n, /*is_batch=*/true);
}

inline BENCHMARK_ALWAYS_INLINE bool State::KeepRunningInternal(size_t n,
inline BENCHMARK_ALWAYS_INLINE bool State::KeepRunningInternal(IterationCount n,
bool is_batch) {
// total_iterations_ is set to 0 by the constructor, and always set to a
// nonzero value by StartKepRunning().
Expand Down Expand Up @@ -754,7 +754,7 @@ struct State::StateIterator {
}

private:
size_t cached_;
IterationCount cached_;
State* const parent_;
};

Expand Down Expand Up @@ -858,7 +858,7 @@ class Benchmark {
// NOTE: This function should only be used when *exact* iteration control is
// needed and never to control or limit how long a benchmark runs, where
// `--benchmark_min_time=N` or `MinTime(...)` should be used instead.
Benchmark* Iterations(size_t n);
Benchmark* Iterations(IterationCount n);

// Specify the amount of times to repeat this benchmark. This option overrides
// the `benchmark_repetitions` flag.
Expand Down Expand Up @@ -957,7 +957,7 @@ class Benchmark {
TimeUnit time_unit_;
int range_multiplier_;
double min_time_;
size_t iterations_;
IterationCount iterations_;
int repetitions_;
bool measure_process_cpu_time_;
bool use_real_time_;
Expand Down Expand Up @@ -1375,7 +1375,7 @@ class BenchmarkReporter {
bool error_occurred;
std::string error_message;

int64_t iterations;
IterationCount iterations;
int64_t threads;
int64_t repetition_index;
int64_t repetitions;
Expand Down
4 changes: 2 additions & 2 deletions src/benchmark.cc
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,8 @@ void UseCharPointer(char const volatile*) {}

} // namespace internal

State::State(size_t max_iters, const std::vector<int64_t>& ranges, int thread_i,
int n_threads, internal::ThreadTimer* timer,
State::State(IterationCount max_iters, const std::vector<int64_t>& ranges,
int thread_i, int n_threads, internal::ThreadTimer* timer,
internal::ThreadManager* manager)
: total_iterations_(0),
batch_leftover_(0),
Expand Down
6 changes: 3 additions & 3 deletions src/benchmark_api_internal.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
namespace benchmark {
namespace internal {

State BenchmarkInstance::Run(
size_t iters, int thread_id, internal::ThreadTimer* timer,
internal::ThreadManager* manager) const {
State BenchmarkInstance::Run(IterationCount iters, int thread_id,
internal::ThreadTimer* timer,
internal::ThreadManager* manager) const {
State st(iters, arg, thread_id, threads, timer, manager);
benchmark->Run(st);
return st;
Expand Down
4 changes: 2 additions & 2 deletions src/benchmark_api_internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,10 @@ struct BenchmarkInstance {
bool last_benchmark_instance;
int repetitions;
double min_time;
size_t iterations;
IterationCount iterations;
int threads; // Number of concurrent threads to us

State Run(size_t iters, int thread_id, internal::ThreadTimer* timer,
State Run(IterationCount iters, int thread_id, internal::ThreadTimer* timer,
internal::ThreadManager* manager) const;
};

Expand Down
2 changes: 1 addition & 1 deletion src/benchmark_register.cc
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ Benchmark* Benchmark::MinTime(double t) {
return this;
}

Benchmark* Benchmark::Iterations(size_t n) {
Benchmark* Benchmark::Iterations(IterationCount n) {
CHECK(n > 0);
CHECK(IsZero(min_time_));
iterations_ = n;
Expand Down
23 changes: 12 additions & 11 deletions src/benchmark_runner.cc
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,12 @@ MemoryManager* memory_manager = nullptr;

namespace {

static const size_t kMaxIterations = 1000000000;
static constexpr IterationCount kMaxIterations = 1000000000;

BenchmarkReporter::Run CreateRunReport(
const benchmark::internal::BenchmarkInstance& b,
const internal::ThreadManager::Result& results, size_t memory_iterations,
const internal::ThreadManager::Result& results,
IterationCount memory_iterations,
const MemoryManager::Result& memory_result, double seconds,
int64_t repetition_index) {
// Create report about this benchmark run.
Expand Down Expand Up @@ -109,8 +110,8 @@ BenchmarkReporter::Run CreateRunReport(

// Execute one thread of benchmark b for the specified number of iterations.
// Adds the stats collected for the thread into *total.
void RunInThread(const BenchmarkInstance* b, size_t iters, int thread_id,
ThreadManager* manager) {
void RunInThread(const BenchmarkInstance* b, IterationCount iters,
int thread_id, ThreadManager* manager) {
internal::ThreadTimer timer(
b->measure_process_cpu_time
? internal::ThreadTimer::CreateProcessCpuTime()
Expand Down Expand Up @@ -187,13 +188,13 @@ class BenchmarkRunner {

std::vector<std::thread> pool;

size_t iters; // preserved between repetitions!
IterationCount iters; // preserved between repetitions!
// So only the first repetition has to find/calculate it,
// the other repetitions will just use that precomputed iteration count.

struct IterationResults {
internal::ThreadManager::Result results;
size_t iters;
IterationCount iters;
double seconds;
};
IterationResults DoNIterations() {
Expand Down Expand Up @@ -248,7 +249,7 @@ class BenchmarkRunner {
return i;
}

size_t PredictNumItersNeeded(const IterationResults& i) const {
IterationCount PredictNumItersNeeded(const IterationResults& i) const {
// See how much iterations should be increased by.
// Note: Avoid division by zero with max(seconds, 1ns).
double multiplier = min_time * 1.4 / std::max(i.seconds, 1e-9);
Expand All @@ -262,10 +263,10 @@ class BenchmarkRunner {
if (multiplier <= 1.0) multiplier = 2.0;

// So what seems to be the sufficiently-large iteration count? Round up.
const size_t max_next_iters =
const IterationCount max_next_iters =
0.5 + std::max(multiplier * i.iters, i.iters + 1.0);
// But we do have *some* sanity limits though..
const size_t next_iters = std::min(max_next_iters, kMaxIterations);
const IterationCount next_iters = std::min(max_next_iters, kMaxIterations);

VLOG(3) << "Next iters: " << next_iters << ", " << multiplier << "\n";
return next_iters; // round up before conversion to integer.
Expand Down Expand Up @@ -319,11 +320,11 @@ class BenchmarkRunner {

// Oh, one last thing, we need to also produce the 'memory measurements'..
MemoryManager::Result memory_result;
size_t memory_iterations = 0;
IterationCount memory_iterations = 0;
if (memory_manager != nullptr) {
// Only run a few iterations to reduce the impact of one-time
// allocations in benchmarks that are not properly managed.
memory_iterations = std::min<size_t>(16, iters);
memory_iterations = std::min<IterationCount>(16, iters);
memory_manager->Start();
std::unique_ptr<internal::ThreadManager> manager;
manager.reset(new internal::ThreadManager(1));
Expand Down
15 changes: 9 additions & 6 deletions src/complexity.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,20 +29,23 @@ BigOFunc* FittingCurve(BigO complexity) {
static const double kLog2E = 1.44269504088896340736;
switch (complexity) {
case oN:
return [](int64_t n) -> double { return static_cast<double>(n); };
return [](IterationCount n) -> double { return static_cast<double>(n); };
case oNSquared:
return [](int64_t n) -> double { return std::pow(n, 2); };
return [](IterationCount n) -> double { return std::pow(n, 2); };
case oNCubed:
return [](int64_t n) -> double { return std::pow(n, 3); };
return [](IterationCount n) -> double { return std::pow(n, 3); };
case oLogN:
/* Note: can't use log2 because Android's GNU STL lacks it */
return [](int64_t n) { return kLog2E * log(static_cast<double>(n)); };
return
[](IterationCount n) { return kLog2E * log(static_cast<double>(n)); };
case oNLogN:
/* Note: can't use log2 because Android's GNU STL lacks it */
return [](int64_t n) { return kLog2E * n * log(static_cast<double>(n)); };
return [](IterationCount n) {
return kLog2E * n * log(static_cast<double>(n));
};
case o1:
default:
return [](int64_t) { return 1.0; };
return [](IterationCount) { return 1.0; };
}
}

Expand Down
5 changes: 3 additions & 2 deletions src/counter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
namespace benchmark {
namespace internal {

double Finish(Counter const& c, int64_t iterations, double cpu_time,
double Finish(Counter const& c, IterationCount iterations, double cpu_time,
double num_threads) {
double v = c.value;
if (c.flags & Counter::kIsRate) {
Expand All @@ -35,7 +35,8 @@ double Finish(Counter const& c, int64_t iterations, double cpu_time,
return v;
}

void Finish(UserCounters* l, int64_t iterations, double cpu_time, double num_threads) {
void Finish(UserCounters* l, IterationCount iterations, double cpu_time,
double num_threads) {
for (auto& c : *l) {
c.second.value = Finish(c.second, iterations, cpu_time, num_threads);
}
Expand Down
3 changes: 2 additions & 1 deletion src/counter.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ namespace benchmark {

// these counter-related functions are hidden to reduce API surface.
namespace internal {
void Finish(UserCounters* l, int64_t iterations, double time, double num_threads);
void Finish(UserCounters* l, IterationCount iterations, double time,
double num_threads);
void Increment(UserCounters* l, UserCounters const& r);
bool SameNames(UserCounters const& l, UserCounters const& r);
} // end namespace internal
Expand Down
6 changes: 6 additions & 0 deletions src/json_reporter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,12 @@ std::string FormatKV(std::string const& key, int64_t value) {
return ss.str();
}

std::string FormatKV(std::string const& key, IterationCount value) {
std::stringstream ss;
ss << '"' << StrEscape(key) << "\": " << value;
return ss.str();
}

std::string FormatKV(std::string const& key, double value) {
std::stringstream ss;
ss << '"' << StrEscape(key) << "\": ";
Expand Down
2 changes: 1 addition & 1 deletion src/thread_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class ThreadManager {

public:
struct Result {
int64_t iterations = 0;
IterationCount iterations = 0;
double real_time_used = 0;
double cpu_time_used = 0;
double manual_time_used = 0;
Expand Down
8 changes: 4 additions & 4 deletions test/basic_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ BENCHMARK(BM_empty_stop_start)->ThreadPerCpu();


void BM_KeepRunning(benchmark::State& state) {
size_t iter_count = 0;
benchmark::IterationCount iter_count = 0;
assert(iter_count == state.iterations());
while (state.KeepRunning()) {
++iter_count;
Expand All @@ -109,8 +109,8 @@ BENCHMARK(BM_KeepRunning);

void BM_KeepRunningBatch(benchmark::State& state) {
// Choose a prime batch size to avoid evenly dividing max_iterations.
const size_t batch_size = 101;
size_t iter_count = 0;
const benchmark::IterationCount batch_size = 101;
benchmark::IterationCount iter_count = 0;
while (state.KeepRunningBatch(batch_size)) {
iter_count += batch_size;
}
Expand All @@ -119,7 +119,7 @@ void BM_KeepRunningBatch(benchmark::State& state) {
BENCHMARK(BM_KeepRunningBatch);

void BM_RangedFor(benchmark::State& state) {
size_t iter_count = 0;
benchmark::IterationCount iter_count = 0;
for (auto _ : state) {
++iter_count;
}
Expand Down

0 comments on commit f92903c

Please sign in to comment.