Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

testing: add an example showcasing B.RunParallel with B.ReportMetric #50756

Open
mvdan opened this issue Jan 22, 2022 · 1 comment
Open

testing: add an example showcasing B.RunParallel with B.ReportMetric #50756

mvdan opened this issue Jan 22, 2022 · 1 comment
Labels
Documentation NeedsFix Suggested

Comments

@mvdan
Copy link
Member

@mvdan mvdan commented Jan 22, 2022

I had a parallel benchmark in the form of:

var totalFoo int64
b.RunParallel(func(pb *testing.PB) {
	for pb.Next() {
		// benchmark...
		totalFoo += someFoo()
	}
})
b.ReportMetric(float64(totalAmount)/float64(b.N), "foo/op")

I was taking a second look at the benchmark because dividing by b.N seemed fishy. This is a parallel benchmark, where I iterate via pb.Next and not b.N, so can I rely on b.N being an accurate detail of how many iterations were run across all goroutines?

The docs seem to imply so:

RunParallel runs a benchmark in parallel. It creates multiple goroutines and distributes b.N iterations among them.

ReportMetric adds "n unit" to the reported benchmark results. If the metric is per-iteration, the caller should divide by b.N

Still, I had to re-read the docs multiple times to convince myself. I think it would be nice to add an example on ReportMetric for parallel benchmarks, such as func ExampleB_ReportMetric_parallel, to help clarify the interaction between the two and how they are meant to be used.

And there's another reason why an example would be good: since RunParallel hides the goroutine spawning from the user, it's relatively easy to introduce data races by mistake, like I did above! totalFoo += someFoo() would need to be something like atomic.AddInt64(&totalFoo, someFoo()). The example could also show that.

Marking as a good candidate for first time contributors. Happy to review it too.

cc @mpvl per https://dev.golang.org/owners

@mvdan mvdan added Suggested Documentation NeedsFix labels Jan 22, 2022
@mvdan
Copy link
Member Author

@mvdan mvdan commented Jan 22, 2022

It might also be a good idea for RunParallel to call out that using b.N for the loop is ill advised, but it can still be used as the total number of iterations that are to be run - either during initial setup work or while collecting metrics at the end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation NeedsFix Suggested
Projects
None yet
Development

No branches or pull requests

1 participant