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 zstd Sequence Producer API #290

Closed
wants to merge 15 commits into from

Conversation

Coder-256
Copy link
Contributor

Hi,

Recently, @matt-000, @tparisi52 and I have been working with the Lehigh University Enterprise Systems Center on a project to implement Java bindings for QAT-ZSTD-Plugin, a library that accelerates zstd compression using Intel QuickAssist Technology (a hardware accelerator architecture). This PR adds support for the zstd sequence producer API introduced in zstd v1.5.4 (facebook/zstd#3333) and adds a basic test suite using a sequence producer based on ZSTD_generateSequences().

We also implemented support for a few more compression context configuration parameters, and improved support for zstd param switches (notably LDM is now configured as documented).

Thank you!

@codecov-commenter
Copy link

codecov-commenter commented Nov 29, 2023

Codecov Report

Merging #290 (6202971) into master (8e0c89b) will increase coverage by 0.16%.
The diff coverage is 58.62%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #290      +/-   ##
============================================
+ Coverage     59.57%   59.74%   +0.16%     
- Complexity      300      307       +7     
============================================
  Files            26       26              
  Lines          1415     1473      +58     
  Branches        163      170       +7     
============================================
+ Hits            843      880      +37     
- Misses          417      436      +19     
- Partials        155      157       +2     

Copy link
Owner

@luben luben left a comment

Choose a reason for hiding this comment

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

Looks great! I left 2 small nits

@luben
Copy link
Owner

luben commented Nov 30, 2023

Thanks for fixing that unnecessary unlocking. Can you rebase it? Looks there is some minor conflict that prevents the merge.

@Coder-256
Copy link
Contributor Author

Should be all set now, thanks!

@luben
Copy link
Owner

luben commented Nov 30, 2023

I rebased and merged that manually

@luben luben closed this Nov 30, 2023
@luben
Copy link
Owner

luben commented Nov 30, 2023

I had to skip 61ac4c1 as it had conflicts and that part was reworked quite extensively recently

@luben
Copy link
Owner

luben commented Dec 1, 2023

@Coder-256 If you have it integrated with QAT-ZSTD-Plugin in some opensource project, I am very interested to take a look, may be even link to it.

BTW, I just published v1.5.5-11 and it should land on maven central soon.

@Coder-256
Copy link
Contributor Author

Coder-256 commented Dec 1, 2023

Perfect, and that's exactly what we're doing! We'll be opening a pull request to qat-java in the coming days adding a "QatZstdSequenceProducer", I'll link the PR here once it's ready.

@Coder-256
Copy link
Contributor Author

Opened intel/qat-java#12 if you'd like to follow along!

@luben
Copy link
Owner

luben commented Mar 27, 2024

@Coder-256 , looks that the use of ZSTD_generateSequences is deprecated in 1.5.6 and the comments suggest using ZSTD_compress instead.

There is also another error in the same code:

[error] /home/luben/zstd-jni/src/main/native/jni_zstd.c: In function ‘builtinSequenceProducer’:
[error] /home/luben/zstd-jni/src/main/native/jni_zstd.c:312:3: warning: ‘ZSTD_generateSequences’ is deprecated [-Wdeprecated-declarations]
[error]   312 |   size_t numSeqs = ZSTD_generateSequences((ZSTD_CCtx *)sequenceProducerState, outSeqs, outSeqsCapacity, src, srcSize);
[error]       |   ^~~~~~
[error] In file included from /home/luben/zstd-jni/src/main/native/jni_zstd.c:5:
[error] /home/luben/zstd-jni/src/main/native/zstd.h:1585:1: note: declared here
[error]  1585 | ZSTD_generateSequences(ZSTD_CCtx* zc,
[error]       | ^~~~~~~~~~~~~~~~~~~~~~
[error] /home/luben/zstd-jni/src/main/native/jni_zstd.c: In function ‘Java_com_github_luben_zstd_Zstd_registerSequenceProducer’:
[error] /home/luben/zstd-jni/src/main/native/jni_zstd.c:356:35: warning: passing argument 3 of ‘ZSTD_registerSequenceProducer’ from incompatible pointer type [-Wincompatible-pointer-types]
[error]   356 |                                   (ZSTD_sequenceProducer_F *)(intptr_t) seqProdFunction);
[error]       |                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[error]       |                                   |
[error]       |                                   size_t (**)(void *, ZSTD_Sequence *, size_t,  const void *, size_t,  const void *, size_t,  int,  size_t) {aka long unsigned int (**)(void *, ZSTD_Sequence *, long unsigned int,  const void *, long unsigned int,  const void *, long unsigned int,  int,  long unsigned int)}
[error] /home/luben/zstd-jni/src/main/native/zstd.h:2853:27: note: expected ‘ZSTD_sequenceProducer_F’ {aka ‘long unsigned int (*)(void *, ZSTD_Sequence *, long unsigned int,  const void *, long unsigned int,  const void *, long unsigned int,  int,  long unsigned int)’} but argument is of type ‘size_t (**)(void *, ZSTD_Sequence *, size_t,  const void *, size_t,  const void *, size_t,  int,  size_t)’ {aka ‘long unsigned int (**)(void *, ZSTD_Sequence *, long unsigned int,  const void *, long unsigned int,  const void *, long unsigned int,  int,  long unsigned int)’}
[error]  2853 |   ZSTD_sequenceProducer_F sequenceProducer
[error]       |   ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~

@luben
Copy link
Owner

luben commented Mar 27, 2024

I will comment out for the moment the sequences stuff

@Coder-256
Copy link
Contributor Author

Thanks. Anything related to the "builtin sequence producer" should be ok to comment out, it was only used for tests. It also seems that ZSTD_sequenceProducer_F was made into a pointer type, which is frustrating since that's a breaking change, but the fix is just using ZSTD_sequenceProducer_F instead of ZSTD_sequenceProducer_F *.

For reference, here's the relevant part of the diff in zstd.h:

@@ -2765,7 +2818,7 @@ ZSTDLIB_STATIC_API size_t ZSTD_resetDStream(ZSTD_DStream* zds);
 
 #define ZSTD_SEQUENCE_PRODUCER_ERROR ((size_t)(-1))
 
-typedef size_t ZSTD_sequenceProducer_F (
+typedef size_t (*ZSTD_sequenceProducer_F) (
   void* sequenceProducerState,
   ZSTD_Sequence* outSeqs, size_t outSeqsCapacity,
   const void* src, size_t srcSize,
@@ -2797,7 +2850,23 @@ ZSTDLIB_STATIC_API void
 ZSTD_registerSequenceProducer(
   ZSTD_CCtx* cctx,
   void* sequenceProducerState,
-  ZSTD_sequenceProducer_F* sequenceProducer
+  ZSTD_sequenceProducer_F sequenceProducer
+);

@luben
Copy link
Owner

luben commented Mar 27, 2024

Thanks! I fixed the signatures and re-enabled it all and it passes tests. Compiles with a warning. I guess in the future we have to find how to stop using ZSTD_generateSequences as it's going to be removed: https://github.com/facebook/zstd/blob/dev/lib/zstd.h#L1555-L1563

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.

4 participants