Buffer pool for decompression

[jarifibrahim]

This PR is an attempt to reduce the memory usage of decompression. This PR uses a sync pool to hold decompression buffers. A buffer is added to the pool only if was used for decompression. We don't want to put buffers that were not used for decompression because these buffers are read from mmaped SST files and any changes to these buffers would lead to a segfault.

Fixes #1239

---

This change is 

[manishrjain]

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

---

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

			decompressPool.Put(&itr.bi.data)
		}
		itr.bi.data = nil

Set nil before putting it into the pool.

---

table/iterator.go, line 351 at r3 (raw file):

			decompressPool.Put(&itr.bi.data)
		}
		itr.bi.data = nil

same here.

---

table/table.go, line 645 at r3 (raw file):

func (t *Table) decompressData(data []byte) ([]byte, error) {
	fmt.Println("Decompress111")
	dst := decompressPool.Get().(*[]byte)

Why not use y.Slice?

[jarifibrahim]

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

---

table/iterator.go, line 109 at r2 (raw file):

Previously, gja (Tejas Dinkar) wrote…

this feels a bit weird that it's done in this way. Can't we keep a single buffer pool for both decompressed and regular data?

We cannot do that since the uncompressed buffers actually point to the mmapped file.

---

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

Previously, manishrjain (Manish R Jain) wrote…

Set nil before putting it into the pool.

I've changed the implementation. This is no longer necessary.

---

table/iterator.go, line 351 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

same here.

I've changed the implementation. This is no longer necessary.

---

table/table.go, line 644 at r2 (raw file):

Previously, gja (Tejas Dinkar) wrote…

Another trick is to do

type BufferPool {
  // contains a sync.Pool internally
  Get() []bytes <- Clean out the buffer here
  Put([] bytes)
}

Done. Created a new type.

---

table/table.go, line 645 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Why not use y.Slice?

y.Slice contains a []byte slice and I don't see any benefit from using y.slice.

[jarifibrahim]

Reviewable status: 2 of 4 files reviewed, 6 unresolved discussions (waiting on @ashish-goswami, @gja, @golangcibot, and @manishrjain)

---

table/builder.go, line 121 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Probably don't need a special struct.

Done.

---

table/builder.go, line 125 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

I'd suggest using y.Slice.

Done.

---

table/iterator.go, line 109 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

S1002: should omit comparison to bool constant, can be simplified to itr.compressed (from gosimple)

Done.

---

table/table.go, line 644 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

The way Go typically uses sync.Pool is directly -- that's how we do it in codebase as well. So, let's keep it consistent and use sync.Pool directly.

Done

---

table/table.go, line 174 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

if b, ok := ... ; ok { ... }

Done.

[jarifibrahim]

Dismissed @manishrjain, and @manishrjain from 6 discussions.
Reviewable status: 2 of 4 files reviewed, all discussions resolved (waiting on @ashish-goswami and @manishrjain)

All comments addressed

[jarifibrahim]

@ashish-goswami tested this with Dgraph there is a reduction in the amount of memory allocated by the decompression.

[ashish-goswami]

Yes, I saw reduction in the allocated memory size after this change. However there was no change in the inuse memory size(even if you leave cluster idle for sometime). First it was shown as part of zstd.Decompression (without this change), with this change it was shown as syncPool.
Old profile:

Entering interactive mode (type "help" for commands, "o" for options)
(pprof) top
Showing nodes accounting for 2725.22MB, 95.26% of 2860.70MB total
Dropped 204 nodes (cum <= 14.30MB)
Showing top 10 nodes out of 113
      flat  flat%   sum%        cum   cum%
  903.07MB 31.57% 31.57%   903.07MB 31.57%  github.com/DataDog/zstd.Decompress
     612MB 21.39% 52.96%      612MB 21.39%  github.com/dgraph-io/badger/v2.(*oracle).newCommitTs
     276MB  9.65% 62.61%      276MB  9.65%  github.com/dgraph-io/badger/v2/table.NewTableBuilder
     256MB  8.95% 71.56%      256MB  8.95%  github.com/dgraph-io/ristretto.newCmRow
  169.53MB  5.93% 77.48%   200.03MB  6.99%  github.com/dgraph-io/dgraph/protos/pb.(*Mutations).Unmarshal
  166.41MB  5.82% 83.30%   166.41MB  5.82%  github.com/dgraph-io/badger/v2/skl.newArena
  165.50MB  5.79% 89.09%   165.50MB  5.79%  github.com/dgraph-io/ristretto/z.(*Bloom).Size
  105.06MB  3.67% 92.76%   144.15MB  5.04%  github.com/dgraph-io/badger/v2/table.OpenTable
   41.15MB  1.44% 94.20%    41.15MB  1.44%  github.com/dgraph-io/ristretto.(*lockedMap).Set
   30.50MB  1.07% 95.26%    30.50MB  1.07%  github.com/dgraph-io/dgraph/protos/pb.(*DirectedEdge).Unmarshal

New profile(with this change):

(pprof) top
Showing nodes accounting for 2618.54MB, 95.96% of 2728.65MB total
Dropped 101 nodes (cum <= 13.64MB)
Showing top 10 nodes out of 92
      flat  flat%   sum%        cum   cum%
  783.55MB 28.72% 28.72%   783.55MB 28.72%  github.com/dgraph-io/badger/v2/table.newSlicePool.func1
  613.53MB 22.48% 51.20%   613.53MB 22.48%  github.com/dgraph-io/badger/v2.(*oracle).newCommitTs
     345MB 12.64% 63.84%      345MB 12.64%  github.com/dgraph-io/badger/v2/table.NewTableBuilder
     256MB  9.38% 73.23%      256MB  9.38%  github.com/dgraph-io/ristretto.newCmRow
  200.41MB  7.34% 80.57%   200.41MB  7.34%  github.com/dgraph-io/ristretto/z.(*Bloom).Size
  166.41MB  6.10% 86.67%   166.41MB  6.10%  github.com/dgraph-io/badger/v2/skl.newArena
  105.79MB  3.88% 90.55%   172.46MB  6.32%  github.com/dgraph-io/badger/v2/table.OpenTable
   65.02MB  2.38% 92.93%    76.74MB  2.81%  github.com/DataDog/zstd.Decompress
   46.27MB  1.70% 94.62%    69.77MB  2.56%  github.com/dgraph-io/badger/v2/pb.(*TableIndex).Unmarshal
   36.56MB  1.34% 95.96%    36.56MB  1.34%  github.com/dgraph-io/ristretto.(*lockedMap).Set