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

internal/grpcrand: use Go top-level random functions for go1.21+ #6925

Merged
merged 3 commits into from Feb 1, 2024

Conversation

kmirzavaziri
Copy link
Contributor

@kmirzavaziri kmirzavaziri commented Jan 17, 2024

This PR closes issue #6650.

What I did
I duplicated all functions in grpcrand package into another file, changing the implementation to call top-level functions of Go math/rand package internally, adding the _go1.21 suffix to new file name and build comments to both files, to instruct the Go compiler to compile the new file instead of the original one for Go 1.21 or higher.

Why
As described in the related issue, apart from reducing complexity, it is concurrent safe as of Go 1.21, and also significantly faster. I ran a benchmark to demonstrate the difference.

import (
	"math/rand"
	"sync"
	"testing"
	"time"
)

var (
	r  = rand.New(rand.NewSource(time.Now().UnixNano()))
	mu sync.Mutex
)

func BenchmarkIntMutex(b *testing.B) {
	for i := 0; i < b.N; i++ {
		IntMutex()
	}
}

func BenchmarkIntTopLevel(b *testing.B) {
	for i := 0; i < b.N; i++ {
		IntTopLevel()
	}
}

func BenchmarkInt63nMutex(b *testing.B) {
	for i := 0; i < b.N; i++ {
		IntMutex()
	}
}

func BenchmarkInt63nTopLevel(b *testing.B) {
	for i := 0; i < b.N; i++ {
		IntTopLevel()
	}
}

func BenchmarkIntnMutex(b *testing.B) {
	for i := 0; i < b.N; i++ {
		IntMutex()
	}
}

func BenchmarkIntnTopLevel(b *testing.B) {
	for i := 0; i < b.N; i++ {
		IntTopLevel()
	}
}

func BenchmarkInt31nMutex(b *testing.B) {
	for i := 0; i < b.N; i++ {
		IntMutex()
	}
}

func BenchmarkInt31nTopLevel(b *testing.B) {
	for i := 0; i < b.N; i++ {
		IntTopLevel()
	}
}

func BenchmarkFloat64Mutex(b *testing.B) {
	for i := 0; i < b.N; i++ {
		IntMutex()
	}
}

func BenchmarkFloat64TopLevel(b *testing.B) {
	for i := 0; i < b.N; i++ {
		IntTopLevel()
	}
}

func BenchmarkUint64Mutex(b *testing.B) {
	for i := 0; i < b.N; i++ {
		IntMutex()
	}
}

func BenchmarkUint64TopLevel(b *testing.B) {
	for i := 0; i < b.N; i++ {
		IntTopLevel()
	}
}

func BenchmarkUint32Mutex(b *testing.B) {
	for i := 0; i < b.N; i++ {
		IntMutex()
	}
}

func BenchmarkUint32TopLevel(b *testing.B) {
	for i := 0; i < b.N; i++ {
		IntTopLevel()
	}
}

func BenchmarkExpFloat64Mutex(b *testing.B) {
	for i := 0; i < b.N; i++ {
		IntMutex()
	}
}

func BenchmarkExpFloat64TopLevel(b *testing.B) {
	for i := 0; i < b.N; i++ {
		IntTopLevel()
	}
}

func BenchmarkShuffleMutex(b *testing.B) {
	for i := 0; i < b.N; i++ {
		IntMutex()
	}
}

func BenchmarkShuffleTopLevel(b *testing.B) {
	for i := 0; i < b.N; i++ {
		IntTopLevel()
	}
}

func IntMutex() int {
	mu.Lock()
	defer mu.Unlock()
	return r.Int()
}

func Int63nMutex(n int64) int64 {
	mu.Lock()
	defer mu.Unlock()
	return r.Int63n(n)
}

func IntnMutex(n int) int {
	mu.Lock()
	defer mu.Unlock()
	return r.Intn(n)
}

func Int31nMutex(n int32) int32 {
	mu.Lock()
	defer mu.Unlock()
	return r.Int31n(n)
}

func Float64Mutex() float64 {
	mu.Lock()
	defer mu.Unlock()
	return r.Float64()
}

func Uint64Mutex() uint64 {
	mu.Lock()
	defer mu.Unlock()
	return r.Uint64()
}

func Uint32Mutex() uint32 {
	mu.Lock()
	defer mu.Unlock()
	return r.Uint32()
}

func ExpFloat64Mutex() float64 {
	mu.Lock()
	defer mu.Unlock()
	return r.ExpFloat64()
}

func ShuffleMutex(n int, f func(int, int)) {
	mu.Lock()
	defer mu.Unlock()
	r.Shuffle(n, f)
}

func IntTopLevel() int {
	return r.Int()
}

func Int63nTopLevel(n int64) int64 {
	return rand.Int63n(n)
}

func IntnTopLevel(n int) int {
	return rand.Intn(n)
}

func Int31nTopLevel(n int32) int32 {
	return rand.Int31n(n)
}

func Float64TopLevel() float64 {
	return rand.Float64()
}

func Uint64TopLevel() uint64 {
	return rand.Uint64()
}

func Uint32TopLevel() uint32 {
	return rand.Uint32()
}

func ExpFloat64TopLevel() float64 {
	return rand.ExpFloat64()
}

func ShuffleTopLevel(n int, f func(int, int)) {
	rand.Shuffle(n, f)
}

results:

BenchmarkIntMutex-8                     86689021                13.69 ns/op
BenchmarkIntTopLevel-8                  464327850                2.581 ns/op
BenchmarkInt63nMutex-8                  87957193                13.74 ns/op
BenchmarkInt63nTopLevel-8               463219412                2.586 ns/op
BenchmarkIntnMutex-8                    87656822                13.71 ns/op
BenchmarkIntnTopLevel-8                 463425360                2.653 ns/op
BenchmarkInt31nMutex-8                  87715021                13.68 ns/op
BenchmarkInt31nTopLevel-8               464232123                2.577 ns/op
BenchmarkFloat64Mutex-8                 87786145                13.71 ns/op
BenchmarkFloat64TopLevel-8              463119002                2.583 ns/op
BenchmarkUint64Mutex-8                  87422935                13.73 ns/op
BenchmarkUint64TopLevel-8               462573543                2.605 ns/op
BenchmarkUint32Mutex-8                  87592838                13.80 ns/op
BenchmarkUint32TopLevel-8               461618060                2.607 ns/op
BenchmarkExpFloat64Mutex-8              87122502                15.18 ns/op
BenchmarkExpFloat64TopLevel-8           453928900                2.581 ns/op
BenchmarkShuffleMutex-8                 86752477                13.67 ns/op
BenchmarkShuffleTopLevel-8              465211470                2.585 ns/op

Making the functions ~5x faster.

RELEASE NOTES:

  • rand: improve performance and simplify implementation of grpcrand by adopting math/rand's top-level functions for go version 1.21.0 and newer.

Copy link

linux-foundation-easycla bot commented Jan 17, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: kmirzavaziri / name: Kamyar Mirzavaziri (4c3353e)
  • ✅ login: arvindbr8 / name: Arvind Bright (b7dc45f, f1811ed)

Copy link

codecov bot commented Jan 17, 2024

Codecov Report

Merging #6925 (f1811ed) into master (ddd377f) will increase coverage by 0.25%.
Report is 14 commits behind head on master.
The diff coverage is 77.77%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6925      +/-   ##
==========================================
+ Coverage   83.51%   83.76%   +0.25%     
==========================================
  Files         287      287              
  Lines       30920    30911       -9     
==========================================
+ Hits        25824    25894      +70     
+ Misses       4020     3957      -63     
+ Partials     1076     1060      -16     
Files Coverage Δ
internal/grpcrand/grpcrand_go1.21.go 77.77% <77.77%> (ø)

... and 34 files with indirect coverage changes

@kmirzavaziri
Copy link
Contributor Author

@dfawley, I would appreciate if you could take a look at this.

@arvindbr8 arvindbr8 added this to the 1.61 Release milestone Jan 23, 2024
@arvindbr8 arvindbr8 added the Type: Dependencies Updating/adding/removing dependencies label Jan 23, 2024
Copy link
Member

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @kmirzavaziri

@zasweq zasweq modified the milestones: 1.61 Release, 1.62 Release Jan 23, 2024
@arvindbr8 arvindbr8 linked an issue Jan 24, 2024 that may be closed by this pull request
@@ -1,3 +1,6 @@
//go:build !go1.21
// +build !go1.21
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind adding a TODO here that says something like:

"TODO: when this file is deleted (after Go 1.20 support is dropped), delete all of grpcrand and call the rand package directly."

?

Copy link
Member

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for my absence and thank you @arvindbr8

Copy link
Member

Choose a reason for hiding this comment

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

No worries, sorry if you were working on the branch. Since this was a oneliner change, and since you had also allowed edits to your PR by maintainers I went ahead and made the change.

@@ -1,3 +1,6 @@
//go:build !go1.21
// +build !go1.21
Copy link
Member

Choose a reason for hiding this comment

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

We don't need both syntaxes for this, since we don't support Go 1.16 or earlier. Just //go:build is fine.

Copy link
Member

Choose a reason for hiding this comment

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

Done

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

@dfawley dfawley changed the title Change grpcrand implementation to use Go top-level random functions for go1.21+ internal/grpcrand: use Go top-level random functions for go1.21+ Feb 1, 2024
@dfawley dfawley merged commit 6f63f05 into grpc:master Feb 1, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Dependencies Updating/adding/removing dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use math/rand top-level functions in grpcrand
4 participants