Decompression block pool is inefficient

[damz]

What version of Go are you using (go version)?

$ go version
go version go1.14.4 linux/amd64

What operating system are you using?

Linux

What version of Badger are you using?

master

Steps to Reproduce the issue

Profile a badger using database, you will likely see a number of allocations both in table.decompress and in zstd.Decompress, something like this:

What's going on?

There are actually three issues:

The block size is hardcoded

The block pool is expecting blocks to be 4kB, regardless of the Options.BlockSize setting of the database (it actually allocates 5kB instead of 4kB as an effort to avoid spurious allocations during decompression);

The zstd library expects a slice of the correct length, not just capacity

The table.decompress passes zstd a block that has a capacity of 5kB, but a length that can be anything. On the other hand zstd expects the block length to be enough (see that it passes the length to the C function here:

https://github.com/DataDog/zstd/blob/89f69fb7df32e0513d85ac76df1c2a13df58e5e7/zstd.go#L106-L110

The block pool keeps a reference to the block structure alive

In addition (but it is a minor issue), the way the block pool is implemented makes the block pool keep a reference to the block struct alive:

https://github.com/dgraph-io/badger/blob/d37ce36911ae349fbedba342f693184aa434cc30/table/table.go#L241-L243

The &b.data is a reference to the block, so the whole struct is kept alive, including everything that it itself references.

[jarifibrahim]

The block size is hardcoded
The block pool is expecting blocks to be 4kB, regardless of the Options.BlockSize setting of the database (it actually allocates 5kB instead of 4kB as an effort to avoid spurious allocations during decompression);

Yes, this is correct. The block size is hardcoded but the assumption is that the pool will be filled up with correct sized blocks eventually. It starts with 4KB but if you've set the block size to 10KB, the blocks picked up from the pool will be not used and ZSTD library will create new 10KB sized blocks which we will insert in the pool. So eventually, the pool will have 10 KB blocks.

The zstd library expects a slice of the correct length, not just capacity
The table.decompress passes zstd a block that has a capacity of 5kB, but a length that can be anything. On the other hand zstd expects the block length to be enough.

We're creating blocks of 5 KB length. The ZSTD library will use these
https://github.com/dgraph-io/badger/blob/3f4761d229515a0208d847b6635011ed5187f75f/table/builder.go#L121-L130
Also, see https://play.golang.org/p/VLpoKIsmYUZ
Am I missing something?

The block pool keeps a reference to the block structure alive
In addition (but it is a minor issue), the way the block pool is implemented makes the block pool keep a reference to the block struct alive:

Interesting. I thought this would keep only the reference to the data []byte slice. If this keeps the reference to the struct alive, how can we insert just the data in the pool? I intentionally used the pointer to void the slice header copies.

[damz]

Interesting. I thought this would keep only the reference to the data []byte slice. If this keeps the reference to the struct alive, how can we insert just the data in the pool? I intentionally used the pointer to void the slice header copies.

I don't think there is a good way, other than carrying over the pointer everywhere.

With a little bit of unsafe, if we assume that blocks are all the same size, you could only store a pointer to the first element, and rebuild the slice header when getting the slice. But it is not very clean either.

[minhaj-shakeel]

Github issues have been deprecated.
This issue has been moved to discuss. You can follow the conversation there and also subscribe to updates by changing your notification preferences.