Mlayer/skiplist

[ashwin95r]

Initial PR to see skiplist performance in mlayer

Current:
$ go test --bench AddMut
BenchmarkAddMutations-4   	  200000	      7683 ns/op

After change (with skiplist):
$ go test --bench AddMut
BenchmarkAddMutations-4   	  500000	      5082 ns/op

Using skiplist is ~33% faster. I'll try to benchmark this for more scenarios and update here.

---

This change is 

[manishrjain]

Review status: 0 of 3 files reviewed at latest revision, 7 unresolved discussions, some commit checks broke.

---

posting/list.go, line 35 at r1 (raw file):

ryszard/goskiplist

Badger has a skiplist. Can't you use that? We've already optimized it.

---

posting/list.go, line 73 at r1 (raw file):

	ghash         uint64
	plist         *protos.PostingList
	mlayer        *skiplist.SkipList //[]*protos.Posting // mutations

If you're modifying things, you don't need to keep the old code

---

posting/list.go, line 74 at r1 (raw file):

	plist         *protos.PostingList
	mlayer        *skiplist.SkipList //[]*protos.Posting // mutations
	len           int

len is a reserved keyword for an internal function.

---

posting/list.go, line 368 at r1 (raw file):

			if mpost.Op == Del {
				// Undo old post.
				l.len--

Is it expensive to get length from the skiplist? If so, we can modify the skiplist (ours) to keep track of length.

---

posting/list.go, line 376 at r1 (raw file):

		} else if oldPost.Op == Del {
			if mpost.Op == Set {
				l.len++

What? Why?

---

posting/list.go, line 409 at r1 (raw file):

	} else {
		if psame { // mpost.Op==Del
			l.len--

I'm not liking this length tracking at all. Let's switch the skiplist impl, and get rid of this len stuff.

---

posting/list.go, line 628 at r1 (raw file):

}

func (l *List) lengthEst() int {

Why is this an estimate?

---

Comments from Reviewable

[manishrjain]

This would also impact the memory usage significantly. Can you benchmark memory allocations as well?

---

Review status: 0 of 3 files reviewed at latest revision, 7 unresolved discussions, some commit checks broke.

---

Comments from Reviewable

[ashwin95r]

With skiplist:
$ go test --benchmem --bench AddMut 
BenchmarkAddMutations-4   	  200000	      5125 ns/op	     765 B/op	       6 allocs/op

With slice:
$ go test --benchmem --bench AddMut 
BenchmarkAddMutations-4   	  300000	     20577 ns/op	     378 B/op	       3 allocs/op

Memory usage is ~2x

---

Review status: 0 of 3 files reviewed at latest revision, 8 unresolved discussions.

---

posting/list.go, line 35 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

ryszard/goskiplist

Badger has a skiplist. Can't you use that? We've already optimized it.

I had a look but that library was too specific to Badger, so I used this library for a quick check as the API was simple.

---

posting/list.go, line 73 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

If you're modifying things, you don't need to keep the old code

Done.

---

posting/list.go, line 368 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Is it expensive to get length from the skiplist? If so, we can modify the skiplist (ours) to keep track of length.

It's not the length of the skiplist but the length of the posting list (the mlayer has add and del ops which affect the lenght). We iterate through the mlayer to see how many add/del posting which is unnecessary. We can track it when an update happens which is this len.

---

posting/list.go, line 376 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

What? Why?

As mentioned, this is to track the total length based on the ops.

---

posting/list.go, line 409 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

I'm not liking this length tracking at all. Let's switch the skiplist impl, and get rid of this len stuff.

The skiplist already has a Len() but we need to know the contents (hence iterate) to calculate our length. So, this can't be passed on to the skiplist lib.

---

posting/list.go, line 628 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Why is this an estimate?

Since we don't look at the actual ops in mlayer, this is approximate. At some places, this approximate length is good enough.

---

Comments from Reviewable

[ashwin95r]

Review status: 0 of 3 files reviewed at latest revision, 7 unresolved discussions, some commit checks broke.

---

posting/list.go, line 35 at r1 (raw file):

Previously, ashwin95r (Ashwin Ramesh) wrote…

I had a look but that library was too specific to Badger, so I used this library for a quick check as the API was simple.

Also badger skl doesn't have Delete() Api

---

Comments from Reviewable

[ashwin95r]

Review status: 0 of 3 files reviewed at latest revision, 7 unresolved discussions, some commit checks broke.

---

posting/list.go, line 74 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

len is a reserved keyword for an internal function.

Done.

---

Comments from Reviewable

[ashwin95r]

Since we're going to shard the PLs and flush them if the size of the mutation layer reaches a threshold, skiplists won't be of much benefit (as it only outperforms insertions on larger lists). So closing this PR.