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

remove the unnecessary call to ResetTimer and StopTimer #6185

Merged
merged 2 commits into from Apr 14, 2023

Conversation

ethanvc
Copy link
Contributor

@ethanvc ethanvc commented Apr 11, 2023

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

remove the unnecessary call to ResetTimer and StopTimer
@ethanvc ethanvc changed the title remove the unnecessary call to ResetTimer and StopTimer #6184 remove the unnecessary call to ResetTimer and StopTimer Apr 11, 2023
@zasweq zasweq self-requested a review April 11, 2023 19:00
@zasweq zasweq self-assigned this Apr 11, 2023
@zasweq
Copy link
Contributor

zasweq commented Apr 11, 2023

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

@zasweq zasweq assigned ethanvc and unassigned zasweq Apr 11, 2023
@ethanvc
Copy link
Contributor Author

ethanvc commented Apr 12, 2023

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 ethanvc removed their assignment Apr 13, 2023
@ethanvc
Copy link
Contributor Author

ethanvc commented Apr 13, 2023

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

@zasweq
Copy link
Contributor

zasweq commented Apr 13, 2023

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

@ethanvc
Copy link
Contributor Author

ethanvc commented Apr 13, 2023

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

@zasweq
Copy link
Contributor

zasweq commented Apr 13, 2023

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 easwars added this to the 1.55 Release milestone Apr 14, 2023
@easwars
Copy link
Contributor

easwars commented Apr 14, 2023

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
Copy link
Contributor

easwars commented Apr 14, 2023

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

@easwars
Copy link
Contributor

easwars commented Apr 14, 2023

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

@zasweq
Copy link
Contributor

zasweq commented Apr 14, 2023

Ok I'll approve it then.

Copy link
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@ethanvc
Copy link
Contributor Author

ethanvc commented Apr 14, 2023

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

done

@ethanvc
Copy link
Contributor Author

ethanvc commented Apr 14, 2023

@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.

@easwars easwars merged commit d90621f into grpc:master Apr 14, 2023
11 checks passed
@zasweq
Copy link
Contributor

zasweq commented Apr 14, 2023

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

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants