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

refactor performance test to be standard golang benchmarks #432

Closed
wants to merge 2 commits into from

Conversation

hickford
Copy link
Contributor

@hickford hickford commented Jun 25, 2023

Fixes #433

Go has built in benchmarking feature https://pkg.go.dev/testing#hdr-Benchmarks

Example output

goos: linux
goarch: amd64
pkg: github.com/AzureAD/microsoft-authentication-library-for-go/apps/tests/performance
cpu: 12th Gen Intel(R) Core(TM) i7-1265U
Benchmark1User10000Tokens-12                2476            414902 ns/op

Benchmark1User100000Tokens-12      
     100          16092443 ns/op
Benchmark10Users10000Tokens-12               716           1592947 ns/op
Benchmark100Users10000Tokens-12              654           1681811 ns/op
Benchmark1000Users1000Tokens-12             8030            142265 ns/op
Benchmark10000Users100Tokens-12            74020             18107 ns/op
PASS
ok      github.com/AzureAD/microsoft-authentication-library-for-go/apps/tests/performance       47.578s

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@chlowell
Copy link
Collaborator

Thanks for doing this! This is nitpicking, but it appears these benchmarks could be consolidated as sub-benchmarks parameterized by user and token counts i.e., something like this:

func BenchmarkCache(b *testing.B) {
	for _, bm := range []struct {
		users, tokens int
	}{
		{100, 10_000},
	} {
		b.Run(fmt.Sprintf("%d users %d tokens", bm.users, bm.tokens), func(b *testing.B) {
			// inline benchmark() and perhaps also queryCache()
		})
	}
}

...which would be tidier. I haven't tried it though, maybe there's a catch. What do you think?

@hickford
Copy link
Contributor Author

Great idea, I agree that’s better

@bgavrilMS
Copy link
Member

Would be nice to integrate with https://github.com/benchmark-action/github-action-benchmark

@bgavrilMS
Copy link
Member

Do we still want to merge this in?

@chlowell
Copy link
Collaborator

I wouldn't block anything on it but I think we should merge this (after tidying it up into sub-benchmarks as I described above). It's weird to benchmark with a unit test printing to console instead of go's standard benchmarking framework.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.3% 0.3% Duplication

@hickford
Copy link
Contributor Author

after tidying it up into sub-benchmarks

@chlowell Done, ready for review

Copy link

sonarqubecloud bot commented Dec 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.3% 0.3% Duplication

@bgavrilMS bgavrilMS deleted the branch AzureAD:dev December 7, 2023 16:54
@bgavrilMS bgavrilMS closed this Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance tests should be Go benchmarks
3 participants