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

Closed
mvdan opened this issue Jan 22, 2022 · 3 comments
Closed

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

mvdan opened this issue Jan 22, 2022 · 3 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Suggested Issues that may be good for new contributors looking for work to do.
Milestone

Comments

@mvdan
Copy link
Member

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 Issues that may be good for new contributors looking for work to do. Documentation NeedsFix The path to resolution is known, but the work has not been done. labels Jan 22, 2022
@mvdan
Copy link
Member Author

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.

@gopherbot
Copy link

Change https://go.dev/cl/415035 mentions this issue: testing: give an example for ReportMetrics in a parallel benchmark

elisshafer added a commit to elisshafer/go that referenced this issue Jun 29, 2022
…mark.

Show how to use b.N within a parallel benchmark at the setup and
reporting phases of the benchmark. Indicate that atomic operations must
be used within the benchmark for loop and that b.N can reliably be
derefenced from outside of the benchmark for loop.

Fixes golang#50756
elisshafer added a commit to elisshafer/go that referenced this issue Jun 29, 2022
…mark

Show how to use b.N within a parallel benchmark at the setup and
reporting phases of the benchmark. Indicate that atomic operations must
be used within the benchmark for loop and that b.N can reliably be
derefenced from outside of the benchmark for loop.

Fixes golang#50756
@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
@gopherbot
Copy link

Change https://go.dev/cl/437815 mentions this issue: testing: add an example showcasing B.RunParallel with B.ReportMetric

romaindoumenc pushed a commit to TroutSoftware/go that referenced this issue Nov 3, 2022
This commit was dedicated to adding an example of using B.ReportMetrics
with B.RunParallel called ExampleB_ReportMetric_parallel. In this
example, the same algorithm for ExampleB_ReportMetric was used, instead
with a concurrent for loop using PB.Next instead of a standard one.
There is also notes noting when to use the B.ReportMetric methods when
running concurrent testing.

Fixes golang#50756
Change-Id: I2a621b4e367af5f4ec47d38a0da1035a8d52f628
Reviewed-on: https://go-review.googlesource.com/c/go/+/437815
Reviewed-by: Carlos Amedee <carlos@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
@golang golang locked and limited conversation to collaborators Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Suggested Issues that may be good for new contributors looking for work to do.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants