-
Notifications
You must be signed in to change notification settings - Fork 95
feat: add lz4 support #331
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
Conversation
Thank you! I have some questions for the PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for taking my advice!
For the impl, I think there are some bugs and some rooms for improvements
src/codec/lz4/encoder.rs
Outdated
let (dst_buffer, dst_max_size) = if direct_output { | ||
let output_len = output.unwritten().len(); | ||
(output.unwritten_mut(), output_len) | ||
} else { | ||
let buffer_size = self.block_buffer_size; | ||
let buffer = self | ||
.maybe_buffer | ||
.get_or_insert_with(|| PartialBuffer::new(Vec::with_capacity(buffer_size))); | ||
buffer.reset(); | ||
(buffer.unwritten_mut(), buffer_size) | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let (dst_buffer, dst_max_size) = if direct_output { | |
let output_len = output.unwritten().len(); | |
(output.unwritten_mut(), output_len) | |
} else { | |
let buffer_size = self.block_buffer_size; | |
let buffer = self | |
.maybe_buffer | |
.get_or_insert_with(|| PartialBuffer::new(Vec::with_capacity(buffer_size))); | |
buffer.reset(); | |
(buffer.unwritten_mut(), buffer_size) | |
}; | |
let (dst_buffer, dst_max_size) = if direct_output { | |
let output_len = output.unwritten().len(); | |
output.unwritten_mut() | |
} else { | |
let buffer = self | |
.maybe_buffer | |
.get_or_insert_with(|| PartialBuffer::new(Vec::with_capacity(self.block_buffer_size))); | |
buffer.reset(); | |
buffer.unwritten_mut() | |
}; | |
let dst_max_size = dst_buffer.len(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
It LGTM except for one comment
cc @robjtede wdyt
src/codec/lz4/encoder.rs
Outdated
LZ4F_compressBegin( | ||
self.ctx.get_mut().ctx, | ||
dst_buffer, | ||
min_dst_size, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
min_dst_size, | |
dst_buffer.unwritten().len(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dst_buffer
is a mut pointer now, so this is not possible. min_dst_size
is the worst case size of the destination buffers required by the lz4 C functions, hence it is safe to use here instead of the actual destination buffer size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could evaluate its length before the function call, then there would only be one active mutable borrow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, LGTM!
cc @robjtede if this look good to you, I'll merge it
Given that there's no objection, I will merge this PR in |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #331 +/- ##
===========================
===========================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This PR adds LZ4 support, drawing from the gzip (encoder) and bzip2 (decoder) implementations.