Merge branch 'main' into fix-timing-misuse-diagnostics

Author: dmah42

AUTHORS

@@ -41,6 +41,7 @@ Haihan Jiang <haihanj99@gmail.com>
 Henrique Bucher <hbucher@gmail.com>
 International Business Machines Corporation
 Ismael Jimenez Martinez <ismael.jimenez.martinez@gmail.com>
+jdymitarai <o10040115@gmail.com>
 Jern-Kuan Leong <jernkuan@gmail.com>
 JianXiong Zhou <zhoujianxiong2@gmail.com>
 Joao Paulo Magalhaes <joaoppmagalhaes@gmail.com>

CONTRIBUTORS

@@ -62,6 +62,7 @@ Hannes Hauswedell <h2@fsfe.org>
 Henrique Bucher <hbucher@gmail.com>
 Ismael Jimenez Martinez <ismael.jimenez.martinez@gmail.com>
 Iakov Sergeev <yahontu@gmail.com>
+jdymitarai <o10040115@gmail.com>
 Jern-Kuan Leong <jernkuan@gmail.com>
 JianXiong Zhou <zhoujianxiong2@gmail.com>
 Joao Paulo Magalhaes <joaoppmagalhaes@gmail.com>

docs/user_guide.md

@@ -850,6 +850,11 @@ BENCHMARK(BM_StringCompare)->RangeMultiplier(2)
     ->Range(1<<10, 1<<18)->Complexity([](benchmark::IterationCount n)->double{return n; });
 ```
 
+The `BigO` row reports the coefficient in the same time unit as the rest of the
+family, so a family declared with `->Unit(benchmark::kMicrosecond)` reports its
+coefficient in microseconds. The `RMS` row is a normalized quantity and is
+reported as a percentage, independent of the time unit.
+
 <a name="custom-benchmark-name" />
 
 ## Custom Benchmark Name

src/benchmark.cc

@@ -42,6 +42,7 @@
 #include <algorithm>
 #include <atomic>
 #include <condition_variable>
+#include <cstddef>
 #include <cstdio>
 #include <cstdlib>
 #include <cstring>
@@ -222,42 +223,17 @@ State::State(std::string name, IterationCount max_iters,
     }
   }
 
-  // Note: The use of offsetof below is technically undefined until C++17
-  // because State is not a standard layout type. However, all compilers
-  // currently provide well-defined behavior as an extension (which is
-  // demonstrated since constexpr evaluation must diagnose all undefined
-  // behavior). However, GCC and Clang also warn about this use of offsetof,
-  // which must be suppressed.
-#if defined(__INTEL_COMPILER)
-#pragma warning push
-#pragma warning(disable : 1875)
-#elif defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Winvalid-offsetof"
-#endif
-#if defined(__NVCC__)
-#pragma nv_diagnostic push
-#pragma nv_diag_suppress 1427
-#endif
-#if defined(__NVCOMPILER)
-#pragma diagnostic push
-#pragma diag_suppress offset_in_non_POD_nonstandard
-#endif
-  // Offset tests to ensure commonly accessed data is on the first cache line.
-  const int cache_line_size = 64;
-  static_assert(
-      offsetof(State, skipped_) <= (cache_line_size - sizeof(skipped_)), "");
-#if defined(__INTEL_COMPILER)
-#pragma warning pop
-#elif defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic pop
-#endif
-#if defined(__NVCC__)
-#pragma nv_diagnostic pop
-#endif
-#if defined(__NVCOMPILER)
-#pragma diagnostic pop
-#endif
+  // Ensure commonly accessed data is on the first cache line. State is not a
+  // standard-layout type, so the offset is measured on the live object.
+  //
+  // The check is enabled under NDEBUG as well: a State is constructed once per
+  // benchmark instance and thread, never inside the iteration loop.
+  BM_CHECK_ALWAYS(reinterpret_cast<const char*>(&skipped_) -
+                      reinterpret_cast<const char*>(this) <=
+                  /*cache_line_size=*/64 -
+                      static_cast<std::ptrdiff_t>(sizeof(skipped_)))
+      << "the commonly accessed members of State must fit in the first cache "
+         "line";
 }
 
 void State::PauseTimingImpl() {

src/check.h

@@ -79,15 +79,20 @@ class CheckHandler {
 }  // end namespace internal
 }  // end namespace benchmark
 
-// The BM_CHECK macro returns a std::ostream object that can have extra
-// information written to it.
-#ifndef NDEBUG
-#define BM_CHECK(b)                                          \
+// Like BM_CHECK, but enabled in every build configuration. Reserved for
+// invariants that are cheap enough to verify unconditionally and that must not
+// be violated silently in a release build.
+#define BM_CHECK_ALWAYS(b)                                   \
   (b ? ::benchmark::internal::GetNullLogInstance()           \
      : ::benchmark::internal::CheckHandler(                  \
            std::string_view(#b), std::string_view(__FILE__), \
            std::string_view(__func__), __LINE__)             \
            .GetLog())
+
+// The BM_CHECK macro returns a std::ostream object that can have extra
+// information written to it.
+#ifndef NDEBUG
+#define BM_CHECK(b) BM_CHECK_ALWAYS(b)
 #else
 #define BM_CHECK(b) ::benchmark::internal::GetNullLogInstance()
 #endif

src/complexity.cc

@@ -229,6 +229,7 @@ std::vector<BenchmarkReporter::Run> ComputeBigO(
   big_o.cpu_accumulated_time = result_cpu.coef;
   big_o.report_big_o = true;
   big_o.complexity = result_cpu.complexity;
+  big_o.time_unit = reports[0].time_unit;
 
   // All the time results are reported after being multiplied by the
   // time unit multiplier. But since RMS is a relative quantity it

test/complexity_test.cc

@@ -20,13 +20,16 @@ namespace {
 int AddComplexityTest(const std::string& test_name,
                       const std::string& big_o_test_name,
                       const std::string& rms_test_name,
-                      const std::string& big_o, int family_index) {
+                      const std::string& big_o, int family_index,
+                      const std::string& time_unit,
+                      const std::string& real_coefficient) {
   SetSubstitutions({{"%name", test_name},
                     {"%bigo_name", big_o_test_name},
                     {"%rms_name", rms_test_name},
                     {"%bigo_str", "[ ]* %float " + big_o},
                     {"%bigo", big_o},
-                    {"%rms", "[ ]*[0-9]+ %"}});
+                    {"%rms", "[ ]*[0-9]+ %"},
+                    {"%unit", time_unit}});
   AddCases(
       TC_ConsoleOut,
       {{"^%bigo_name %bigo_str %bigo_str[ ]*$"},
@@ -44,9 +47,9 @@ int AddComplexityTest(const std::string& test_name,
        {"\"aggregate_name\": \"BigO\",$", MR_Next},
        {"\"aggregate_unit\": \"time\",$", MR_Next},
        {"\"cpu_coefficient\": %float,$", MR_Next},
-       {"\"real_coefficient\": %float,$", MR_Next},
+       {"\"real_coefficient\": " + real_coefficient + ",$", MR_Next},
        {"\"big_o\": \"%bigo\",$", MR_Next},
-       {"\"time_unit\": \"ns\"$", MR_Next},
+       {"\"time_unit\": \"%unit\"$", MR_Next},
        {"}", MR_Next},
        {"\"name\": \"%rms_name\",$"},
        {"\"family_index\": " + std::to_string(family_index) + ",$", MR_Next},
@@ -104,15 +107,15 @@ constexpr char lambda_big_o_1[] = "f\\(N\\)";
 
 // Add enum tests
 ADD_COMPLEXITY_CASES(one_test_name, big_o_1_test_name, rms_o_1_test_name,
-                     enum_auto_big_o_1, /*family_index=*/0);
+                     enum_auto_big_o_1, /*family_index=*/0, "ns", "%float");
 
 // Add auto tests
 ADD_COMPLEXITY_CASES(one_test_name, big_o_1_test_name, rms_o_1_test_name,
-                     enum_auto_big_o_1, /*family_index=*/1);
+                     enum_auto_big_o_1, /*family_index=*/1, "ns", "%float");
 
 // Add lambda tests
 ADD_COMPLEXITY_CASES(one_test_name, big_o_1_test_name, rms_o_1_test_name,
-                     lambda_big_o_1, /*family_index=*/2);
+                     lambda_big_o_1, /*family_index=*/2, "ns", "%float");
 
 // ========================================================================= //
 // --------------------------- Testing BigO O(N) --------------------------- //
@@ -161,15 +164,15 @@ constexpr char lambda_big_o_n[] = "f\\(N\\)";
 
 // Add enum tests
 ADD_COMPLEXITY_CASES(n_test_name, big_o_n_test_name, rms_o_n_test_name,
-                     enum_auto_big_o_n, /*family_index=*/3);
+                     enum_auto_big_o_n, /*family_index=*/3, "ns", "%float");
 
 // Add auto tests
 ADD_COMPLEXITY_CASES(n_test_name, big_o_n_test_name, rms_o_n_test_name,
-                     enum_auto_big_o_n, /*family_index=*/4);
+                     enum_auto_big_o_n, /*family_index=*/4, "ns", "%float");
 
 // Add lambda tests
 ADD_COMPLEXITY_CASES(n_test_name, big_o_n_test_name, rms_o_n_test_name,
-                     lambda_big_o_n, /*family_index=*/5);
+                     lambda_big_o_n, /*family_index=*/5, "ns", "%float");
 
 // ========================================================================= //
 // ------------------------- Testing BigO O(NlgN) ------------------------- //
@@ -222,17 +225,17 @@ constexpr char lambda_big_o_n_lg_n[] = "f\\(N\\)";
 // Add enum tests
 ADD_COMPLEXITY_CASES(n_lg_n_test_name, big_o_n_lg_n_test_name,
                      rms_o_n_lg_n_test_name, enum_auto_big_o_n_lg_n,
-                     /*family_index=*/6);
+                     /*family_index=*/6, "ns", "%float");
 
 // NOTE: auto big-o is wron.g
 ADD_COMPLEXITY_CASES(n_lg_n_test_name, big_o_n_lg_n_test_name,
                      rms_o_n_lg_n_test_name, enum_auto_big_o_n_lg_n,
-                     /*family_index=*/7);
+                     /*family_index=*/7, "ns", "%float");
 
 //// Add lambda tests
 ADD_COMPLEXITY_CASES(n_lg_n_test_name, big_o_n_lg_n_test_name,
                      rms_o_n_lg_n_test_name, lambda_big_o_n_lg_n,
-                     /*family_index=*/8);
+                     /*family_index=*/8, "ns", "%float");
 
 // ========================================================================= //
 // -------- Testing formatting of Complexity with captured args ------------ //
@@ -265,7 +268,36 @@ const std::string complexity_capture_name =
 
 ADD_COMPLEXITY_CASES(complexity_capture_name, complexity_capture_name + "_BigO",
                      complexity_capture_name + "_RMS", "N",
-                     /*family_index=*/9);
+                     /*family_index=*/9, "ns", "%float");
+
+// ========================================================================= //
+// ------------------- Testing BigO with a declared unit ------------------- //
+// ========================================================================= //
+
+void BM_Complexity_O_N_ms(benchmark::State& state) {
+  for (auto _ : state) {
+    // 1us per iteration per entry, the 2048 size 50% slower so the RMS is
+    // above zero, reported in milliseconds
+    const double skew = (state.range(0) & (1 << 11)) ? 1.5 : 1.0;
+    state.SetIterationTime(static_cast<double>(state.range(0)) * 1e-6 * skew);
+  }
+  state.SetComplexityN(state.range(0));
+}
+BENCHMARK(BM_Complexity_O_N_ms)
+    ->RangeMultiplier(2)
+    ->Range(1 << 10, 1 << 16)
+    ->UseManualTime()
+    ->Unit(benchmark::kMillisecond)
+    ->Complexity(benchmark::oN);
+
+constexpr char n_ms_test_name[] = "BM_Complexity_O_N_ms/manual_time";
+constexpr char big_o_n_ms_test_name[] = "BM_Complexity_O_N_ms/manual_time_BigO";
+constexpr char rms_o_n_ms_test_name[] = "BM_Complexity_O_N_ms/manual_time_RMS";
+
+ADD_COMPLEXITY_CASES(n_ms_test_name, big_o_n_ms_test_name, rms_o_n_ms_test_name,
+                     enum_auto_big_o_n, /*family_index=*/10, "ms",
+                     "1\\.[0-9]+e-03");
+
 }  // end namespace
 
 // ========================================================================= //

tools/compare.py

@@ -12,6 +12,7 @@
 import sys
 import unittest
 from argparse import ArgumentParser
+from unittest.mock import patch
 
 import gbench
 from gbench import report, util
@@ -56,6 +57,45 @@ def check_inputs(in1, in2, flags):
         sys.exit(1)
 
 
+def enable_virtual_terminal_processing():
+    """
+    On Windows, enable ENABLE_VIRTUAL_TERMINAL_PROCESSING on the console
+    output handle to allow ANSI escape sequences to be rendered natively.
+    Returns True if VT processing is enabled or on non-Windows; False otherwise.
+    """
+    if sys.platform == "win32":
+        try:
+            import ctypes
+
+            kernel32 = ctypes.windll.kernel32
+            handle = kernel32.GetStdHandle(-11)  # STD_OUTPUT_HANDLE
+            mode = ctypes.c_ulong()
+            if kernel32.GetConsoleMode(handle, ctypes.byref(mode)):
+                # ENABLE_VIRTUAL_TERMINAL_PROCESSING = 0x0004
+                return bool(
+                    kernel32.SetConsoleMode(handle, mode.value | 0x0004)
+                )
+            return False
+        except Exception:
+            return False
+    return True
+
+
+def should_use_color():
+    """
+    Determine whether terminal color output should be enabled by default.
+    Follows the NO_COLOR specification (https://no-color.org/), verifies that
+    stdout is a TTY, and checks that the Windows console supports VT processing.
+    """
+    if "NO_COLOR" in os.environ:
+        return False
+    if not hasattr(sys.stdout, "isatty") or not sys.stdout.isatty():
+        return False
+    if sys.platform == "win32":
+        return enable_virtual_terminal_processing()
+    return os.environ.get("TERM", "") != "dumb"
+
+
 def create_parser():
     parser = ArgumentParser(
         description="versatile benchmark output compare tool"
@@ -76,7 +116,7 @@ def create_parser():
     parser.add_argument(
         "--no-color",
         dest="color",
-        default=True,
+        default=should_use_color(),
         action="store_false",
         help="Do not use colors in the terminal output",
     )
@@ -534,6 +574,39 @@ def test_benchmarksfiltered_with_remainder_after_doubleminus(self):
         self.assertEqual(parsed.filter_contender[0], "e")
         self.assertEqual(parsed.benchmark_options[0], "g")
 
+    def test_benchmarks_no_color_flag(self):
+        parsed = self.parser.parse_args(
+            ["--no-color", "benchmarks", self.testInput0, self.testInput1]
+        )
+        self.assertFalse(parsed.color)
+
+    @patch.dict(os.environ, {"NO_COLOR": "1"})
+    def test_benchmarks_color_default_no_color_env(self):
+        parser = create_parser()
+        parsed = parser.parse_args(
+            ["benchmarks", self.testInput0, self.testInput1]
+        )
+        self.assertFalse(parsed.color)
+
+    @patch.dict(os.environ, {}, clear=True)
+    @patch("sys.stdout.isatty", return_value=False)
+    def test_benchmarks_color_default_not_a_tty(self, mock_isatty):
+        parser = create_parser()
+        parsed = parser.parse_args(
+            ["benchmarks", self.testInput0, self.testInput1]
+        )
+        self.assertFalse(parsed.color)
+
+    @patch.dict(os.environ, {}, clear=True)
+    @patch("compare.enable_virtual_terminal_processing", return_value=True)
+    @patch("sys.stdout.isatty", return_value=True)
+    def test_benchmarks_color_default_tty(self, mock_isatty, mock_vt):
+        parser = create_parser()
+        parsed = parser.parse_args(
+            ["benchmarks", self.testInput0, self.testInput1]
+        )
+        self.assertTrue(parsed.color)
+
 
 if __name__ == "__main__":
     # unittest.main()