Skip to content

testing: improve the text of the b.Loop documentation #70787

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

Closed
cespare opened this issue Dec 11, 2024 · 4 comments
Closed

testing: improve the text of the b.Loop documentation #70787

cespare opened this issue Dec 11, 2024 · 4 comments
Labels
Documentation Issues describing a change to documentation. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@cespare
Copy link
Contributor

cespare commented Dec 11, 2024

Go version

tip

Output of go env in your module/workspace:

n/a

What did you do?

The documentation for b.Loop (upcoming in Go 1.24) currently reads:

// Loop returns true until b.N calls has been made to it.
//
// A benchmark should either use Loop or contain an explicit loop from 0 to b.N, but not both.
// After the benchmark finishes, b.N will contain the total number of calls to op, so the benchmark
// may use b.N to compute other average metrics.
//
// The parameters and results of function calls inside the body of "for b.Loop() {...}" are guaranteed
// not to be optimized away.
// Also, the local loop scaling for b.Loop ensures the benchmark function containing the loop will only
// be executed once, i.e. for such construct:
//
//	testing.Benchmark(func(b *testing.B) {
//			...(setup)
//			for b.Loop() {
//				...(benchmark logic)
//			}
//			...(clean-up)
//	}
//
// The ...(setup) and ...(clean-up) logic will only be executed once.
// Also benchtime=Nx (N>1) will result in exactly N executions instead of N+1 for b.N style loops.
func (b *B) Loop() bool

In my opinion this still needs quite a bit of polish. Some of the issues:

  • s/has/have/ in the first sentence
  • s/will contain/contains/, s/will only be/is only/g, s/will result/results/
  • What is "calls to op"?
  • I feel like "local loop scaling" is non-obvious.
  • "i.e. for such construct" needs rephrasing
  • Comma after "Also" (but also that is repetitive).
  • s/benchtime/-benchtime/

I feel like the best fix, though, is not a bunch of spot tweaks but a more comprehensive overhaul. I can take a stab at it but I think someone else would be better suited. (Writing good docs is hard!)

What did you see happen?

n/a

What did you expect to see?

n/a

@gabyhelp
Copy link

Related Issues

Related Code Changes

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@gopherbot gopherbot added the Documentation Issues describing a change to documentation. label Dec 11, 2024
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 11, 2024
@cagedmantis cagedmantis added this to the Go1.24 milestone Dec 11, 2024
@cagedmantis
Copy link
Contributor

cc @adonovan @neild JunyangShao

@cespare
Copy link
Contributor Author

cespare commented Dec 13, 2024

This is fixed by CL 635895.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/635895 mentions this issue: testing: improve B.Loop docs, use B.Loop in examples

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Issues describing a change to documentation. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants