Change zstd compression algorithm

[jarifibrahim]

NOTE: The following benchmarks are invalid. The default compression level in both the implementations is different and that leads to the performance difference. On same compression levels, both the implementations perform similarly in terms of compression speed but there's a difference in decompression speed.

Currently, we use https://github.com/DataDog/zstd implementation for ZSTD compression and https://github.com/valyala/gozstd seems to be faster than https://github.com/DataDog/zstd . This PR
proposes use of valyala/gozstd instead of DataDog/zstd.

benchstat zstd.txt gozstd.txt 
name                       old time/op  new time/op  delta
Compression/compress-16     213ms ± 6%   116ms ± 3%  -45.79%  (p=0.000 n=9+10)
Compression/decompress-16  48.5ms ± 3%  45.0ms ± 1%   -7.30%  (p=0.000 n=10+10)

The following script was used for benchmarking

var a []byte
func BenchmarkCompression(b *testing.B) {
	opts := Options{BlockSize: 4 * 1024, BloomFalsePositive: 0.01}
	count := uint64(1e6)
	builder := NewTableBuilder(opts)
	// Approx 64 MB table size
	for i := uint64(0); i < count*2; i++ {
		k := fmt.Sprintf("%016x", i)
		v := fmt.Sprintf("%016d", i)
		builder.Add([]byte(k), y.ValueStruct{Value: []byte(v)})
	}
	// Build a dummy table so that we can use the same buffer for benchmarks
	buf := builder.Finish()

	var x []byte
	var err error
	// Used for decompression benchmarks
	// cbuf, err := zstd.Compress(nil, buf) 	// Use this buffer for zstd.
	// require.NoError(b, err)
	cbuf1 := gozstd.Compress(nil, buf)

	fmt.Println("len of buf", humanize.Bytes(uint64(len(buf))))
	b.ResetTimer()
	b.Run("compress", func(b *testing.B) {
		for i := 0; i < b.N; i++ {
			x = gozstd.Compress(nil, buf) // change this for zstd.
		}
		a = x
	})
	b.Run("decompress", func(b *testing.B) {
		for i := 0; i < b.N; i++ {
			x, err = gozstd.Decompress(nil, cbuf1) // change this for zstd.
			if err != nil {
				panic(err)
			}
		}
		a = x
	})
}

The following is the benchmark of time taken to build a table in badger (https://github.com/dgraph-io/badger/blob/f50343ff404d8198df6dc83755ec2eab863d5ff2/table/builder_test.go#L116)

benchstat master-builder.txt new-builder.txt                       
name        old time/op  new time/op  delta
Builder-16   1.02s ± 1%   0.73s ± 2%  -27.77%  (p=0.000 n=9+10)

---

This change is 

[pullrequest]

✅ A review job has been created and sent to the PullRequest network.

---

@jarifibrahim you can click here to see the review status or cancel the code review job.

[coveralls]

Coverage decreased (-0.02%) to 77.53% when pulling d6d06ca on ibrahim/gozstd into f50343f on master.

[ashish-goswami]

Benchmarks are surely better for gozstd than zstd. But we should also consider stability and support for both. Just try to find who are users of both libraries(if they are public) and using those in production.

[pullrequest]

Please note that the goztsd can panic, and does not return a result.

---

Reviewed with ❤️ by PullRequest

[pullrequest]

although gozstd does not return an error, it will panic instead. If you want to keep a consistent behavior, and not kill the server when something goes wrong, catch the panic in a recover.

[pullrequest]

I would say stick with consistency, and keep the current behavior.
Furthermore, I think it's better to keep issues isolated, and not have to depend on the error handling/panic handling behavior of the caller.

[pullrequest]

Just wondering why this option wasn't required before.

[pullrequest]

LGTM, great job including the benchmarks in the comments.

---

Reviewed with ❤️ by PullRequest

[jarifibrahim]

@ashish-goswami gozstd is used by https://github.com/VictoriaMetrics/VictoriaMetrics

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @ashish-goswami, @manishrjain, and @pullrequest[bot])

---

table/builder.go, line 350 at r1 (raw file):

Previously, pullrequest[bot] wrote…

although gozstd does not return an error, it will panic instead. If you want to keep a consistent behavior, and not kill the server when something goes wrong, catch the panic in a recover.

I could try to recover the panic here but the finishBlock function that calls this function will crash the process if compress returns an error. So the question being, do we want to crash the process by a panic or by returning an error.

https://github.com/dgraph-io/badger/blob/f50343ff404d8198df6dc83755ec2eab863d5ff2/table/builder.go#L162-L164

I'd prefer to keep it this way.

---

table/builder_test.go, line 128 at r1 (raw file):

Previously, pullrequest[bot] wrote…

Just wondering why this option wasn't required before.

This option isn't really needed. I added it because I realized it was missing from the benchmark.

[ashish-goswami]

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @ashish-goswami, @manishrjain, and @pullrequest[bot])

[pullrequest]

I still think panic should be recovered, and keep consistent behaviors, but it isn't a major blocker for me.

---

Reviewed with ❤️ by PullRequest

[jarifibrahim]

Benchmarks show LZ4 is faster than zstd and so we don't need this PR anymore.

go test -v -run ^$ -bench BenchmarkTableCompression
data size 4056
goos: linux
goarch: amd64
pkg: github.com/dgraph-io/badger/table
BenchmarkTableCompression/Compression/Snappy-16 	   	   	    	  148005	      8103 ns/op
BenchmarkTableCompression/Compression/LZ4-16      	   	   	   	  	  192206	      5589 ns/op
BenchmarkTableCompression/Compression/ZSTD_-_Datadog-16         	   24697	     47433 ns/op
BenchmarkTableCompression/Decompression/Snappy-16               	  308114	      3952 ns/op
BenchmarkTableCompression/Decompression/LZ4-16                  	  379030	      2728 ns/op
BenchmarkTableCompression/Decompression/ZSTD_-_Datadog-16       	  100808	     11431 ns/op
PASS
ok  	github.com/dgraph-io/badger/table	8.708s

https://gist.github.com/jarifibrahim/50b596c233aa9d82706aa0df65a546ec used for benchmarking.

[arl]

I know this is closed but there's another reason to switch zstd library, a serious memory corruption bug in DataDog/zstd, that can either lead to segfaults, lower compression ratio, or nothing depending on your usage of the library.

Please see DataDog/zstd#22