Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hardware-accelerated codecs for DEFLATE and LZ4 #122

Merged

Conversation

mulugetam
Copy link
Contributor

@mulugetam mulugetam commented Mar 15, 2024

Description

Adds hardware-accelerated DEFLATE and LZ4 compression codecs for stored fields. The hardware in focus here is Intel (R) QAT, which is an integrated, built-in accelerator on the latest 4th and 5th Gen Intel Xeon processors. The implementation relies on the Qat-Java library.

The PR adds two additional valid values for index.codec: qat_deflate and qat_lz4. It also introduces a new setting, index.codec.qatmode, that specifies the mode of execution for QAT.

Two values are supported for index.codec.qatmode: hardware and auto. A hardware execution mode uses only the QAT hardware, while an auto execution mode may switch to software if hardware resources are not available.

Closes

#130

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>
@mulugetam mulugetam changed the title Add hardware-accelerated DEFLATE and LZ4 compression codec Add hardware-accelerated codecs for DEFLATE and LZ4 Mar 15, 2024
@sarthakaggarwal97
Copy link
Collaborator

@mulugetam thanks for raising the PR. Could you please share some performance numbers for these modes?

Choose a reason for hiding this comment

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

In case anyone's wondering, it looks like there is already BSD software in the project: https://github.com/search?q=repo%3Aopensearch-project%2FOpenSearch+bsd+license&type=code

@asonje
Copy link

asonje commented Mar 18, 2024

@mulugetam thanks for raising the PR. Could you please share some performance numbers for these modes?

Here are some performance numbers for indexing using stack overflow workload

  qat_deflate relative to deflate qat_lz4 relative to default
Total time -11.4% -2.5%
Mean Indexing throughput 23.7% 3.4%
Store Size 1.6% -1.76%

@mulerm
Copy link
Contributor

mulerm commented Mar 19, 2024

Thank you @asonje. @sarthakaggarwal97 we will also share the performance numbers for search when they're ready.

@sarthakaggarwal97
Copy link
Collaborator

thanks @mulugetam @asonje for initial numbers. Out of curiosity, if the underlying algorithm is still same (in this case lz4, zlib), how are we seeing differences in store size?

@asonje
Copy link

asonje commented Mar 20, 2024

Out of curiosity, if the underlying algorithm is still same (in this case lz4, zlib), how are we seeing differences in store size?

This is expected. As you know the store size will vary from run to run but it is still a good approximation of compression ratio. qat_deflate and deflate will have different compression ratios because even though the algorithm is the same, their implementations are different. The resulting compressed bytes will not be identical in both cases even though they are compatible.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>
@reta
Copy link
Collaborator

reta commented Mar 27, 2024

@mulugetam it looks pretty cool, could you please share what arch/oses it is available? (the arch part is somewhat clear, but not arch + os combinations, windows / linux / intel macs, ...)

…xception.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>
@mulerm
Copy link
Contributor

mulerm commented Mar 27, 2024

@mulugetam it looks pretty cool, could you please share what arch/oses it is available? (the arch part is somewhat clear, but not arch + os combinations, windows / linux / intel macs, ...)

The QAT built-in accelerator is available on 4th and 5th gen Intel (R) Xeon Processors. This version requires amd64/Linux. For all other systems, the auto mode can be used to do a software-only compression.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>
@reta
Copy link
Collaborator

reta commented Mar 27, 2024

@mulugetam when you have chance, could you please resolve the conflicts? thank you

@mulugetam
Copy link
Contributor Author

@mulugetam when you have chance, could you please resolve the conflicts? thank you

Looks like it's asking me to insert new lines because existing code is not spotless formatted.

Signed-off-by: mulugetam <mulugeta.mammo@intel.com>
Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>
@mulugetam
Copy link
Contributor Author

2. Based on the PR my understanding is that the storage format does not change. I think in future, codec like lz4_qat should be inferred, and continue to work with other implementation of LZ4 in the example case. We should add tests to ensure that the storage remains compatible always if we're adding changes which impact only compute (e.g. use lz4_qat for compression, and lz4 for decompression)

Thanks. I believe some of us discussed similar ideas on Slack. An issue entry was also created: #130.

@mulugetam
Copy link
Contributor Author

@reta do we have any pending issues that need to be resolved?

@reta
Copy link
Collaborator

reta commented May 15, 2024

@reta do we have any pending issues that need to be resolved?

@mulugetam we still need a signoff #122 (comment)

@sarthakaggarwal97
Copy link
Collaborator

sarthakaggarwal97 commented May 15, 2024

We should add tests to ensure that the storage remains compatible always if we're adding changes which impact only compute (e.g. use lz4_qat for compression, and lz4 for decompression)

@mulerm I think we can add this test to ensure the only change would be in the compute. What do you think @reta?

@reta
Copy link
Collaborator

reta commented May 15, 2024

@mulerm I think we can add this test to ensure the only change would be in the compute. What do you think @reta?

@sarthakaggarwal97 I am not sure we could pull it off, for lz4_qat we need a real hardware support, right? I don't think it is available in GA.

@mulugetam
Copy link
Contributor Author

@reta @sarthakaggarwal97

Adding the suggested test in the current implementation is not sufficient and probably will not work, as the fieldsWriter and fieldsReader of Lucene store BEST_COMPRESSION and BEST_SPEED as the value of the MODE_KEY attribute.

My thinking was that we should treat qat_deflate (vs. BEST_COMPRESSION) and qat_lz4 (vs. BEST_SPEED) as separate codecs until such a time when BEST_COMPRESSION and BEST_SPEED would just use the accelerator "transparently", if it is present.

@reta
Copy link
Collaborator

reta commented May 15, 2024

My thinking was that we should treat qat_deflate (vs. BEST_COMPRESSION) and qat_lz4 (vs. BEST_SPEED) as separate codecs

Certainly +1 to that

@mgodwan
Copy link
Member

mgodwan commented May 16, 2024

I am not sure we could pull it off, for lz4_qat we need a real hardware support, right? I don't think it is available in GA.

If this is the case, I would highly advocate for this to be behind a plugin setting. #148

@reta
Copy link
Collaborator

reta commented May 16, 2024

If this is the case, I would highly advocate for this to be behind a plugin setting. #148

But it is separate codec already, which users have to opt-in to use (not a default one)? So users have to pick a codec AND set a setting to use it? Sounds like unreasonably complicated process to me

@mgodwan
Copy link
Member

mgodwan commented May 16, 2024

But it is separate codec already, which users have to opt-in to use (not a default one)?

So was the case when zstd was added to OpenSearch in the first iteration, but it was done via a sandbox plugin. Since the plugin is no longer sandbox, I think it would benefit to have a way to denote this is experimental and innovate with time on the settings, codec management, issue handling, etc. for these new codecs without worrying about breaking changes.

The only reason I'm saying this is that the codec impacts storage of data and in case of issues, just disabling may not bring users out of any issues unless already written data is also fixed.

That said, I'm okay with the call you and @sarthakaggarwal97 take on this.

@sarthakaggarwal97
Copy link
Collaborator

sarthakaggarwal97 commented May 17, 2024

Given the precedent we have had with Zstd, I think its okay to keep the new QAT codecs as experimental for now.
Currently, there is not a clean way to mark the codecs as experimental, and I have opened an issue opensearch-project/OpenSearch#13723.

With that, it would be nice if we can come up with a plan as well to make new codecs, here QAT, generally available in the future. Let me think back on it. One of the list I created earlier for Zstd correctness was this for reference: opensearch-project/OpenSearch#9502

@reta
Copy link
Collaborator

reta commented May 17, 2024

Given the precedent we have had with Zstd, I think its okay to keep the new QAT codecs as experimental for now.

@sarthakaggarwal97 we have #148 to address the problem for every custom codec.

@mulugetam
Copy link
Contributor Author

@sarthakaggarwal97 we have #148 to address the problem for every custom codec.

@reta @sarthakaggarwal97 Are we on hold now until #148 is implemented? I think we should not be, as it is a separate codec that users would have to opt in to use.

@reta
Copy link
Collaborator

reta commented May 29, 2024

@sarthakaggarwal97 we have #148 to address the problem for every custom codec.

@reta @sarthakaggarwal97 Are we on hold now until #148 is implemented? I think we should not be, as it is a separate codec that users would have to opt in to use.

@mulugetam I don't think we need to wait for #148, we could addressed that right after (before 2.15.0)

@sarthakaggarwal97
Copy link
Collaborator

@reta I think we would need to introduce the experimental settings in OpenSearch, since we do the validation of index codecs in EngineConfig

Ideally would want to stop the creation of index only if the codecs is not experimental. I feel we would need a two changes. A new method in CodecSettings can tell us if the codec is experimental or not, and a feature flag setting will tell us whether we should make the experimental codecs available or not.

@sarthakaggarwal97
Copy link
Collaborator

Thank you @mulugetam for this change.

@sarthakaggarwal97 sarthakaggarwal97 merged commit c8b0d80 into opensearch-project:main May 30, 2024
17 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/custom-codecs/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/custom-codecs/backport-2.x
# Create a new branch
git switch --create backport/backport-122-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 c8b0d80a8286459857f2db2c0e9d3c1c076ada9d
# Push it to GitHub
git push --set-upstream origin backport/backport-122-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/custom-codecs/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-122-to-2.x.

@sarthakaggarwal97
Copy link
Collaborator

@mulugetam would you please help with the backport as well? Thank you

@reta
Copy link
Collaborator

reta commented May 30, 2024

@reta I think we would need to introduce the experimental settings in OpenSearch, since we do the validation of index codecs in EngineConfig

@sarthakaggarwal97 it would be great but we validation logic won't help us here I think: the codecs are registered by Apache Lucene SPI (the validation logic you are referring to only helps with ensuring the codec settings validness).

Ideally would want to stop the creation of index only if the codecs is not experimental. I feel we would need a two changes. A new method in CodecSettings can tell us if the codec is experimental or not, and a feature flag setting will tell us whether we should make the experimental codecs available or not.

That's one of the problems: we could extend CodecSettings (this is ours) but none of the Codec implementations are required to implement it. Ideally, Apache Lucene SPI should have that feature, or alternatively, we could disable some codecs through settings.

@mgodwan
Copy link
Member

mgodwan commented May 30, 2024

the codecs are registered by Apache Lucene SPI (the validation logic you are referring to only helps with ensuring the codec settings validness).

I think a check like this will definitely help to disable its usage in the write path. For write path, NamedSPI interface is not used.

mulugetam added a commit to mulugetam/custom-codecs that referenced this pull request May 30, 2024
…ct#122)

* Add QAT accelerated compression.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>

* Use own classes for QAT codec. Apply SpotlessJavaCheck.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>

* Declare fields final, unless required not to. Throw a valid type of exception.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>

* Use assumeThat in the Qat test classes.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>

* Add more QAT availability check in QatCodecTests.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>

* Make LZ4 the default algorithm for QAT.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>

* Make 'auto' the default execution mode for QAT. Also, minor clean up work.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>

* Revert compression level for ZSTD to 3.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>

* Replace QatLz4/DeflateCompressionMode classes with QatCompressionMode.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>

* Fix a MultiCodecMergeIT test fail.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>

* Remove hard-coded values for default compression level.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>

---------

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>
Signed-off-by: mulugetam <mulugeta.mammo@intel.com>
Co-authored-by: Mulugeta Mammo <cppx86@gmail.com>
(cherry picked from commit c8b0d80)
@mulugetam
Copy link
Contributor Author

@mulugetam would you please help with the backport as well? Thank you

@reta @sarthakaggarwal97 #150

reta pushed a commit that referenced this pull request May 30, 2024
* Add QAT accelerated compression.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>

* Use own classes for QAT codec. Apply SpotlessJavaCheck.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>

* Declare fields final, unless required not to. Throw a valid type of exception.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>

* Use assumeThat in the Qat test classes.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>

* Add more QAT availability check in QatCodecTests.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>

* Make LZ4 the default algorithm for QAT.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>

* Make 'auto' the default execution mode for QAT. Also, minor clean up work.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>

* Revert compression level for ZSTD to 3.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>

* Replace QatLz4/DeflateCompressionMode classes with QatCompressionMode.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>

* Fix a MultiCodecMergeIT test fail.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>

* Remove hard-coded values for default compression level.

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>

---------

Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com>
Signed-off-by: mulugetam <mulugeta.mammo@intel.com>
Co-authored-by: Mulugeta Mammo <cppx86@gmail.com>
(cherry picked from commit c8b0d80)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants