benchmark icon indicating copy to clipboard operation
benchmark copied to clipboard

[FR] Allow benchmarks to report their parameterization structurally to JSON

Open matta opened this issue 3 years ago • 10 comments

Is your feature request related to a problem? Please describe.

The benchmark::RegisterBenchmark() API supports adding benchmarks that are parameterized with arbitrary values, including custom names, lambdas, arbitrary objects, etc. The structural details of the parameterization is currently not available in the JSON output. The best the user can do is encode information in the benchmark name, which is a good idea anyway for clear console output. For the JSON output, however, it would be nice if a more data driven approach was available, such as key/value pairs. This way, analysis scripts need not parse the information out of the benchmark names.

I have personally come across a few cases where I have registered a benchmark matrix manually in code and had to come up with a benchmark name parsing approach when analyzing the data. This is an uncommon edge case, though, so I agree that any solution here should be simple.

This is similar to #838, but for the parameters that the benchmark library can't know about, doesn't know how to represent in JSON, etc.

Describe the solution you'd like

I am not invested in any particular solution, but this is one idea:

Question is, what's the end use case? I think in general it is bad to enshrine in API something that is going to be rather obscurely used, but rather instead some generalization should be provided that allows to solve bigger problem. There's already global AddCustomContext(). Perhaps having the same but with different scopes will be sufficient?

Originally posted by @LebedevRI in https://github.com/google/benchmark/issues/838#issuecomment-1242788747

So perhaps code could look like this (pseudocode):

for (param1 : param1_values) {
  for (param2 : param2_values) {
      name = "BM_Thing/" + param1.ToString() + "/" + param2.ToString();
      auto* benchmark = benchmark::RegisterBenchmark(
          name, [param1, param2](benchmark::State& state) {
            BM_MyBenchmark(state, param1.value(), param2.value());
          });
      benchmark->AddCustomContext("param1", param1.ToString());
      benchmark->AddCustomContext("param2", param2.ToString());
  }
}

Some of the fancier BENCHMARK_ macro wrappers around benchmark::RegisterBenchmark() could perhaps be adapted to add their stringified args using this new benchmark->AddCustomContext() API.

Describe alternatives you've considered

In the past I thought about URL encoding key/value pairs in the benchmark name. ;-)

Additional context

Eventually, with something like this in place perhaps some of the filtering logic in compare.py could optionally match benchmarks by these values, rather than requiring the use of a regex against the name.

matta avatar Sep 13 '22 15:09 matta

if we could wrap this in the macro i'd prefer it (it's quite duplicative).

dmah42 avatar Sep 13 '22 16:09 dmah42

if we could wrap this in the macro i'd prefer it (it's quite duplicative).

I'm not against a macro API, but what would the macro API look like? I have a hard time imagining this without restricting what the user can do. One issue I have is that benchmark::RegisterBenchmarks() allows parameterization with arbitrary C++ objects, which are neither serializable to human readable strings nor JSON values without extra work.

I began at the C++ level because this whole thing feels like an edge case that perhaps doesn't justify the convenience of a "pretty" macro API. The benchmark::RegisterBenchmark() API is already the escape hatch for when the current BENCHMARK_ macros are insufficient or unwieldy. That said, there is nothing stopping those macros from also using this new API too (e.g. with the stringified macro args).

matta avatar Sep 13 '22 16:09 matta

if we could wrap this in the macro i'd prefer it (it's quite duplicative).

I'm not against a macro API, but what would the macro API look like? I have a hard time imagining this without restricting what the user can do. One issue I have is that benchmark::RegisterBenchmarks() allows parameterization with arbitrary C++ objects, which are neither serializable to human readable strings nor JSON values without extra work.

I began at the C++ level because this whole thing feels like an edge case that perhaps doesn't justify the convenience of a "pretty" macro API. The benchmark::RegisterBenchmark() API is already the escape hatch for when the current BENCHMARK_ macros are insufficient or unwieldy. That said, there is nothing stopping those macros from also using this new API too (e.g. with the stringified macro args).

that's totally fair.

how about adding the custom context keys to the benchmark name automatically so that part at least is unnecessary.

dmah42 avatar Sep 13 '22 16:09 dmah42

how about adding the custom context keys to the benchmark name automatically so that part at least is unnecessary.

Would the keys be useful? Or did you mean the custom context values? Those are what are likely to uniquely identify the way the benchmark is parameterized.

matta avatar Sep 14 '22 16:09 matta

param1.ToString() which you're adding to the benchmark name and the custom context separately. i wonder if it's in custom context should we automatically add it to the benchmark name on reports.

dmah42 avatar Sep 14 '22 16:09 dmah42

Yes, I like that idea.

matta avatar Sep 14 '22 17:09 matta

I just would like to reiterate that i'm not convinced that we can come up with a sensible yet sufficiently generic interface for this, which is why i would like to reiterate that it would perhaps make sense to start with adding the ability to have per-benchmark custom context first.

LebedevRI avatar Sep 14 '22 17:09 LebedevRI

@LebedevRI, I am proposing a new AddCustomContext method on the Benchmark object. Did you have something else in mind?

matta avatar Sep 14 '22 18:09 matta

Not without seeing the actual patch, no.

LebedevRI avatar Sep 14 '22 18:09 LebedevRI

@LebedevRI, I am proposing a new AddCustomContext method on the Benchmark object. Did you have something else in mind?

FWIW, just to reiterate, i still think implementing that would be good, if for nothing else, then for API consistency. Patches welcomed.

LebedevRI avatar Oct 25 '22 22:10 LebedevRI