Fix complexity calculation when range includes 0 (#410) by amoralpha · Pull Request #2346 · google/benchmark

amoralpha · 2026-10-23T08:09:44Z

Fixes #410.

complexity_n now defaults to -1 (“not set”) instead of 0, so SetComplexityN(0) is distinguishable from never calling it, as suggested in the issue. The check in ComputeBigO is relaxed to >= 0. Since n == 0 is now reachable, I added guards for log2(0) in oLogN/oNLogN and for a zero denominator in MinimalLeastSq.

Tests: zero-starting ranges for oN, auto, lambda, and oLogN with Range(0, 1), plus a death test for the missing SetComplexityN case. Without the fix, complexity_benchmark fails with the exact error from the issue (“Did you forget to call SetComplexityN?”).

Design note: n == 0 carries no asymptotic information, so the motivation is only that Range(0, ...) is a natural parameterization and currently aborts with a misleading message. If you’d prefer, I can instead keep n == 0 runs in the report but exclude them from the BigO fit.

Differentiate between uncalled SetComplexityN and SetComplexityN(0) by initializing State::complexity_n_ and ThreadManager::Result::complexity_n to -1, and relaxing the check in ComputeBigO to BM_CHECK_GE(run.complexity_n, 0). Add guards against n <= 1 in logarithmic curves and sigma_gn_squared == 0 in MinimalLeastSq. Add unit tests for zero-starting ranges and a death test for missing SetComplexityN calls.

LebedevRI · 2026-10-23T15:32:02Z

    default:
      return [](IterationCount) { return 1.0; };
  }
 }


These apparently take ComplexityN, not IterationCount, even though both are int64_t.
Please adjust these (and in the docs) while here.

Done! Updated 'FittingCurve', 'Result::complexity_n', 'test/complexity_test.cc', and 'docs/user_guide.md' to use 'ComplexityN' instead of 'IterationCount'.

LebedevRI · 2026-10-23T15:36:47Z

    double cpu_time_used = 0;
    double manual_time_used = 0;
-    int64_t complexity_n = 0;
+    int64_t complexity_n = -1;


Ah, so complexity_n is a signed integer, that's weird, but allows the fix to be not too intrusive...

- Use ComplexityN instead of IterationCount for complexity curves, Result::complexity_n, tests, and documentation. - Update complexity_n accumulation logic in RunInThread.

LebedevRI

The more i stare at this -1 the more i don't like it.
Please just do std::Optional<ComplexityN> complexity_n = None;

LebedevRI · 2026-10-23T22:08:48Z

    report.use_real_time_for_initial_big_o = b.use_manual_time();
    report.cpu_accumulated_time = results.cpu_time_used;
-    report.complexity_n = results.complexity_n;
+    report.complexity_n = results.complexity_n.value_or(-1);


report.complexity_n should also be `Optional<>

I tried making BenchmarkReporter::Run::complexity_n an std::optional<ComplexityN>, but because BenchmarkReporter is in include/benchmark/reporter.h (a public header), doing so breaks cxx11_test (test/CMakeLists.txt:77-86), which compiles public headers in C++11/C++14 mode where <optional> is unavailable:

warning STL4038: The contents of <optional> are available only with C++17 or later.

Therefore, Run::complexity_n uses -1 internally as a sentinel for the public reporter struct while internal runner/thread manager logic uses std::optional.

LebedevRI

Please update all complexity_n/complexity_n_ fields to all be Optional<>.
There should not be any == -1/<0/>=0 checks.

This reverts commit 172972c.

amoralpha · 2026-10-24T06:07:50Z

Please update all complexity_n/complexity_n_ fields to all be Optional<>. There should not be any == -1/<0/>=0 checks.

Thanks for the review. I ran into an issue making everything std::optional, so I’d like to ask for your input before going further.

ThreadManager::Result is internal, so std::optional works fine there. The problem is State::complexity_n_ and BenchmarkReporter::Run::complexity_n, which are in public headers. cxx11_test builds those headers in C++11/14 mode, where doesn’t exist, so it breaks (MSVC: 'optional': is not a member of 'std'). Everything else builds fine.

Also, changing the type of the public Run::complexity_n would break any custom reporter that reads it as an integer.

I see three options:

1.Bump cxx11_test to require C++17, and use std::optional everywhere, including the public headers.
2.Keep the public headers C++11-compatible: use std::optional in the internal implementation, and in the public structs use a plain value plus a bool flag.
3.Add a benchmark::Optional that is the same for all standards (no #if) and use it for every complexity_n.

Since this is more of a policy decision, I’d rather not pick one myself. Which would you prefer?

LebedevRI · 2026-10-24T15:31:52Z

Please update all complexity_n/complexity_n_ fields to all be Optional<>. There should not be any == -1/<0/>=0 checks.

Thanks for the review. I ran into an issue making everything std::optional, so I’d like to ask for your input before going further.

ThreadManager::Result is internal, so std::optional works fine there. The problem is State::complexity_n_ and BenchmarkReporter::Run::complexity_n, which are in public headers. cxx11_test builds those headers in C++11/14 mode, where doesn’t exist, so it breaks (MSVC: 'optional': is not a member of 'std'). Everything else builds fine.

Oh well, i forgot about that.

Also, changing the type of the public Run::complexity_n would break any custom reporter that reads it as an integer.

So here's the thing, this diff already breaks that. They most probably expect 0 as 'no-value', and suddenly it's -1.

I see three options:

1.Bump cxx11_test to require C++17, and use std::optional everywhere, including the public headers. 2.Keep the public headers C++11-compatible: use std::optional in the internal implementation, and in the public structs use a plain value plus a bool flag. 3.Add a benchmark::Optional that is the same for all standards (no #if) and use it for every complexity_n.

Since this is more of a policy decision, I’d rather not pick one myself. Which would you prefer?

I think it shouldn't be 1.
I guess 3., but just make a strict ComplexityN type that has a method to check if it has a value (internally an <0 check), and a getter that panics if the value isn't there.

amoralpha · 2026-10-24T20:17:23Z

Please update all complexity_n/complexity_n_ fields to all be Optional<>. There should not be any == -1/<0/>=0 checks.

Thanks for the review. I ran into an issue making everything std::optional, so I’d like to ask for your input before going further.
ThreadManager::Result is internal, so std::optional works fine there. The problem is State::complexity_n_ and BenchmarkReporter::Run::complexity_n, which are in public headers. cxx11_test builds those headers in C++11/14 mode, where doesn’t exist, so it breaks (MSVC: 'optional': is not a member of 'std'). Everything else builds fine.

Oh well, i forgot about that.

Also, changing the type of the public Run::complexity_n would break any custom reporter that reads it as an integer.

So here's the thing, this diff already breaks that. They most probably expect 0 as 'no-value', and suddenly it's -1.

I see three options:
1.Bump cxx11_test to require C++17, and use std::optional everywhere, including the public headers. 2.Keep the public headers C++11-compatible: use std::optional in the internal implementation, and in the public structs use a plain value plus a bool flag. 3.Add a benchmark::Optional that is the same for all standards (no #if) and use it for every complexity_n.
Since this is more of a policy decision, I’d rather not pick one myself. Which would you prefer?

I think it shouldn't be 1. I guess 3., but just make a strict ComplexityN type that has a method to check if it has a value (internally an <0 check), and a getter that panics if the value isn't there.

Got it, I've implemented ComplexityN locally as a strict class in include/benchmark/statistics.h (default-constructed as unset with n_ = -1, has_value() doing the >= 0 check, and value() aborting if unset, plus aborting on negative construction), and updated State::complexity_n_, ThreadManager::Result::complexity_n, and BenchmarkReporter::Run::complexity_n to use it with no external -1 / < 0 / >= 0 checks.

One quick question before I push: since ComplexityN is now a class rather than an integer typedef, what would you prefer for BigOFunc (typedef double(BigOFunc)(...)) and MinimalLeastSq?

Keep BigOFunc taking int64_t so existing user lambdas (e.g. [](int64_t n) -> double) keep compiling without changes.
Have BigOFunc take ComplexityN (callers/lambdas access n.value()).

LebedevRI · 2026-10-24T20:36:04Z

Code around

    BM_CHECK_GE(run.complexity_n, 0)
        << "Did you forget to call SetComplexityN?";

would desugar the new type into a valid int64_t.
I think the existing ComplexityN type alias should stay the way it is with the existing name.

amoralpha · 2026-10-24T22:03:58Z

Code around
    BM_CHECK_GE(run.complexity_n, 0)
        << "Did you forget to call SetComplexityN?";
would desugar the new type into a valid int64_t. I think the existing ComplexityN type alias should stay the way it is with the existing name.

Thanks! I've pushed this. ComplexityN stays the int64_t alias, and BigOFunc, MinimalLeastSq, the fitting curves, the tests and the user guide are unchanged.

The strict type is a separate OptionalComplexityN (default-constructed means "no value", has_value(), and value() aborts if unset; constructing it from a negative number also aborts). It is used only for State::complexity_n_, ThreadManager::Result::complexity_n and BenchmarkReporter::Run::complexity_n. ComputeBigO checks has_value() and unwraps it into a plain ComplexityN, as you described.

Please let me know if you'd like a different name. The Python bindings (State.complexity_n) are updated too, but they aren't compiled in my local CMake setup, so CI will be the first real build of that part.

LebedevRI · 2026-10-25T03:26:41Z

+      std::fprintf(stderr, "Complexity N must be non-negative: %lld\n",
+                   static_cast<long long>(n));
+      std::abort();


These should be BM_CHECK (maaaybe BM_CHECK_always or however it's called, but i wouldn't go there)

Done! Since BM_CHECK lives in internal src/check.h, I defined OptionalComplexityN(ComplexityN) and value() out-of-line in src/statistics.cc (exporting OptionalComplexityN with BENCHMARK_EXPORT), and guarded the two new death tests with TEST_BENCHMARK_LIBRARY_HAS_NO_ASSERTIONS.

LebedevRI · 2026-10-25T03:26:51Z

+    if (!has_value()) {
+      std::fprintf(stderr, "Did you forget to call SetComplexityN?\n");
+      std::abort();
+    }


LebedevRI · 2026-10-25T03:28:43Z

+    BM_CHECK(run.complexity_n.has_value())
        << "Did you forget to call SetComplexityN?";
-    n.push_back(run.complexity_n);
+    n.push_back(run.complexity_n.value());


the getter already handles the error case

Done — dropped the redundant BM_CHECK(run.complexity_n.has_value()) here.

LebedevRI

Nice, this looks somewhat reasonable.
I'll need to sleep on this, but i do believe this will be fine overall.
Thank you.

…teBigO

amoralpha mentioned this pull request Oct 23, 2026

Fail when calculating complexity #410

Open