-
Notifications
You must be signed in to change notification settings - Fork 127
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
Rewrite gzip component to support trailing bytes without external tool and compress with PIGZ. Addresses #476 #485
Rewrite gzip component to support trailing bytes without external tool and compress with PIGZ. Addresses #476 #485
Conversation
…l (using zlib library instead of gzip)
…stead of temp file
Co-authored-by: Jacob Strieb <99368685+rbs-jacob@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a changelog entry, but after that, should probably be good to go.
Last change was still in unreleased section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments.
Based on benchmarks of zlib module vs pigz, no benefit to using pigz for decompression, but significant benefit to using it for compression. https://gist.github.com/alchzh/24ca62a6f87548ea2b1911c174e0ed54 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Especially like the profiling and data-driven approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not review the tests in great detail, but I like the way that this implementation ended up. Seems like a big improvement!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I relented and included one minor change that I would like to see before merging this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this fix. Looks ready to merge once the tests pass!
Rewrite gzip component to support trailing bytes without external tool.
Link to Related Issue(s)
#476
python/cpython#68489
Please describe the changes in your request.
gzip
library with calls to the underlyingzlib
library directly, in a way that doesn't fail with trailing garbage at the end of files.