proposal: testing: add B method for adding stats

[josharian]

Package testing has built-in support for three statistics: wall time, allocations, and memory allocated. There is discussion of adding more: #24918.

I propose that we also expose to users the ability to report their own benchmark-specific statistics. An example comes from package sort (proximally via #26031): The number of calls to Swap and Less.

Proposed API:

func (b *B) ReportStats(n int, units string)

This will cause benchmark invocations to include <n/b.N> <units>/op in the benchmark output. Then the sort of output reported in #26031 could be used as input to benchstat.

[josharian]

On a second read, perhaps ReportMetric or just Report would be a better name. And perhaps it should accept an int64.

[rsc]

Not a float64?

/cc @aclements

[josharian]

Not a float64?

I couldn't think of any non-integral metrics, and an int has the advantage of making it clear that the intended usage is to report a total count, not count/b.N. But I'm not opposed to a float64.

We'd also want to make sure that downstream tools (like benchstat) are prepared to parse floats, on pain of ending up in a weird situation similar to testing.AllocsPerRun: AllocsPerRun returns the average number of allocations during calls to f. Although the return value has type float64, it will always be an integral value.

[bradfitz]

Seems fine to me and @ianlancetaylor, @aclements, @rsc?

[aclements]

Yes please!

Though I'm not so sure about implicitly dividing by b.N or automatically adding "/op". Many custom benchmarks report metrics that are not per-op. gcbench reports many metrics that are aggregated differently across iterations, such as quantiles and time-rates. x/benchmarks reports things like peak RSS (arguably also a quantile). benchcmd is similar.

Users can easily divide by b.N themselves, or we could provide two methods to make the distinction clear.

In this case, the type should clearly be float64, though I would argue that it should be float64 even if the framework implicitly divides by b.N. We already made this mistake with the most common metric: "time" is not an integral resource, and we have to multiply it by 10⁹ in order to pretend that it is.

[aclements]

This may be a dup of #16110, though the discussion there got a bit confusing.

[josharian]

This may be a dup of #16110, though the discussion there got a bit confusing.

I agree. I'll close that one in favor of this, since this one seems to be moving along.

Users can easily divide by b.N themselves, or we could provide two methods to make the distinction clear.

I'm inclined to add only a single method, and use examples and docs to get the rest of the way there. About out of laptop time, will update this proposal with something concrete soon.

[josharian]

New proposal:

// ReportMetric adds n/unit to the reported benchmark results.
// If the metric is per-iteration, the caller should divide by b.N,
// and by convention units should end in "/op".
// ReportMetric will panic if unit is the empty string
// or if unit contains any whitespace.
func (b *testing.B) ReportMetric(n float64, unit string)

Examples to include something like a swap count (sorting) and a cache hit rate (sync.Pool? other?).

BenchmarkResult should also have a new field:

Extra map[string]float64 // Extra records additional metrics, reported by ReportMetric

Open question: What should we do if ReportMetric is called multiple times with the same unit? (Overwrite? Panic? Ignore?) What if ReportMetric is called with a reserved unit like "ns/op"?

I'm inclined to answer overwrite and panic, respectively, although documenting that is annoying.

Ready for a new round of input, thanks!

[aclements]

SGTM.

Open question: What should we do if ReportMetric is called multiple times with the same unit? (Overwrite? Panic? Ignore?) What if ReportMetric is called with a reserved unit like "ns/op"?

I agree that it should overwrite if called multiple times with the same unit (anything else seems more annoying or needlessly constrained).

I worry more about panicking on reserved units. Is "allocs/op" always reserved or only with -benchmem? What if we add more built-in units in the future (e.g., for #21295). Since the set is somewhat dynamic, that would make the panicking behavior somewhat unpredictable. I would lean toward overwriting the built-in measure, but that's a bit messy to implement. And, arguably, if a later call to ReportMetric overwrites the earlier value, then it should just ignore reserved units, since those are being reported by the benchmark framework at the end of the benchmark and hence would "override" any user-reported values.

How does this interact with parallel benchmarks?

[rsc]

In the description two messages up, "n/unit" should be "n unit" I think. Otherwise this seems OK. Will leave for Austin to accept the proposal when ready.

[rsc]

@josharian, @aclements, any ideas about parallel benchmarks or other details needed to accept this proposal?

[aclements]

I think if we're taking the value and unit from the user at face value, parallel benchmarks don't present any particular complication. We'll just report the value and unit in the benchmark line, whether or not it's parallel. (I'm not sure what, if anything, I had in mind when I questioned how this would interact with parallel benchmarks.)

So, tweaking Josh's proposal, how about:

// ReportMetric adds n unit to the reported benchmark results.
// If the metric is per-iteration, the caller should divide by b.N,
// and by convention units should end in "/op".
// If ReportMetric is called multiple times for the same unit,
// later calls override earlier calls.
// If ReportMetric is called for a unit normally reported by the
// benchmark framework itself (such as "ns/op"), n will override
// the value reported by the benchmark framework.
// ReportMetric will panic if unit is the empty string
// or if unit contains any whitespace.
func (b *testing.B) ReportMetric(n float64, unit string)

plus extending BenchmarkResult with

Extra map[string]float64 // Extra records additional metrics, reported by ReportMetric