GH-37169: [C++] Zstd compression uses context #48851
Conversation
|
|
HuaHuaY
changed the title
|
|
| : compression_level) {} | ||
|
|
||
| ~ZSTDCodec() override { | ||
| ZSTD_freeCCtx(compression_context_); |
There was a problem hiding this comment.
Zstd documentation states that these two free functions can accept nullptr. But if someone thinks that null pointer judgment is needed here, it's fine for me. Or we can wrap it with std::unique_ptr.
https://facebook.github.io/zstd/zstd_manual.html
size_t ZSTD_freeCCtx(ZSTD_CCtx* cctx); /* accept NULL pointer */
size_t ZSTD_freeDCtx(ZSTD_DCtx* dctx); /* accept NULL pointer */
mapleFU
left a comment
mapleFU
left a comment
There was a problem hiding this comment.
General idea ok to me, would createCctx Dctx in ctor expansive?
| size_t ret = ZSTD_compress(output_buffer, static_cast<size_t>(output_buffer_len), | ||
| input, static_cast<size_t>(input_len), compression_level_); | ||
| if (compression_context_ == nullptr) { | ||
| compression_context_ = ZSTD_createCCtx(); |
There was a problem hiding this comment.
I don't would this class be used multi threaded...
There was a problem hiding this comment.
The comments in your issue show that the codec methods are not multi-thread safe. But there are many fail unit tests. I will check whether codec is shared in multiple threads.
There was a problem hiding this comment.
TEST_F(TestWriteRecordBatch, WriteWithCompression) will share codec in multiple threads. So the implementation of this PR is not suitable. I will fix it.
github-actions
bot
added
awaiting committer review
I guess that it will create a context every time we call |
HuaHuaY
force-pushed
the
arrow_zstd_context
branch
from
51a7bf9 to
ba4bdde
Compare
|
In previous comments, we found that the
I chose option 2. But after repeated testing, I found that in my private environment, reusing the context only resulted in a significant performance improvement when the compression level was greater than or equal to 6. There was no performance improvement, and even a performance decrease, when the compression level was less than 6 or during decompression. In the screenshot, a template parameter explicitly set to true indicates context reuse. 
Therefore, I've decided to close this PR. Those interested can still view the PR's commits. |
|
For the record, a similar request was made in #48192 |
3 participants
Rationale for this change
Reduce zstd compression/decompression resource consumption by reusing context.
What changes are included in this PR?
ZSTDCodecwill create a context lazily.Are these changes tested?
Yes. I improved the benchmark to test different compression level. And I test in my private environment. The above is the benchmark of this PR, and the following is the original one.
Are there any user-facing changes?
No.