Added support for zstd compression #1247
Conversation
kiplingw
added
enhancement
fix in progress
dependencies
src/common/http.cc
Outdated
| // Allocate a smart buffer to contain compressed data... | ||
| std::unique_ptr compressedData = std::make_unique<std::byte[]>(estimated_size); | ||
|
|
||
| // Compress data using compresion_level = 5: https://facebook.github.io/zstd/zstd_manual.html#Chapter5 |
There was a problem hiding this comment.
We currently don't have a public API for the user to specify the compression level (each compressor can use a different range). Until we figure out an elegant way to do that, it might be best to set the compression level consistent with whatever we're using on the others (I think max).
There was a problem hiding this comment.
I believe max is really max on zstd. We could also set compression level dynamically, or partially dynamically, taking into account available CPU bandwidth, available network bandwidth, number of cores (zstd is good with multithreading) etc.
But maybe a middling value is wisest until we do a well-thought-through API/algorithm for compression level?
There was a problem hiding this comment.
@dgreatwood @kiplingw I agree. Using a middle value makes the most sense to me. At least until we find a better solution as both of you guys mentioned.
| @@ -77,7 +81,8 @@ foreach test_name : pistache_test_files | |||
| gmock_dep, | |||
| curl_dep, | |||
| cpp_httplib_dep, | |||
| brotli_dep | |||
| brotli_dep, | |||
| zstd_dep | |||
There was a problem hiding this comment.
Remember that libzstd-dev also needs to be added to Build-Depends in debian/control. That's in a separate branch and so you'll need a separate PR for that.
There was a problem hiding this comment.
Hi @EdgarModesto23 -
I see a number of the github workflows (https://github.com/pistacheio/pistache/actions) are failing, e.g. with:
tests/meson.build:71:1: ERROR: Unknown variable "zstd_dep".
Some of that may be related to the need for an update to debian/control. Certainly the way brotli is dealt with should be a good model.
The workflows linuxflibev, linux and macOS libevent should all pass (occasionally the debian:testing tests under linux workflow will fail if debian:testing is in a funny state, but in fact it seems OK at present). The conventional-commits and abidiff are less important (just make sure you bump version number if abidiff changes).
There was a problem hiding this comment.
Regarding http_server_test.cpp -
There are a lot of formatting changes that amount to indenting code that lies within encapsulating (scope limiting) curly braces, like:
{ // encapsulate
// Indent this line by 4 more spaces
// Etc.
}
The extra indentation that has been added is 100% logical, however it was not added before so that the encapsulating braces can be added or removed without causing a "diff" for each of the enclosed lines.
Would it be practical to revert the formatting changes from http_server_test.cpp, leaving only the substantive changes to be reviewed?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1247 +/- ##
==========================================
- Coverage 76.29% 76.26% -0.04%
==========================================
Files 58 58
Lines 10027 9698 -329
==========================================
- Hits 7650 7396 -254
+ Misses 2377 2302 -75 ☔ View full report in Codecov by Sentry. |
|
@Tachi107, your feedback is appreciated. |
There was a problem hiding this comment.
I've left some feedback in a few different comments, check it out!
Also, the 1.5.6 release of zstd seems to have specific improvements to usage in web contexts. You might want to see what changed and evaluate if tweaking your implementation is worthwhile.
src/common/http.cc
Outdated
| auto compress_size = ZSTD_compress((void*)compressedData.get(), estimated_size, | ||
| data, size, 11); |
There was a problem hiding this comment.
Please use reinterpret_cast instead of C-style casts. Also, what's this "11"? You've added a contentEncodingZstdLevel variable, why don't you use it?
Also, in any case, I believe that it should be initialized to the value returned by ZSTD_defaultCLevel() (or ZSTD_CLEVEL_DEFAULT). Alternatively, you might be able to specify 0, as by reading zstd's manual it seems that a value of 0 for the compression level means default - I'm not sure though!
tests/http_server_test.cc
Outdated
| @@ -1297,6 +1301,12 @@ struct ContentEncodingHandler : public Http::Handler | |||
| switch (encoding) | |||
| { | |||
|
|
|||
| #ifdef PISTACHE_USE_CONTENT_ENCODING_ZSTD | |||
| case Http::Header::Encoding::Zstd: | |||
| writer.setCompressionZstdLevel(ZSTD_btultra2); | |||
There was a problem hiding this comment.
Are you sure that ZSTD_btultra2 is doing what you expect here? That enum value is part of ZSTD_strategy, which as far as I can tell has a different meaning compared to the raw "compression level". You might want to use ZSTD_maxCLevel() instead.
There was a problem hiding this comment.
Same as the ZSTD_lazy2 issue above. I got a bit confused with the documentation. I changed it to be ZSTD_maxCLevel() as you suggested.
Following #1200
This patch adds support for zstd compression and it updates build scripts and the main README to include the new -DPISTACHE_USE_CONTENT_ENCODING_ZSTD flag. I mostly followed the style suggested by @kiplingw on #1177 and #1178.
It uses version 1.5.6 of the C implementation of zstd library.