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 new constructor to ZstdDictCompress and ZstdDictDecompress #306

Merged
merged 2 commits into from
Apr 3, 2024

Conversation

mortengrouleff
Copy link
Contributor

that allows the byReference semantics for the provided byte buffer: If you set this to true, you avoid the copying of the dict data into a natively malloc'ed buffer, but then also have to promise that the byte buffer will not be modified before the CTX has been closed.

allows the byReference semantics for the provided byte buffer: If you
set this to true, you avoid the copying of the dict data into a
natively malloc'ed buffer, but then also have to promise that the byte
buffer will not be modified before the CTX has been closed.
Copy link

codecov bot commented Apr 3, 2024

Codecov Report

Merging #306 (8c0407e) into master (77e74a7) will decrease coverage by 0.11%.
Report is 6 commits behind head on master.
The diff coverage is 72.72%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #306      +/-   ##
============================================
- Coverage     60.01%   59.90%   -0.11%     
- Complexity      308      313       +5     
============================================
  Files            26       26              
  Lines          1473     1489      +16     
  Branches        170      176       +6     
============================================
+ Hits            884      892       +8     
- Misses          434      441       +7     
- Partials        155      156       +1     

@mortengrouleff
Copy link
Contributor Author

Hi @luben

Thanks for your continuing efforts to maintain this library.

I'd like to add this slight API extension that allows not triggering a copy-operation in the native layer, but instead letting the native layer use the direct byte buffer it was supplied for the dict. The native zstd already has this feature as "ByReference" methods. The purpose is to avoid the external malloc that is incurred by the native code for the existing way of accessing the dict, by instead allowing the application to keep a pool of direct buffers.

The refactor has been done in a manner that should be 100% compatible with existing clients.

The new "get" method for the shared buffer reference is to make it easy for the client application to get the proper reference that it needs to either "free" or "return to pool" just before it closes the CTX instance. If the field was not accessible, client apps would need to track this ref along with the CTX ref.

@luben
Copy link
Owner

luben commented Apr 3, 2024

LGTM, thanks for the contribution!

@luben luben merged commit b833326 into luben:master Apr 3, 2024
8 checks passed
@mortengrouleff mortengrouleff deleted the directbuffer-dict branch April 3, 2024 12:18
@mortengrouleff
Copy link
Contributor Author

Thanks!

Would it be fair to ask for a minor version bump and a published build, so that I can start using this 'for real"?

@luben
Copy link
Owner

luben commented Apr 3, 2024

Sure, I bumped the version earlier today for some build system changes and I can release it later today or tomorrow.

@luben
Copy link
Owner

luben commented Apr 4, 2024

Published 1.5.6-2

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