Libdeflate port #25
Libdeflate port #25
Conversation
| if (compressor_) | ||
| { | ||
| libdeflate_free_compressor(compressor_); | ||
| } |
There was a problem hiding this comment.
Noting that this approach (initialize C struct pointer in constructor + free the memory in the deconstructor) is applying RAII (https://en.wikipedia.org/wiki/Resource_acquisition_is_initialization) to avoid a memory leak.
| #pragma GCC diagnostic ignored "-Wold-style-cast" | ||
| if (deflateInit2(&deflate_s, level_, Z_DEFLATED, window_bits, mem_level, Z_DEFAULT_STRATEGY) != Z_OK) | ||
| std::size_t max_compressed_size = libdeflate_gzip_compress_bound(compressor_, size); | ||
| // TODO: sanity check this before allocating |
There was a problem hiding this comment.
The reason for this comment is fear/lack of knowledge on my part. What happens if libdeflate_gzip_compress_bound is buggy and returns a really massive value? Is that possible? Would we end up trying to allocate so much memory the machine would crumble? Probably not possible, but I've also not looked inside libdeflate yet to figure out how much to worry about this.
| @@ -56,7 +56,7 @@ class Decompressor | |||
| // https://github.com/kaorimatz/libdeflate-ruby/blob/0e33da96cdaad3162f03ec924b25b2f4f2847538/ext/libdeflate/libdeflate_ext.c#L340 | |||
| // https://github.com/ebiggers/libdeflate/commit/5a9d25a8922e2d74618fba96e56db4fe145510f4 | |||
| std::size_t actual_size; | |||
| std::size_t uncompressed_size_guess = size * 3; | |||
| std::size_t uncompressed_size_guess = size * 4; | |||
| output.resize(uncompressed_size_guess); | |||
There was a problem hiding this comment.
This probably needs to be re-written to avoid needing to guess the size up front. @artemp could you tackle that?
There was a problem hiding this comment.
std::size_t uncompressed_size_guess = size * 4; - good starting point but it definitely needs a way to dynamically increase capacity if size * 4 is not large enough. Approach from the ruby link looks reasonable, here is my patch :
diff --git a/include/gzip/decompress.hpp b/include/gzip/decompress.hpp
index badd315..627e670 100644
--- a/include/gzip/decompress.hpp
+++ b/include/gzip/decompress.hpp
@@ -57,17 +57,23 @@ class Decompressor
// https://github.com/ebiggers/libdeflate/commit/5a9d25a8922e2d74618fba96e56db4fe145510f4
std::size_t actual_size;
std::size_t uncompressed_size_guess = size * 4;
- output.resize(uncompressed_size_guess);
- enum libdeflate_result result = libdeflate_gzip_decompress(decompressor_,
- data,
- size,
- static_cast<void*>(&output[0]),
- uncompressed_size_guess, &actual_size);
- if (result == LIBDEFLATE_INSUFFICIENT_SPACE)
+ output.reserve(uncompressed_size_guess);
+ enum libdeflate_result result;
+ for (;;)
{
- throw std::runtime_error("no space: did not succeed");
+ result = libdeflate_gzip_decompress(decompressor_,
+ data,
+ size,
+ const_cast<char*>(output.data()),
+ output.capacity(), &actual_size);
+ if (result != LIBDEFLATE_INSUFFICIENT_SPACE)
+ {
+ break;
+ }
+ output.reserve((output.capacity() << 1) - output.size());
}
- else if (result == LIBDEFLATE_SHORT_OUTPUT)
+
+ if (result == LIBDEFLATE_SHORT_OUTPUT)
{
throw std::runtime_error("short output: did not succeed");
}
Couple notes:
- Use
std::vector::reserve()to increase capacity - Use
std::vector::data()to access a pointer to the first element (c++11) which works for empty containers, while&vec[0]is not safe and needs an extra check.
/cc @springmeyer
There was a problem hiding this comment.
@artemp great fix - can you please commit directly to this branch? But two concerns to keep in mind:
- Can you think of a situation where
resultwill never become == toLIBDEFLATE_INSUFFICIENT_SPACEand therefore the loop will run infinitely? Can we protect against that by checking for other error states inside the loop? - I wonder if we should avoid calling
output.reservebecause sometimes we may overeserve and the extra cost of over-reserving might overrule the benefit of reserving when it is relatively correct. This concern of mine comes from seeing howreserveis expensive and when wrong hurts perf over at Optimize coalesce carmen-cache#126
There was a problem hiding this comment.
Can you think of a situation where result will never become == to LIBDEFLATE_INSUFFICIENT_SPACE and therefore the loop will run infinitely? Can we protect against that by checking for other error states inside the loop?
Good point, I'll add conditional exception in the loop if allocation size is getting to big.
I wonder if we should avoid calling output.reserve because sometimes we may overeserve and the extra cost of over-reserving might overrule the benefit of reserving when it is relatively correct. This concern of mine comes from seeing how reserve is expensive and when wrong hurts perf over at mapbox/carmen-cache#126
Let me investigate and fix this ^
I'm going to push updates and continue work on this branch
There was a problem hiding this comment.
I'm going to push updates and continue work on this branch
👍 I think you'll likely need an easy way to test larger, varied files and a CLI would allow that. So could you finish #29 to aid your testing of this branch?
There was a problem hiding this comment.
Looking good overall and faster according to make bench but see my comments with proposed changes /cc @springmeyer
…icient + use `data()`
…size logic to output buffer + throw an exception if request to grow output buffer exceeds max_size (default 2GB).
| @@ -130,7 +130,7 @@ TEST_CASE("test decompression size limit") | |||
| std::istreambuf_iterator<char>()); | |||
| stream.close(); | |||
|
|
|||
| std::size_t limit = 20 * 1024 * 1024; // 20 Mb | |||
| std::size_t limit = 500 * 1024 * 1024; // 500 Mb | |||
There was a problem hiding this comment.
@springmeyer - I've changed logic to validate output buffer size rather then input, which makes more sense in my opinion.
| struct libdeflate_decompressor* decompressor_ = nullptr; | ||
|
|
||
| public: | ||
| Decompressor(std::size_t max_bytes = 1000000000) // by default refuse operation if compressed data is > 1GB | ||
| Decompressor(std::size_t max_bytes = 2147483648u) // by default refuse operation if required uutput buffer is > 2GB |
There was a problem hiding this comment.
@artemp typo: uutput
| // https://github.com/kaorimatz/libdeflate-ruby/blob/0e33da96cdaad3162f03ec924b25b2f4f2847538/ext/libdeflate/libdeflate_ext.c#L340 | ||
| // https://github.com/ebiggers/libdeflate/commit/5a9d25a8922e2d74618fba96e56db4fe145510f4 | ||
| std::size_t actual_size; | ||
| std::size_t uncompressed_size_guess = size * 4; | ||
| output.reserve(uncompressed_size_guess); | ||
| std::size_t uncompressed_size_guess = std::min(size * 4, max_); |
There was a problem hiding this comment.
What happens when size * 4 will not fit within std::numeric_limits<std::size_t>::max()?
|
@artemp all the changes look good. I made a few minor comments inline. One other high level thought: check out #27 which may be of interest as you find bugs or find ways to improve the code in general (non-specific to this libdeflate branch). If you do have generally applicable changes I think it would be ideal to keep track of these and attempt to land them in a separate PR. |
|
@springmeyer - thanks for the feedback, I'll reply asap. Going through #27 looks like a good plan. |
|
@artemp I noticed that the at https://travis-ci.org/mapbox/vtcomposite/jobs/438830669#L986. @artemp could you fix this warning/error? Perhaps it could be fixed by making the class noncopyable? You should be able to replicate locally with g++ and adding |
|
@artemp one more need that I noticed: we have a downstream consumer
|
Yep, just deleting copy constructor and copy assignment operator would do. Done in b4684ca |
@springmeyer - I've no idea why this module called gzip-hpp. I'm not buying 'for historical reasons', let's make it right, now, and avoid perpetual confusions! Once we're on the same page I'll add support for handling zlib (de)compression. In terms of APIs, I see two high level approaches. The first one is the correct one and the second is a 'lets-make-life-easier-by-hiding-implementation-switch-inside-a-class`. :)
// This is not a working code just an illustration
if (gzip)
{
deflate::gzip_decompressor obj{};
// or
deflate::decompressor<GZIP> obj{};
// do the work
}
else if (zlib)
{
deflate::zlib_decompressor obj{};
// or
deflate::decompressor<ZLIB> obj{};
// do the work
}This is how
deflate::compressor obj{};
// do the workWondering if name choices here can be better: Compressor::compress(...);
Decompressor::decompress(...);? |
This ports gzip-hpp to use libdeflate. This is for
experimental purposes onlytesting in production and to investigate the potential speed improvements in using libdeflate.If this proves viable (fast and robust), this is exciting because:
Anyway, here are the first, promising, numbers locally:
Locally I see (with this PR) (smaller numbers is faster):
And with master:
TODO:
std::size_t uncompressed_size_guess = size * 3;to figure out the uncompressed size to allocate.Z_DEFAULT_COMPRESSIONproducing roughly the same sizes?