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

Implement QAT Zstd sequence producer plugin #12

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

Coder-256
Copy link

Implements the QatZstdSequenceProducer class, links with QAT-ZSTD-Plugin, implements a JNI interface, and includes benchmarks, tests, and several other improvements.

@matt-000 @tparisi52

@luben
Copy link

luben commented Dec 8, 2023

Interesting, what speedups do you observe?

Copy link

@mulerm mulerm left a comment

Choose a reason for hiding this comment

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

Thank you for this PR. Please review and try to address the comments.

.gitignore Outdated
Copy link

Choose a reason for hiding this comment

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

LGTM.

Copy link

@mulerm mulerm Dec 8, 2023

Choose a reason for hiding this comment

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

  1. JMH has more params than -t. And, with BenchmarkDriver not being modified, you don't need to change how the benchmark is run.
  2. Instead, add a third column, Params, to the existing column that specifies the custom parameters each class takes.
  3. Remove java -jar target/benchmarks.jar ~/Downloads/silesia.concat -t64 -p zstdChunkSize=16384 QatZstdBench ZstdSoftwareBench.

Copy link

Choose a reason for hiding this comment

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

This file is hosted and maintained somewhere else. Please remove.

Copy link

Choose a reason for hiding this comment

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

Useful but would not work when the benchmark mode is other than thrpt. Please remove.

Copy link

Choose a reason for hiding this comment

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

LGTM.

Copy link

Choose a reason for hiding this comment

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

LGTM.

Copy link

Choose a reason for hiding this comment

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

LGTM.

Copy link

@mulerm mulerm Dec 8, 2023

Choose a reason for hiding this comment

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

  1. Good catch with the possible torn-writes issue spotted here. I think _Thead_local with the helper function is a heavy-handed approach. Will address this with static initialization in the next update.
  2. Do not modify the return types for existing functions.

Copy link

Choose a reason for hiding this comment

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

Same comment. Do not break existing code by modifying the return types of the existing functions.

Copy link

Choose a reason for hiding this comment

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

LGTM, pending coverage analysis.

@tparisi52
Copy link

@mulerm We've updated the PR based on your feedback - fixing breaking changes, standardizing the benchmarks, etc. Please re-review and advise if any further changes are needed. Thank you for your patience.

@Coder-256

@luben
Copy link

luben commented Mar 27, 2024

@mulerm , @Coder-256 - Zstd-1.5.6 have deprecated some of the functions you use and modified the parameters of others. I will disable the Sequence API in the zstd-jni to not block the update

@mulugetam
Copy link
Contributor

@mulerm , @Coder-256 - Zstd-1.5.6 have deprecated some of the functions you use and modified the parameters of others. I will disable the Sequence API in the zstd-jni to not block the update

Thank you @luben. We will review the changes.

@luben
Copy link

luben commented Apr 3, 2024

I have updated the zstd-jni to use the new signatures and released v1.5.6-1. The only warning left is about deprecation of ZSTD_extractSequences

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.

None yet

5 participants