[#2362] feat(client): Add support of zstd parallel compression#2363
[#2362] feat(client): Add support of zstd parallel compression#2363zuston merged 2 commits intoapache:masterfrom
Conversation
zuston
changed the title
|
cc @jerqi |
|
Could u add the document for this PR? |
Done |
| @Override | ||
| public byte[] compress(byte[] src) { | ||
| return Zstd.compress(src, compressionLevel); | ||
| ZstdCompressCtx ctx = new ZstdCompressCtx(); |
There was a problem hiding this comment.
One question: Do we need to create Ctx for every time? It will cost time if ZstdCompressCtx will create and destroy the thread.
There was a problem hiding this comment.
Nice catch. Actually the code of Zstd.compress(src, compressionLevel); also will create ctx everytime. Let me dig this deeply
There was a problem hiding this comment.
It's ok if the default context don't need to create extra threads actually. Just feel that we should consider more about this place.
There was a problem hiding this comment.
Yes, the compression ctx could be reused.
| if (bufferManager != null) { | ||
| bufferManager.freeAllMemory(); | ||
| try { | ||
| bufferManager.close(); |
There was a problem hiding this comment.
If bufferManager has the method close, the method close method should contain freeAllMemory method.
|
After test, I found we can't reuse the zstc ctx. @jerqi . And so the cost of multi workers compression could be up to users. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2363 +/- ##
============================================
+ Coverage 51.25% 51.36% +0.11%
Complexity 3007 3007
============================================
Files 477 480 +3
Lines 22954 22974 +20
Branches 2116 2115 -1
============================================
+ Hits 11765 11801 +36
+ Misses 10449 10438 -11
+ Partials 740 735 -5 ☔ View full report in Codecov by Sentry. |
|
ptal again @jerqi |
…pache#2363) ### What changes were proposed in this pull request? Add support of zstd parallel compression ### Why are the changes needed? for apache#2362 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing tests --------- Co-authored-by: Junfan Zhang <zhangjunfan@qiyi.com>
3 participants
What changes were proposed in this pull request?
Add support of zstd parallel compression
Why are the changes needed?
for #2362
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing tests