Skip to content

Conversation

cnswee
Copy link

@cnswee cnswee commented Aug 14, 2023

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:

  • Does the affected code have corresponding tests, e.g. unit test, E2E test?
  • Does this change require a documentation update?
  • Does this introduce breaking changes that would require an announcement or bumping the major version?
  • Do all new files have an appropriate license header?


int nbatch() override {
// return DEFAULT_N_BATCH;
return (qat_enable ? DEFAULT_N_BATCH : 1);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better to use max_batch() instead of nbatch()? And the number is fixed in all QAT hardware, is it?

uint32_t max_dst_size = 0;
uint32_t src_blk_size = 0;
// vector<unsigned char *> raw_data;
vector<unsigned char *> compressed_data;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually we don't need to put the function body backward.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use photon::object_cache instead. It is more optimized for concurrency & performance. And it automatically releases objects as they get cold.

~LZ4Compressor() {
}

bool check_qat() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does check_qat() need root permission?

if (check_qat()) {
pQat = new LZ4_qat_param();
qat_init(pQat);
qat_enable = true;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that batch_decompress() is not used yet in zfile.cpp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants