Skip to content

Update zstd to 1.1.3 for 1.x branch#11

Closed
Viq111 wants to merge 2 commits into1.xfrom
viq111/1.1.3
Closed

Update zstd to 1.1.3 for 1.x branch#11
Viq111 wants to merge 2 commits into1.xfrom
viq111/1.1.3

Conversation

@Viq111
Copy link
Collaborator

@Viq111 Viq111 commented Feb 22, 2017

Straightforward C files update is available at this commit: 45f7613

Unfortunately that triggers some warnings on ZBUFF deprecation:

cgo-gcc-prolog:39:25: warning: 'ZBUFF_createDCtx' is deprecated: use ZSTD_createDStream [-Wdeprecated-declarations]
./zbuff.h:120:56: note: 'ZBUFF_createDCtx' has been explicitly marked deprecated here
cgo-gcc-prolog:60:6: warning: 'ZBUFF_decompressContinue' is deprecated: use ZSTD_decompressStream [-Wdeprecated-declarations]
./zbuff.h:126:54: note: 'ZBUFF_decompressContinue' has been explicitly marked deprecated here
cgo-gcc-prolog:77:6: warning: 'ZBUFF_decompressInit' is deprecated: use ZSTD_initDStream [-Wdeprecated-declarations]
./zbuff.h:123:59: note: 'ZBUFF_decompressInit' has been explicitly marked deprecated here
cgo-gcc-prolog:96:6: warning: 'ZBUFF_decompressInitDictionary' is deprecated: use ZSTD_initDStream_usingDict [-Wdeprecated-declarations]
./zbuff.h:124:59: note: 'ZBUFF_decompressInitDictionary' has been explicitly marked deprecated here
cgo-gcc-prolog:113:6: warning: 'ZBUFF_freeDCtx' is deprecated: use ZSTD_freeDStream [-Wdeprecated-declarations]
./zbuff.h:121:56: note: 'ZBUFF_freeDCtx' has been explicitly marked deprecated here
cgo-gcc-prolog:129:6: warning: 'ZBUFF_recommendedDInSize' is deprecated: use ZSTD_DStreamInSize [-Wdeprecated-declarations]
./zbuff.h:166:52: note: 'ZBUFF_recommendedDInSize' has been explicitly marked deprecated here
cgo-gcc-prolog:145:6: warning: 'ZBUFF_recommendedDOutSize' is deprecated: use ZSTD_DStreamOutSize [-Wdeprecated-declarations]
./zbuff.h:167:52: note: 'ZBUFF_recommendedDOutSize' has been explicitly marked deprecated here

I updated the bindings to the new interface here: 8ec87b6

Unfortunately (😞), this makes us use go pointers inside a C struct (ZSTD_inBuffer & ZSTD_outBuffer) so this needs compilation with GODEBUG=cgocheck=0
We will have to do memory management ourselves if we want to update to the new bindings.

panic: runtime error: cgo argument has Go pointer to Go pointer

However if you want to give it a go and need zstd 1.1.3, 45f7613 should be pretty stable (no bindings changes, it just add deprecation warnings)

Benchmarks (with mr):

 go version
go version go1.8 darwin/amd64

 git checkout 1.x
~/datadog/zstd (1.x)  go test -run no_test -bench .
BenchmarkStreamCompression-4     	    1000	 119786193 ns/op	  83.24 MB/s
BenchmarkStreamDecompression-4   	    3000	  34123937 ns/op	 292.19 MB/s
BenchmarkCompression-4           	    1000	 121966846 ns/op	  81.75 MB/s
BenchmarkDecompression-4         	    3000	  25224592 ns/op	 395.27 MB/s
	zstd_test.go:156: Reduced from 9970564 to 3444121

 git checkout 45f7613
~/datadog/zstd (45f7613)  go test -run no_test -bench .
BenchmarkStreamCompression-4     	    1000	 118444230 ns/op	  84.18 MB/s
BenchmarkStreamDecompression-4   	    3000	  36405709 ns/op	 273.87 MB/s
BenchmarkCompression-4           	    1000	 117427971 ns/op	  84.91 MB/s
BenchmarkDecompression-4         	    5000	  23340524 ns/op	 427.18 MB/s
	zstd_test.go:156: Reduced from 9970564 to 3444121

 git checkout 8ec87b6
~/datadog/zstd (8ec87b6)  GODEBUG=cgocheck=0 go test -run no_test -bench .
BenchmarkStreamCompression-4     	      10	 131603981 ns/op	  75.76 MB/s
BenchmarkStreamDecompression-4   	      50	  33397258 ns/op	 298.54 MB/s
BenchmarkCompression-4           	      10	 116480682 ns/op	  85.60 MB/s
BenchmarkDecompression-4         	      50	  22810589 ns/op	 437.10 MB/s
	zstd_test.go:156: Reduced from 9970564 to 3444121

This takes the c code from commit
cbc5225d38ba65a1cd77701e91a9f45f3cc825ba available at
facebook/zstd@cbc5225
3cc825ba

The error list has been updated to reflect the C code
Unfortunately, since Go 1.6, you can't have a C object with a go
pointer inside it, so we have to either use `GODEBUG=cgocheck=0` or do
memory management ourselves.

I guess we will want to do mem management ourselves because we can't
ask users to compile their program with `cgocheck=0`
@Viq111
Copy link
Collaborator Author

Viq111 commented Feb 22, 2017

Closing in favor of #12.
We can cherry-pick 8ec87b6 when we will want to use the new streaming interface

@Viq111 Viq111 closed this Feb 22, 2017
@Viq111 Viq111 deleted the viq111/1.1.3 branch March 13, 2018 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant