remove the unnecessary call to ResetTimer and StopTimer

[ethanvc]

remove the unnecessary call to ResetTimer and StopTimer. which will misleading other people.

In the implemation of testing.B:

// runN runs a single benchmark for the specified number of iterations.
func (b *B) runN(n int) {
	benchmarkLock.Lock()
	defer benchmarkLock.Unlock()
	defer b.runCleanup(normalPanic)
	// Try to get a comparable environment for each run
	// by clearing garbage from previous runs.
	runtime.GC()
	b.raceErrors = -race.Errors()
	b.N = n
	b.parallelism = 1
	b.ResetTimer()
	b.StartTimer()
	b.benchFunc(b) // **here, no need to call ResetTimer and StopTimer to cover benchmark function.**
	b.StopTimer()
	b.previousN = n
	b.previousDuration = b.duration
	b.raceErrors += race.Errors()
	if b.raceErrors > 0 {
		b.Errorf("race detected during execution of benchmark")
	}
}

The testing framework already called ResetTimer/StopTimer before call user's benchmark function.

RELEASE NOTES: none

[zasweq]

Is it not more accurate (i.e. the measurements of the timer) to call this in the function body rather than the caller?

[ethanvc]

The answer is no need. First of all, i will give you a test data:

go test -bench . -benchmem code_string_test.go
goos: darwin
goarch: arm64
BenchmarkCodeStringStringer-10                  518726848                2.268 ns/op           0 B/op          0 allocs/op
BenchmarkCodeStringStringerX-10                 520579990                2.263 ns/op           0 B/op          0 allocs/op
BenchmarkCodeStringMap-10                       236651772                5.128 ns/op           0 B/op          0 allocs/op
BenchmarkCodeStringMapX-10                      233088195                5.087 ns/op           0 B/op          0 allocs/op
BenchmarkCodeStringSwitch-10                    424234076                2.829 ns/op           0 B/op          0 allocs/op
BenchmarkCodeStringSwitchX-10                   422929536                2.829 ns/op           0 B/op          0 allocs/op
BenchmarkCodeStringStringerWithOverflow-10      334875387                3.598 ns/op           0 B/op          0 allocs/op
BenchmarkCodeStringStringerWithOverflowX-10     331276580                3.600 ns/op           0 B/op          0 allocs/op
BenchmarkCodeStringSwitchWithOverflow-10        269804214                4.458 ns/op           0 B/op          0 allocs/op
BenchmarkCodeStringSwitchWithOverflowX-10       261088226                4.459 ns/op           0 B/op          0 allocs/op
BenchmarkFuncCall-10                            1000000000               0.9354 ns/op          0 B/op          0 allocs/op
PASS
ok      command-line-arguments  18.049s

All the test cases tailed with X is the version removed ResetTimer/StopTimer. and you can see, there is no observable difference between them.

and in the theory analysis, testing will do increase b.N to make the convergence of the op time and remove the effect of the non inline call to bench function.

finally, it not a better practice to add ResetTImer/StopTImer wrapper the whole benchmark function. an the testing package already to a better work for us.

[ethanvc]

@zasweq please help solve this issue when you have time.

[zasweq]

I'm seeing the same or greater nanoseconds/operation for each benchmark. Thoughts @easwars?

[ethanvc]

half of thie tests show the version removed is more faster, maybe you misunderstand the report? @zasweq

[zasweq]

Yes, I see 2/5 ones getting faster, 1 of them equal, and 2/5ths getting slower. Thus, this doesn't seem to improve it.

[easwars]

I'm seeing the same or greater nanoseconds/operation for each benchmark. Thoughts @easwars?

This code was written 6 years ago, and at that time we probably didn't understand how to use the benchmarking support in the testing package.

[easwars]

@ethanvc : Could you please rebase your branch to master. We recently fixed a flaky test which is affecting you branch. Thanks.

[easwars]

@zasweq : I'm fine with these changes. Assigning back for second set of eyes.

[zasweq]

Ok I'll approve it then.

[zasweq]

LGTM.

[ethanvc]

@ethanvc : Could you please rebase your branch to master. We recently fixed a flaky test which is affecting you branch. Thanks.

done

[ethanvc]

@zasweq

This MR is not intended to improve performance or make these benchmark tests more accurate. Instead, the purpose of this MR is to use the testing library correctly.

From an intent perspective, wrapping the benchmark test function with ResetTimer and StopTimer is meant to eliminate the error caused by the inability to inline (function pointer calls) when the testing library invokes the base test function. However, the original author did not consider that the testing library would adjust the value of N to mitigate this overhead, so we do not need to worry about this impact when using the testing library.

As you can see, adding ResetTimer does not affect the benchmark test results; it is doing unnecessary work. To my understanding, this has the same problem as the following code:

Copy code
func add(n int) int{
    n += 0
	n += 1
	return n
}

In the function above, the n += 0 statement is meaningless and only confuses maintainers.

As an influential open-source library, I believe that both procedural correctness and result correctness are equally important. Although these two versions of the code have no impact on the benchmark test results, the code itself may mislead others. It may lead people to believe that calling ResetTimer and StopTimer before and after a benchmark test can perceptibly improve the accuracy of the benchmark test. However, this understanding is incorrect.

[zasweq]

Ok, sounds good, thanks for the contribution/explanation :).