Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor stream reading #3

Closed
wants to merge 73 commits into from
Closed

Conversation

j-vizcaino
Copy link

This PR refactors how data is read and uncompressed.

It adds a doubleBuffer helper struct to do the magic C, double buffer dance and makes use of this in the Read function.

Here are the benchmarks comparing the current master with this branch:

master

$ go test -bench='^BenchmarkStreamUncompress.*' -benchtime 15s -count 2
goos: darwin
goarch: amd64
pkg: github.com/DataDog/golz4
BenchmarkStreamUncompress-4   	 5527802	      4236 ns/op	7735.24 MB/s	     123 B/op	       2 allocs/op
BenchmarkStreamUncompress-4   	 3750264	      6997 ns/op	4682.91 MB/s	     142 B/op	       2 allocs/op
PASS
ok  	github.com/DataDog/golz4	65.629s

New version

$ go test -bench='^BenchmarkStreamUncompress.*' -benchtime 15s -count 2
goos: darwin
goarch: amd64
pkg: github.com/DataDog/golz4
BenchmarkStreamUncompress-4   	 5643277	      4133 ns/op	7927.65 MB/s	     126 B/op	       3 allocs/op
BenchmarkStreamUncompress-4   	 3755970	      6837 ns/op	4792.58 MB/s	     140 B/op	       3 allocs/op
PASS
ok  	github.com/DataDog/golz4	65.232s

Performance is very similar, sometimes a bit better. There's still an extra alloc that slipped through the cracks, probably related to the addition of doubleBuffer.

FWIW, I also tested moving the temporary readBuffer as a struct member but this turned out to be worse, performance wise

jmoiron and others added 30 commits November 10, 2016 21:55
[golz4]add lz4 streaming API
Make the Reader work with io.Copy
add new printout to make sure using the right one
add more unit tests and checks in the Reader
zzzzssss and others added 27 commits November 11, 2019 16:20
[lz4]add comment for decomp double buffer
lz4 streaming compression support
Use external LibLZ4, detected using standard pkg-config (ME-768)
This is a noop
* `LZ4_compress_limitedOutput` calls `LZ4_compress_default`: https://github.com/lz4/lz4/blob/v1.9.2/lib/lz4.c#L2335-L2338
* `LZ4_compressHC2_limitedOutput` calls `LZ4_compress_HC`: https://github.com/lz4/lz4/blob/v1.9.2/lib/lz4hc.c#L1156
The input data needs to be at least 512 bytes long.
Use an internal buffer to store decompressed data between Read() calls
when dst buffer is not big enough.
* Use the doubleBuffer struct for handling read buffers
* Do not use C.GoBytes as this leads to extra copies, reuse the internal
buffers for pending reads
This ensures the overhead of creating the reader is mitigated by the
successive decompression calls, which should better map reality.
@j-vizcaino
Copy link
Author

Wrong target repo.

@j-vizcaino j-vizcaino closed this Jan 27, 2020
@j-vizcaino j-vizcaino deleted the refactor-read branch February 8, 2021 16:25
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.

5 participants