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.

Sign up for GitHub

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

Jump to bottom

Refactor zstd Decoder #477

Closed
klauspost opened this issue Jan 25, 2022 · 2 comments · Fixed by #498
Closed

Refactor zstd Decoder #477

klauspost opened this issue Jan 25, 2022 · 2 comments · Fixed by #498
Labels
enhancement performance

Comments

@klauspost
Copy link
Owner

klauspost commented Jan 25, 2022 •
edited
Loading

Currently working on refactoring the zstandard decoder. Goals of the refactor:
This was referenced Jan 25, 2022
Closed
Closed
@dsnet dsnet mentioned this issue Jan 28, 2022
Closed
@MarkusFreitag
Copy link

MarkusFreitag commented Feb 3, 2022 •
edited
Loading

Currently the *zstd.Decoder has a Close method without a return value and so does not fulfill the io.ReadCloser interface. Is this something that can be changed during the refactoring?

I'm not sure whether this belongs to this issue, if not I'm happy to create a new one

@klauspost
Copy link
Owner Author

@MarkusFreitag No. That would be a breaking change. Use IOReadCloser.

It shouldn't be used like a standard ReadCloser, so I on purpose does not support the interface.

@klauspost klauspost mentioned this issue Feb 18, 2022
Merged
klauspost added a commit that referenced this issue Feb 24, 2022
@klauspost
Refactor zstd decoder (#498)
308a751 
# TLDR;

* Streams can now be decoded without goroutines using `WithDecoderConcurrency(1)`.

* `WithDecoderConcurrency(4)` is now default. If you need more concurrent `DecodeAll` operations, use `WithDecoderConcurrency(0)`.

Goroutines exit when streams have finished reading (either error or EOF).

Designed and tested to be compatible, but test before committing upgrade.

# Changes

Goroutines will now only be created on demand, and `WithDecoderConcurrency(1)` is now strictly synchronized.

Decompression will typically be about 2x faster when using multiple goroutines, and will prepare input for the upstream reader async to reads. This can lead to ~3x faster input in total than using no goroutines.

New default is now `WithDecoderConcurrency(4)` (or less, if GOMAXPROCS is less). Beyond 4, there is little benefit for streaming decompression.

* No goroutines created, unless streaming, and auto-closed at error/EOF.
* Synchronous stream decoding with  `WithDecoderConcurrency(1)`.
* Split sequence decoding/execution for streams up to 50% faster.
* Simplified error flow.
* Speedup on streams.
* More consistent error reporting.
* Improved error detection/compliance with reference decoder.
* Improved test coverage.

Fixes #477
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Refactor zstd decoder klauspost/compress
2 participants
@klauspost @MarkusFreitag