From 36c76403ab482546e8821f499ba3d67688d99547 Mon Sep 17 00:00:00 2001 From: Daniel Harvey Date: Fri, 22 Mar 2019 05:53:20 +0000 Subject: [PATCH 1/8] Add FIXME in multiple_ranges_test.cc --- test/multiple_ranges_test.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/multiple_ranges_test.cc b/test/multiple_ranges_test.cc index c64acabc25..e60f9d1ffc 100644 --- a/test/multiple_ranges_test.cc +++ b/test/multiple_ranges_test.cc @@ -40,6 +40,8 @@ class MultipleRangesFixture : public ::benchmark::Fixture { // NOTE: This is not TearDown as we want to check after _all_ runs are // complete. virtual ~MultipleRangesFixture() { + // FIXME Should the 'if' below be checking for equality + // of the values in the containers, rather than their size? assert(actualValues.size() == expectedValues.size()); if (actualValues.size() != expectedValues.size()) { std::cout << "EXPECTED\n"; From 02a67f0a244eea85572f0706056b89630b8bc756 Mon Sep 17 00:00:00 2001 From: Daniel Harvey Date: Tue, 19 Mar 2019 06:17:20 +0000 Subject: [PATCH 2/8] Improve handling of large bounds in AddRange. Due to breaking the loop too early, AddRange would miss a final multplier of 'mult' that was within the numeric range of T. --- src/benchmark_register.h | 6 ++++-- test/benchmark_gtest.cc | 15 +++++++++++++++ test/options_test.cc | 3 +++ 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/benchmark_register.h b/src/benchmark_register.h index 0705e219f2..914edbeda6 100644 --- a/src/benchmark_register.h +++ b/src/benchmark_register.h @@ -17,11 +17,13 @@ void AddRange(std::vector* dst, T lo, T hi, int mult) { static const T kmax = std::numeric_limits::max(); // Now space out the benchmarks in multiples of "mult" - for (T i = 1; i < kmax / mult; i *= mult) { - if (i >= hi) break; + for (T i = 1; i < hi; i *= mult) { if (i > lo) { dst->push_back(i); } + // Break the loop here since multiplying by + // 'mult' would move outside of the range of T + if (i > kmax / mult) break; } // Add "hi" (if different from "lo") diff --git a/test/benchmark_gtest.cc b/test/benchmark_gtest.cc index 10683b433a..07e0a3257d 100644 --- a/test/benchmark_gtest.cc +++ b/test/benchmark_gtest.cc @@ -30,4 +30,19 @@ TEST(AddRangeTest, Advanced64) { EXPECT_THAT(dst, testing::ElementsAre(5, 8, 15)); } +TEST(AddRangeTest, FullRange8) { + std::vector dst; + AddRange(&dst, int8_t{1}, std::numeric_limits::max(), 8); + EXPECT_THAT(dst, testing::ElementsAre(1, 8, 64, 127)); +} + +TEST(AddRangeTest, FullRange64) { + std::vector dst; + AddRange(&dst, int64_t{1}, std::numeric_limits::max(), 1024); + EXPECT_THAT( + dst, testing::ElementsAre(1LL, 1024LL, 1048576LL, 1073741824LL, + 1099511627776LL, 1125899906842624LL, + 1152921504606846976LL, 9223372036854775807LL)); +} + } // end namespace diff --git a/test/options_test.cc b/test/options_test.cc index fdec69174e..3e12aa7ffc 100644 --- a/test/options_test.cc +++ b/test/options_test.cc @@ -35,6 +35,9 @@ BENCHMARK(BM_basic)->UseRealTime(); BENCHMARK(BM_basic)->ThreadRange(2, 4); BENCHMARK(BM_basic)->ThreadPerCpu(); BENCHMARK(BM_basic)->Repetitions(3); +BENCHMARK(BM_basic) + ->RangeMultiplier(std::numeric_limits::max()) + ->Range(0, std::numeric_limits::max()); void CustomArgs(benchmark::internal::Benchmark* b) { for (int i = 0; i < 10; ++i) { From 2ddf958ef95599e5aeafd1e82969af5a13775516 Mon Sep 17 00:00:00 2001 From: Daniel Harvey Date: Sat, 23 Mar 2019 14:34:00 +0000 Subject: [PATCH 3/8] Enable negative values for Range argument Fixes #762. --- src/benchmark_register.cc | 9 ++-- src/benchmark_register.h | 88 +++++++++++++++++++++++++++++++++++---- test/benchmark_gtest.cc | 78 ++++++++++++++++++++++++++++++++++ test/options_test.cc | 9 +++- 4 files changed, 169 insertions(+), 15 deletions(-) diff --git a/src/benchmark_register.cc b/src/benchmark_register.cc index 9d77529797..e39010f2f4 100644 --- a/src/benchmark_register.cc +++ b/src/benchmark_register.cc @@ -34,6 +34,8 @@ #include #include +#include + #include "benchmark/benchmark.h" #include "benchmark_api_internal.h" #include "check.h" @@ -183,11 +185,7 @@ bool BenchmarkFamilies::FindBenchmarks( } } - // we know that the args are always non-negative (see 'AddRange()'), - // thus print as 'unsigned'. BUT, do a cast due to the 32-bit builds. - instance.name.args += - StrFormat("%lu", static_cast(arg)); - + instance.name.args += StrFormat("%" PRId64, arg); ++arg_i; } @@ -334,7 +332,6 @@ Benchmark* Benchmark::ArgNames(const std::vector& names) { Benchmark* Benchmark::DenseRange(int64_t start, int64_t limit, int step) { CHECK(ArgsCnt() == -1 || ArgsCnt() == 1); - CHECK_GE(start, 0); CHECK_LE(start, limit); for (int64_t arg = start; arg <= limit; arg += step) { args_.push_back({arg}); diff --git a/src/benchmark_register.h b/src/benchmark_register.h index 914edbeda6..61377d7423 100644 --- a/src/benchmark_register.h +++ b/src/benchmark_register.h @@ -5,20 +5,25 @@ #include "check.h" +namespace benchmark { +namespace internal { + +// Append the powers of 'mult' in the closed interval [lo, hi]. +// Returns iterator to the start of the inserted range. template -void AddRange(std::vector* dst, T lo, T hi, int mult) { +typename std::vector::iterator +AddPowers(std::vector* dst, T lo, T hi, int mult) { CHECK_GE(lo, 0); CHECK_GE(hi, lo); CHECK_GE(mult, 2); - // Add "lo" - dst->push_back(lo); + const size_t start_offset = dst->size(); static const T kmax = std::numeric_limits::max(); - // Now space out the benchmarks in multiples of "mult" - for (T i = 1; i < hi; i *= mult) { - if (i > lo) { + // Space out the values in multiples of "mult" + for (T i = 1; i <= hi; i *= mult) { + if (i >= lo) { dst->push_back(i); } // Break the loop here since multiplying by @@ -26,10 +31,77 @@ void AddRange(std::vector* dst, T lo, T hi, int mult) { if (i > kmax / mult) break; } - // Add "hi" (if different from "lo") - if (hi != lo) { + return dst->begin() + start_offset; +} + +template +void AddNegatedPowers(std::vector* dst, T lo, T hi, int mult) { + // We negate lo and hi so we require that they cannot be equal to 'min'. + CHECK_GT(lo, std::numeric_limits::min()); + CHECK_GT(hi, std::numeric_limits::min()); + CHECK_GE(hi, lo); + CHECK_LE(hi, 0); + + // Add positive powers, then negate and reverse. + // Casts necessary since small integers get promoted + // to 'int' when negating. + const auto lo_complement = static_cast(-lo); + const auto hi_complement = static_cast(-hi); + + const auto it = AddPowers(dst, hi_complement, lo_complement, mult); + + std::for_each(it, dst->end(), [](T& t) { t *= -1; }); + std::reverse(it, dst->end()); +} + +template +void AddRange(std::vector* dst, T lo, T hi, int mult) { + static_assert(std::is_integral::value && std::is_signed::value, + "Args type must be a signed integer"); + + CHECK_GE(hi, lo); + CHECK_GE(mult, 2); + + // Add "lo" + dst->push_back(lo); + + // Handle lo == hi as a special case, so we then know + // lo < hi and so it is safe to add 1 to lo and subtract 1 + // from hi without falling outside of the range of T. + if (lo == hi) return; + + // Ensure that lo_inner <= hi_inner below. + if (lo + 1 == hi) { + dst->push_back(hi); + return; + } + + // Add all powers of 'mult' in the range [lo+1, hi-1] (inclusive). + const auto lo_inner = static_cast(lo + 1); + const auto hi_inner = static_cast(hi - 1); + + // Insert negative values + if (lo_inner < 0) { + AddNegatedPowers(dst, lo_inner, std::min(hi_inner, T{-1}), mult); + } + + // Treat 0 as a special case (see discussion on #762). + if (lo <= 0 && hi >= 0) { + dst->push_back(0); + } + + // Insert positive values + if (hi_inner > 0) { + AddPowers(dst, std::max(lo_inner, T{1}), hi_inner, mult); + } + + // Add "hi" (if different from last value). + if (hi != dst->back()) { dst->push_back(hi); } } +} // namespace internal +} // namespace benchmark + #endif // BENCHMARK_REGISTER_H diff --git a/test/benchmark_gtest.cc b/test/benchmark_gtest.cc index 07e0a3257d..2bad4338a1 100644 --- a/test/benchmark_gtest.cc +++ b/test/benchmark_gtest.cc @@ -6,6 +6,8 @@ namespace { +using namespace benchmark::internal; + TEST(AddRangeTest, Simple) { std::vector dst; AddRange(&dst, 1, 2, 2); @@ -45,4 +47,80 @@ TEST(AddRangeTest, FullRange64) { 1152921504606846976LL, 9223372036854775807LL)); } +TEST(AddRangeTest, NegativeRanges) { + std::vector dst; + AddRange(&dst, -8, 0, 2); + EXPECT_THAT(dst, testing::ElementsAre(-8, -4, -2, -1, 0)); +} + +TEST(AddRangeTest, StrictlyNegative) { + std::vector dst; + AddRange(&dst, -8, -1, 2); + EXPECT_THAT(dst, testing::ElementsAre(-8, -4, -2, -1)); +} + +TEST(AddRangeTest, SymmetricNegativeRanges) { + std::vector dst; + AddRange(&dst, -8, 8, 2); + EXPECT_THAT(dst, testing::ElementsAre(-8, -4, -2, -1, 0, 1, 2, 4, 8)); +} + +TEST(AddRangeTest, SymmetricNegativeRangesOddMult) { + std::vector dst; + AddRange(&dst, -30, 32, 5); + EXPECT_THAT(dst, testing::ElementsAre(-30, -25, -5, -1, 0, 1, 5, 25, 32)); +} + +TEST(AddRangeTest, NegativeRangesAsymmetric) { + std::vector dst; + AddRange(&dst, -3, 5, 2); + EXPECT_THAT(dst, testing::ElementsAre(-3, -2, -1, 0, 1, 2, 4, 5)); +} + +TEST(AddRangeTest, NegativeRangesLargeStep) { + // Always include -1, 0, 1 when crossing zero. + std::vector dst; + AddRange(&dst, -8, 8, 10); + EXPECT_THAT(dst, testing::ElementsAre(-8, -1, 0, 1, 8)); +} + +TEST(AddRangeTest, ZeroOnlyRange) { + std::vector dst; + AddRange(&dst, 0, 0, 2); + EXPECT_THAT(dst, testing::ElementsAre(0)); +} + +TEST(AddRangeTest, NegativeRange64) { + std::vector dst; + AddRange(&dst, -4, 4, 2); + EXPECT_THAT(dst, testing::ElementsAre(-4, -2, -1, 0, 1, 2, 4)); +} + +TEST(AddRangeTest, NegativeRangePreservesExistingOrder) { + // If elements already exist in the range, ensure we don't change + // their ordering by adding negative values. + std::vector dst = {1, 2, 3}; + AddRange(&dst, -2, 2, 2); + EXPECT_THAT(dst, testing::ElementsAre(1, 2, 3, -2, -1, 0, 1, 2)); +} + +TEST(AddRangeTest, FullNegativeRange64) { + std::vector dst; + const auto min = std::numeric_limits::min(); + const auto max = std::numeric_limits::max(); + AddRange(&dst, min, max, 1024); + EXPECT_THAT( + dst, testing::ElementsAre( + min, -1152921504606846976LL, -1125899906842624LL, + -1099511627776LL, -1073741824LL, -1048576LL, -1024LL, -1LL, 0LL, + 1LL, 1024LL, 1048576LL, 1073741824LL, 1099511627776LL, + 1125899906842624LL, 1152921504606846976LL, max)); +} + +TEST(AddRangeTest, Simple8) { + std::vector dst; + AddRange(&dst, 1, 8, 2); + EXPECT_THAT(dst, testing::ElementsAre(1, 2, 4, 8)); +} + } // end namespace diff --git a/test/options_test.cc b/test/options_test.cc index 3e12aa7ffc..7bfc235465 100644 --- a/test/options_test.cc +++ b/test/options_test.cc @@ -37,7 +37,14 @@ BENCHMARK(BM_basic)->ThreadPerCpu(); BENCHMARK(BM_basic)->Repetitions(3); BENCHMARK(BM_basic) ->RangeMultiplier(std::numeric_limits::max()) - ->Range(0, std::numeric_limits::max()); + ->Range(std::numeric_limits::min(), + std::numeric_limits::max()); + +// Negative ranges +BENCHMARK(BM_basic)->Range(-64, -1); +BENCHMARK(BM_basic)->RangeMultiplier(4)->Range(-8, 8); +BENCHMARK(BM_basic)->DenseRange(-2, 2, 1); +BENCHMARK(BM_basic)->Ranges({{-64, 1}, {-8, -1}}); void CustomArgs(benchmark::internal::Benchmark* b) { for (int i = 0; i < 10; ++i) { From b58e59e5a4c183f0e300be632844bfa0e643374a Mon Sep 17 00:00:00 2001 From: Daniel Harvey Date: Sun, 24 Mar 2019 09:09:35 +0000 Subject: [PATCH 4/8] Try to fix build of benchmark_gtest --- test/benchmark_gtest.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/benchmark_gtest.cc b/test/benchmark_gtest.cc index 2bad4338a1..bde9638ef6 100644 --- a/test/benchmark_gtest.cc +++ b/test/benchmark_gtest.cc @@ -110,11 +110,11 @@ TEST(AddRangeTest, FullNegativeRange64) { const auto max = std::numeric_limits::max(); AddRange(&dst, min, max, 1024); EXPECT_THAT( - dst, testing::ElementsAre( + dst, testing::ElementsAreArray({ min, -1152921504606846976LL, -1125899906842624LL, -1099511627776LL, -1073741824LL, -1048576LL, -1024LL, -1LL, 0LL, 1LL, 1024LL, 1048576LL, 1073741824LL, 1099511627776LL, - 1125899906842624LL, 1152921504606846976LL, max)); + 1125899906842624LL, 1152921504606846976LL, max})); } TEST(AddRangeTest, Simple8) { From 8c8c4d289b197e76c72fbc3afd3b661c1782ec0f Mon Sep 17 00:00:00 2001 From: Daniel Harvey Date: Sun, 24 Mar 2019 13:06:51 +0000 Subject: [PATCH 5/8] Try some more to fix build --- test/benchmark_gtest.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/benchmark_gtest.cc b/test/benchmark_gtest.cc index bde9638ef6..2fcf7f7435 100644 --- a/test/benchmark_gtest.cc +++ b/test/benchmark_gtest.cc @@ -110,7 +110,7 @@ TEST(AddRangeTest, FullNegativeRange64) { const auto max = std::numeric_limits::max(); AddRange(&dst, min, max, 1024); EXPECT_THAT( - dst, testing::ElementsAreArray({ + dst, testing::ElementsAreArray(std::vector{ min, -1152921504606846976LL, -1125899906842624LL, -1099511627776LL, -1073741824LL, -1048576LL, -1024LL, -1LL, 0LL, 1LL, 1024LL, 1048576LL, 1073741824LL, 1099511627776LL, From a5cd02255bc0efbf669d691a64942deac014a8c0 Mon Sep 17 00:00:00 2001 From: Daniel Harvey Date: Sun, 24 Mar 2019 17:44:23 +0000 Subject: [PATCH 6/8] Attempt to fix format macros --- src/benchmark_register.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/benchmark_register.cc b/src/benchmark_register.cc index e39010f2f4..4e94b5a219 100644 --- a/src/benchmark_register.cc +++ b/src/benchmark_register.cc @@ -34,6 +34,7 @@ #include #include +#define __STDC_FORMAT_MACROS #include #include "benchmark/benchmark.h" From 22137e5117018d388280898999175676b9d65e90 Mon Sep 17 00:00:00 2001 From: Daniel Harvey Date: Sun, 24 Mar 2019 19:42:12 +0000 Subject: [PATCH 7/8] Attempt to resolve format errors for mingw32 --- src/string_util.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/string_util.h b/src/string_util.h index fc5f8b0304..c5dcee6fcd 100644 --- a/src/string_util.h +++ b/src/string_util.h @@ -12,7 +12,9 @@ void AppendHumanReadable(int n, std::string* str); std::string HumanReadableNumber(double n, double one_k = 1024.0); -#ifdef __GNUC__ +#if defined(__MINGW32__) +__attribute__((format(__MINGW_PRINTF_FORMAT, 1, 2))) +#elif defined(__GNUC__) __attribute__((format(printf, 1, 2))) #endif std::string From 4a1a846b0ba6f79f26fae87bd42aa6d30dd614ae Mon Sep 17 00:00:00 2001 From: Daniel Harvey Date: Mon, 25 Mar 2019 19:42:02 +0000 Subject: [PATCH 8/8] Review feedback Put unit tests in benchmark::internal namespace Fix error reporting in multiple_ranges_test.cc --- test/benchmark_gtest.cc | 8 +++++--- test/multiple_ranges_test.cc | 5 +---- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/test/benchmark_gtest.cc b/test/benchmark_gtest.cc index 2fcf7f7435..9557b20ec7 100644 --- a/test/benchmark_gtest.cc +++ b/test/benchmark_gtest.cc @@ -4,10 +4,10 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" +namespace benchmark { +namespace internal { namespace { -using namespace benchmark::internal; - TEST(AddRangeTest, Simple) { std::vector dst; AddRange(&dst, 1, 2, 2); @@ -123,4 +123,6 @@ TEST(AddRangeTest, Simple8) { EXPECT_THAT(dst, testing::ElementsAre(1, 2, 4, 8)); } -} // end namespace +} // namespace +} // namespace internal +} // namespace benchmark diff --git a/test/multiple_ranges_test.cc b/test/multiple_ranges_test.cc index e60f9d1ffc..b25f40eb52 100644 --- a/test/multiple_ranges_test.cc +++ b/test/multiple_ranges_test.cc @@ -40,10 +40,7 @@ class MultipleRangesFixture : public ::benchmark::Fixture { // NOTE: This is not TearDown as we want to check after _all_ runs are // complete. virtual ~MultipleRangesFixture() { - // FIXME Should the 'if' below be checking for equality - // of the values in the containers, rather than their size? - assert(actualValues.size() == expectedValues.size()); - if (actualValues.size() != expectedValues.size()) { + if (actualValues != expectedValues) { std::cout << "EXPECTED\n"; for (auto v : expectedValues) { std::cout << "{";