Use fastRand instead of locked-rand in skiplist (#1173)

[jarifibrahim]

The math/rand package (https://golang.org/src/math/rand/rand.go) uses a global lock to allow concurrent access to the rand object. We do not write to skiplist concurrently and hence we don't need a lock at all.

This blog has some interesting details https://blog.sgmansfield.com/2016/01/the-hidden-dangers-of-default-rand/

The PR replaces math.Rand with ristretto/z.FastRand(). FastRand is much faster than math.Rand and rand.New(..) generators.

The change improves concurrent writes to skiplist by ~30%

func BenchmarkWrite(b *testing.B) {
	value := newValue(123)
	l := NewSkiplist(int64((b.N + 1) * MaxNodeSize))
	defer l.DecrRef()
	b.ResetTimer()
	b.RunParallel(func(pb *testing.PB) {
		rng := rand.New(rand.NewSource(time.Now().UnixNano()))
		for pb.Next() {
			l.Put(randomKey(rng), y.ValueStruct{Value: value, Meta: 0, UserMeta: 0})
		}
	})
}

name      old time/op  new time/op  delta
Write-16   657ns ± 3%   441ns ± 1%  -32.94%  (p=0.000 n=9+10)

This PR also fixes the test timed out after 10 minutes issue on teamcity (see build https://teamcity.dgraph.io/viewLog.html?buildId=36889&buildTypeId=Badger_UnitTests#testNameId521499096021883325 )
The problem here is that the test TestValueGCManaged tries to create many bufferes with random data

badger/value_test.go

Lines 119 to 124 in ea01d38

	sz := 64 << 10
	var wg sync.WaitGroup
	for i := 0; i < N; i++ {
	v := make([]byte, sz)
	rand.Read(v[:rand.Intn(sz)])

The rand.Read(...) acquires a global lock (https://golang.org/src/math/rand/rand.go) and since N=10,000 in the above test, it leads to starvation. We would have too many rand.Read(...) calls waiting to acquire the same lock. At the same time, every insert to skip list also needs to acquire the same lock

badger/skl/skl.go

Lines 168 to 174 in ea01d38

	func randomHeight() int {
	h := 1
	for h < maxHeight && rand.Uint32() <= heightIncrease {
	h++
	}
	return h
	}

Hence, the test wouldn't complete in 10 minutes and the build fails.

---

This change is 

[coveralls]

Coverage decreased (-0.2%) to 69.785% when pulling c9ed8f2 on ibrahim/global-rand-memtable into 0f2e629 on master.

[poonai]

we write concurrently while updating head i guess

[jarifibrahim]

we write concurrently while updating head i guess

We don't concurrently update the same skip list. When we update head, the memtable being used for new writes is a different one. See this

badger/db.go

Lines 869 to 884 in ea01d38

	case db.flushChan <- flushTask{mt: db.mt, vptr: db.vhead}:
	// After every memtable flush, let's reset the counter.
	atomic.StoreInt32(&db.logRotates, 0)
	// Ensure value log is synced to disk so this memtable's contents wouldn't be lost.
	err = db.vlog.sync(db.vhead.Fid)
	if err != nil {
	return err
	}
	db.opt.Debugf("Flushing memtable, mt.size=%d size of flushChan: %d\n",
	db.mt.MemSize(), len(db.flushChan))
	// We manage to push this task. Let's modify imm.
	db.imm = append(db.imm, db.mt)
	db.mt = skl.NewSkiplist(arenaSize(db.opt))
	// New memtable is empty. We certainly have room.

Note how we created a new memtable after pushing the existing one to the flush chan.

[ashish-goswami]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @ashish-goswami, @jarifibrahim, and @manishrjain)

---

skl/skl.go, line 83 at r1 (raw file):

	ref    int32
	arena  *Arena
	rng    rand.Source

*rand.Source would be nice.

---

skl/skl.go, line 173 at r1 (raw file):

func (s *Skiplist) randomHeight() int {
	h := 1
	for h < maxHeight && uint32(s.rng.Int63()) <= heightIncrease {

Not sure if it is a good idea to directly cast it to uint32.
This is how rand.Rand uses it.
You should cast it in same way as done below.

// Uint32 returns a pseudo-random 32-bit value as a uint32.
func (r *Rand) Uint32() uint32 { return uint32(r.Int63() >> 31) }

Another choice is to use rand.Rand instead of rand.Source which directly provides Uint32() method.

[jarifibrahim]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @ashish-goswami and @manishrjain)

---

skl/skl.go, line 83 at r1 (raw file):

Previously, ashish-goswami (Ashish Goswami) wrote…

*rand.Source would be nice.

I've changed this to *rand.Rand

---

skl/skl.go, line 173 at r1 (raw file):

Previously, ashish-goswami (Ashish Goswami) wrote…

Not sure if it is a good idea to directly cast it to uint32.
This is how rand.Rand uses it.
You should cast it in same way as done below.

// Uint32 returns a pseudo-random 32-bit value as a uint32.
func (r *Rand) Uint32() uint32 { return uint32(r.Int63() >> 31) }

Another choice is to use rand.Rand instead of rand.Source which directly provides Uint32() method.

This is great. Thank you so much. I've changed the code to use rand.Rand instead 👍

[ashish-goswami]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @ashish-goswami and @manishrjain)

[manishrjain]

Yes, do check if we are writing concurrently to the skiplist.

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jarifibrahim)

---

skl/skl.go, line 137 at r2 (raw file):

		arena:  arena,
		ref:    1,
		rand:   rand.New(rand.NewSource(time.Now().UnixNano())),

Note that local rand is not thread-safe. So, ensure that we don't do concurrent calls to rand.

[jarifibrahim]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @manishrjain)

---

skl/skl.go, line 137 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Note that local rand is not thread-safe. So, ensure that we don't do concurrent calls to rand.

I checked this. We don't do concurrent writes to memtable. All writes to memtable (except the head write) are done through writeCh which means they are sequential.

[coocood]

Why not use runtime.fastrand?

[jarifibrahim]

Why not use runtime.fastrand?

Interesting. I did some benchmarks and fastRand seems to be more than 2X faster than rand.NewSource(..)

goos: linux
goarch: amd64
pkg: github.com/dgraph-io/ristretto/z
BenchmarkFastRand-16      	1000000000	         0.292 ns/op
BenchmarkRandSource-16    	1000000000	         0.747 ns/op
BenchmarkRandGlobal-16    	 6822332	       176 ns/op
BenchmarkRandAtomic-16    	77950322	        15.4 ns/op
PASS
ok  	github.com/dgraph-io/ristretto/z	4.808s

https://github.com/dgraph-io/ristretto/pull/118/files#diff-a9ed67d3dcec1ff808f43c15fe731caeR289-R310

I'll change it. Thanks @coocood !

[manishrjain]

Reviewed 4 of 4 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jarifibrahim)

---

table/table.go, line 320 at r4 (raw file):

	readPos := t.tableSize

	y.AssertTruef(readPos > 0, "readPos less than zero: %d", readPos)

if readPos > 0 {
return errors.New(...)
}

[jarifibrahim]

Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @manishrjain)

---

table/table.go, line 320 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

if readPos > 0 {
return errors.New(...)
}

Done.