Repository navigation
Conversation
dgryski
left a comment
dgryski
left a comment
There was a problem hiding this comment.
Missing handling an error condition to match upstream.
| // iteration count is not scaled inside Loop. runN already ramps b.N, so Loop | ||
| // simply runs b.N iterations per call of the benchmark function. | ||
| func (b *B) Loop() bool { | ||
| panic("unimplemented: testing.B.Loop") |
There was a problem hiding this comment.
B.Loop should panic if called with the timer stopped, for example if you stop the timer during your iteration and forget to start it again.
There was a problem hiding this comment.
Right, didn't look too closely at the upstream impl. Will now panic if !b.timerOn 👍
c885179 to
3a2e448
Compare
deadprogram
left a comment
deadprogram
left a comment
There was a problem hiding this comment.
Thanks for the fixes @msujew. I ran the tests on my local machine and took a good look. The following is edited from an automated review:
-
Upstream
B.Loopalso promises that the compiler will not optimize away the loop body. That does not hold in TinyGo. When the body's input does not change between iterations, LLVM hoists or removes it, even when the result is stored to a global. A body likefor b.Loop() { sink = f(size) }measures about 1.2 ns/op in TinyGo and about 900 ns/op in Go. The oldfor i := 0; i < b.N; i++style has the same problem, so this PR does not make it worse. Bodies that depend on the previous iteration measure correctly in both styles.Fixing it properly would need compiler support, so it is out of scope here. I am mentioning it in this PR just in case someone reads it in the future.
|
@deadprogram Good point re: lack of optimization protection. I wonder if we should add that as a comment to |
@dgryski good point. @msujew can you please do this before we merge? |
3a2e448 to
2348231
Compare
|
Sure, I've left #5917 and added a comment to |
|
@msujew can you please rebase against the latest |
Benchmarks written against the Go 1.24 API call b.Loop() and currently
die with:
panic: unimplemented: testing.B.Loop
Loop runs b.N iterations per invocation of the benchmark function.
runN already ramps b.N, so no iteration scaling is done inside Loop.
Like upstream, the timer is reset on the first call and stopped when
Loop returns false, so setup and cleanup are not measured.
b.extra was never allocated, so any call to ReportMetric failed with:
panic: assignment to entry in nil map
Allocate the map in ResetTimer like upstream Go does. runN calls
ResetTimer before every invocation of the benchmark function, so each
run starts with an empty map and the metrics end up in b.result.
B.Context() returned a nil interface, so benchmarks that pass it into
code calling ctx.Err() or ctx.Done() crashed with:
panic: runtime error: nil pointer dereference
Create the context in runN like upstream Go does, so every B built by
runBenchmarks, B.Run or Benchmark gets one. runCleanup already cancels
it.
2348231 to
3233238
Compare
3 participants
Fixes #5830
ReportMetric: Uses the same code that upstream Go uses to allocate the map.Loop: An implementation that scalesN(like upstream Go) would require more involved changes. This is a minimal implementation that roughly does what it's supposed to do.common.ctxandcommon.cancelCtxwere never set, leading tonilreturn values onB.Context()(eventually resulting in panics).