GODRIVER-2361 Refactor pseudo-random number and UUID generation with "golang.org/x/exp/rand" #945

qingyang-hu · 2022-05-11T20:58:07Z

GODRIVER-2361 Refactor pseudo-random number and UUID generation with "golang.org/x/exp/rand".

Third-party packages are imported into "internal/uuid/uuid" and "internal/randutil/rand" in commit 7633425.
The refactoring is in commit d4defe6.

There is not any modification to the third-party code.

Two files are impacted after the refactoring:
x/mongo/driver/connstring/connstring.go
x/mongo/driver/topology/topology.go

I put the LICENSE files (3-Clause BSD License copied from the original source repo) with the third-party packages. Please let me know if we follow a different practice.

…"golang.org/x/exp/rand"

prestonvasquez

Great work!

prestonvasquez · 2022-05-12T18:34:49Z

internal/uuid/uuidutil_test.go

+		var wg sync.WaitGroup
+		for i := 1; i < 1000000; i++ {
+			wg.Add(1)


Suggested change

var wg sync.WaitGroup

for i := 1; i < 1000000; i++ {

wg.Add(1)

iterations := 1e6

wg := sync.WaitGroup{}.Add(iterations)

for i := 1; i < threshold; i++ {

I think this will help ensure this subtest is consistent with the other subtests. I think decoupling Add from the loop will also make the test safer, although it's probably fine as is.

Good point! Thank you!

prestonvasquez · 2022-05-12T18:36:34Z

internal/uuid/uuidutil_test.go

+		var wg sync.WaitGroup
+		uuids := new(sync.Map)
+		for i := 0; i < iterations; i++ {
+			wg.Add(1)


Suggested change

var wg sync.WaitGroup

uuids := new(sync.Map)

for i := 0; i < iterations; i++ {

wg.Add(1)

wg := sync.WaitGroup{}.Add(iterations)

uuids := new(sync.Map)

for i := 0; i < iterations; i++ {

prestonvasquez · 2022-05-12T18:38:02Z

internal/uuid/uuidutil_test.go

+		var wg sync.WaitGroup
+		for i := 1; i < 1000000; i++ {
+			wg.Add(1)


Suggested change

var wg sync.WaitGroup

for i := 1; i < 1000000; i++ {

wg.Add(1)

iterations := 1e6

wg := sync.WaitGroup{}.Add(iterations)

for i := 1; i < iterations; i++ {

prestonvasquez · 2022-05-12T18:38:44Z

internal/uuid/uuidutil_test.go

+		var wg sync.WaitGroup
+		uuids := new(sync.Map)
+		for i := 0; i < iterations; i++ {
+			wg.Add(1)


Suggested change

var wg sync.WaitGroup

uuids := new(sync.Map)

for i := 0; i < iterations; i++ {

wg.Add(1)

wg := sync.WaitGroup{}.Add(iterations)

uuids := new(sync.Map)

for i := 0; i < iterations; i++ {

prestonvasquez · 2022-05-12T18:44:37Z

internal/uuid/uuidutil.go

+func Equal(a, b UUID) bool {
+	return a == b
+}


Reading through the google/uuid package, I notice that they don't actually have a routine for equality. That's kind of curious, and I really like that we are adding this. What are your thoughts on making it a receiver for the UUID type like what the time pacakge does?

Suggested change

func Equal(a, b UUID) bool {

return a == b

}

func (u UUID) Equal(b UUID) bool {

return u == b

}

Actually the Equal function seems unnecessary because users can compare UUIDs directly with the equality operator (the Equal code just uses the == operator). We should consider removing that function.

matthewdale · 2022-05-17T18:18:54Z

internal/uuid/uuid/marshal.go

+import "fmt"
+
+// MarshalText implements encoding.TextMarshaler.
+func (uuid UUID) MarshalText() ([]byte, error) {


Do we use these UUID marshalers and unmarshalers anywhere? If we don't use them anywhere, can we exclude this file?

matthewdale · 2022-05-17T18:26:58Z

internal/uuid/uuidutil.go

+// globalSource is a package-global pseudo-random UUID generator.
+var globalSource = func() *source {
+	uuid.SetRand(randutil.GlobalRand)
+	uuid.EnableRandPool()


If we're using a pseudo-random number generator for the UUID randomness source, are there any benefits to enable random value pooling?

You are right. A new benchmark test showed pooling did not bring any benefit in this scenario. Thus, I removed the github.com/google/uuid package as well.

matthewdale · 2022-05-17T18:36:25Z

x/mongo/driver/topology/topology.go

@@ -461,7 +457,7 @@ func (t *Topology) SelectServer(ctx context.Context, ss description.ServerSelect
 // provided, pick2 will panic.
 func pick2(ds []description.Server) (description.Server, description.Server) {
 	// Select a random index from the input slice and keep the server description from that index.
-	idx := random.Intn(len(ds))
+	idx := randutil.GlobalRand.Intn(len(ds))


Is there any performance impact to using the same randutil.GlobalRand for selecting servers as generating UUIDs? Since GlobalRand uses a mutex to ensure sequential calls, it seems like it could add lock contention to different parts of the driver.

matthewdale · 2022-05-17T22:50:38Z

internal/randutil/rand/exp.go

@@ -0,0 +1,221 @@
+// Copyright 2009 The Go Authors. All rights reserved.


Please add a comment somewhere at the top of all files copied from the "golang.org/x/exp/rand" package to indicate where they're from and what version they are.

E.g.

// Copied from https://cs.opensource.google/go/x/exp/+/24438e51023af3bfc1db8aed43c1342817e8cfcd:rand/exp.go

matthewdale · 2022-05-17T23:50:59Z

internal/randutil/rand/vendor/math/bits/LICENSE

@@ -0,0 +1,27 @@
+Copyright (c) 2009 The Go Authors. All rights reserved.


I'm concerned that vendoring the newer version of the full "math/bits" is introducing a lot of code and dependency complexity (e.g. "How does the nested vendor directory work with different versions of Go and different dependency strategies?") just to add the Add64 and Mul64 functions.

There are a few alternatives that could allow us to use the "golang.org/x/exp/rand" pRNG with less dependency complexity:

Copy in only Add64 and Mul64 functions from the latest "math/bits" library to "internal/math/bits" and update the relevant code in rng.go to use the copied-in versions of Add64 and Mul64.

Only use "golang.org/x/exp/rand" when building with Go 1.12+. Requires adding versions of the randutil package that are built for Go 1.12+ and Go <1.12 using Go build constraints (e.g. // +build go1.12), using the "golang.org/x/exp/rand" pRNG for Go 1.12+ and "math/rand" pRNG for Go <1.12.

Go build constraints require a lot of testing with different Go versions to validate, so I usually try to avoid them. Because of that, I think option 1 is easier and more robust, although it does require lightly modifying copied-in code. Does that seem like a positive change?

matthewdale · 2022-05-18T00:01:25Z

internal/randutil/rand/LICENSE

@@ -0,0 +1,27 @@
+Copyright (c) 2009 The Go Authors. All rights reserved.


The current pattern for license/copyright information is to copy the license into THIRD-PARTY-NOTICES with a reference to the code or package it references. Is that sufficient in this case or do we need to retain this LICENSE file in the package?

matthewdale · 2022-05-18T00:03:23Z

internal/uuid/uuidutil_test.go

+// GODRIVER-2349
+// Test that initializing many package-global UUID sources concurrently never leads to any duplicate
+// UUIDs being generated.
+func TestGlobalSource(t *testing.T) {


Nice tests and comment referencing the GODRIVER ticket!

…"golang.org/x/exp/rand"

prestonvasquez

Looks great, just a few suggestions.

internal/uuid/uuid_test.go

prestonvasquez · 2022-05-19T21:08:46Z

internal/uuid/uuid_test.go

+				uuid, err := New()
+				require.NoError(t, err, "new() error")
+				_, ok := uuids.Load(uuid)
+				require.Falsef(t, ok, "New returned a duplicate UUID on iteration %d: %v", i, uuid)


Suggested change

require.Falsef(t, ok, "New returned a duplicate UUID on iteration %d: %v", i, uuid)

require.Falsef(t, ok, "goroutine %d returned a duplicate UUID on iteration %d: %v", i, j, uuid)

Can we return the same message from the other subtest?

matthewdale

The pRNG updates look great! One outstanding concern about the tests.

matthewdale · 2022-05-20T19:15:17Z

internal/uuid/uuid_test.go

+		for i := 0; i < iterations; i++ {
+			go func(i int) {
+				defer wg.Done()
+				uuid, err := New()


This currently doesn't test that initializing the pRNG randomness source many times always results in a distinct sequence of values. We either need to re-initialize the package-global variable globalRandom on each iteration (which would necessitate doing all UUID generation sequentially, i.e. no goroutines) or make a UUID source that can be instantiated many times.

You are right.

matthewdale

Looks great 👍

qingyang-hu added 2 commits May 11, 2022 16:27

GODRIVER-2361 Import third party packages

7633425

GODRIVER-2361 Refactor pseudo-random number and UUID generation with …

d4defe6

…"golang.org/x/exp/rand"

qingyang-hu requested review from matthewdale and prestonvasquez May 11, 2022 20:58

qingyang-hu changed the title ~~GODRIVER-2361 Refactor pseudo-random number and UUID generation with "golang .org/x/exp/rand"~~ GODRIVER-2361 Refactor pseudo-random number and UUID generation with "golang.org/x/exp/rand" May 11, 2022

prestonvasquez reviewed May 12, 2022

View reviewed changes

matthewdale reviewed May 18, 2022

View reviewed changes

qingyang-hu added 2 commits May 19, 2022 13:25

GODRIVER-2361 Refactor pseudo-random number and UUID generation with …

3625346

…"golang.org/x/exp/rand"

GODRIVER-2361 Update .golangci.yml

1cf9a66

qingyang-hu requested review from matthewdale and prestonvasquez May 19, 2022 17:55

prestonvasquez reviewed May 19, 2022

View reviewed changes

GODRIVER-2361 Touch up test case

0a81ef2

matthewdale reviewed May 20, 2022

View reviewed changes

prestonvasquez approved these changes May 20, 2022

View reviewed changes

GODRIVER-2361 Touch up test cases

a0988dd

qingyang-hu requested a review from matthewdale May 20, 2022 22:11

matthewdale approved these changes May 23, 2022

View reviewed changes

qingyang-hu merged commit ef8ad53 into mongodb:master May 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GODRIVER-2361 Refactor pseudo-random number and UUID generation with "golang.org/x/exp/rand" #945

GODRIVER-2361 Refactor pseudo-random number and UUID generation with "golang.org/x/exp/rand" #945

qingyang-hu commented May 11, 2022 •

edited

Loading

prestonvasquez left a comment

prestonvasquez May 12, 2022

qingyang-hu May 12, 2022

prestonvasquez May 12, 2022

prestonvasquez May 12, 2022

prestonvasquez May 12, 2022

prestonvasquez May 12, 2022

matthewdale May 18, 2022

matthewdale May 17, 2022

matthewdale May 17, 2022 •

edited

Loading

qingyang-hu May 19, 2022

matthewdale May 17, 2022

matthewdale May 17, 2022

qingyang-hu May 19, 2022

matthewdale May 17, 2022

matthewdale May 18, 2022

matthewdale May 18, 2022

prestonvasquez left a comment

prestonvasquez May 19, 2022

matthewdale left a comment

matthewdale May 20, 2022

qingyang-hu May 20, 2022

matthewdale left a comment

		@@ -0,0 +1,221 @@
		// Copyright 2009 The Go Authors. All rights reserved.

		@@ -0,0 +1,27 @@
		Copyright (c) 2009 The Go Authors. All rights reserved.

	require.Falsef(t, ok, "New returned a duplicate UUID on iteration %d: %v", i, uuid)
	require.Falsef(t, ok, "goroutine %d returned a duplicate UUID on iteration %d: %v", i, j, uuid)

GODRIVER-2361 Refactor pseudo-random number and UUID generation with "golang.org/x/exp/rand" #945

GODRIVER-2361 Refactor pseudo-random number and UUID generation with "golang.org/x/exp/rand" #945

Conversation

qingyang-hu commented May 11, 2022 • edited Loading

prestonvasquez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewdale May 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestonvasquez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewdale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewdale left a comment

Choose a reason for hiding this comment

qingyang-hu commented May 11, 2022 •

edited

Loading

matthewdale May 17, 2022 •

edited

Loading