Add QAT support in zfile #250
Conversation
Signed-off-by: cnswee <siwei.qin@intel.com>
| @@ -192,69 +197,173 @@ class LZ4Compressor : public BaseCompressor { | |||
| return (qat_enable ? DEFAULT_N_BATCH : 1); | |||
There was a problem hiding this comment.
Is it better to use max_batch() instead of nbatch()? And the number is fixed in all QAT hardware, is it?
| @@ -46,7 +49,7 @@ class BaseCompressor : public ICompressor { | |||
| vector<unsigned char *> compressed_data; | |||
There was a problem hiding this comment.
It seems that the content of compressed_data[] / uncompressed_data[] are generated in compress_batch() / decompress_bath, which get used in do_compress() / do_decompress().
I believe these 2 vectors are useless. One can use function-local arrays instead.
| @@ -192,69 +197,173 @@ class LZ4Compressor : public BaseCompressor { | |||
| return (qat_enable ? DEFAULT_N_BATCH : 1); | |||
| } | |||
|
|
|||
| virtual int do_compress(size_t *src_chunk_len, size_t *dst_chunk_len, | |||
| size_t dst_buffer_capacity, size_t nblock) override { | |||
| int do_compress(size_t *src_chunk_len, size_t *dst_chunk_len, size_t dst_buffer_capacity, | |||
There was a problem hiding this comment.
Usually we don't need to put the function body backward.
There was a problem hiding this comment.
I don't feel necessary to have separate do_compress() and do_decompress(). Simply realizes the logic in batch_compress() / batch_decompress(). I do feel necessary to have qat-related logic separated as a singleton object shared by all lz4_decompressor objects (and threads). The qat wrapper object simply provides 2 functions: batch_compress() / batch_decompress(). Whenever they fail, we fall back to the software-only implementation.
| } | ||
|
|
||
| // put the session back to the session pool in a RAII manner | ||
| struct cached_session_t { |
There was a problem hiding this comment.
Use photon::object_cache instead. It is more optimized for concurrency & performance. And it automatically releases objects as they get cold.
| #endif | ||
| bool qat_enable = false; | ||
| ~LZ4Compressor() { | ||
| } | ||
|
|
||
| bool check_qat() { |
There was a problem hiding this comment.
Does check_qat() need root permission?
| @@ -179,8 +186,6 @@ class LZ4Compressor : public BaseCompressor { | |||
| max_dst_size = LZ4_compressBound(src_blk_size); | |||
| #ifdef ENABLE_QAT | |||
| if (check_qat()) { | |||
| pQat = new LZ4_qat_param(); | |||
| qat_init(pQat); | |||
| qat_enable = true; | |||
There was a problem hiding this comment.
We may need to create a lot of decompressor instances, we should integrate check_qat() and session cache into a global qat object.
| "LZ4 decompress returns 0. THIS SHOULD BE NEVER HAPPEN!"); | ||
| } | ||
| for (size_t i = 0; i < n; i++) { | ||
| int rc = qzDecompress(session.get(), compressed_data[i], |
There was a problem hiding this comment.
It seems that batch_decompress() is not used yet in zfile.cpp
What this PR does / why we need it:
Add QAT support in zfile, which means QAT is available for compressing and decompressing data now.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Please check the following list: