-
Notifications
You must be signed in to change notification settings - Fork 89
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
feat: add lz4 support #331
base: main
Are you sure you want to change the base?
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(); |
src/codec/lz4/encoder.rs
Outdated
unsafe { | ||
self.maybe_buffer | ||
.as_mut() | ||
.unwrap_unchecked() |
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.
I'm slightly worried about this unwrap_unchecked
, would be good if we can optimize this out the unwrap
completely, i.e. by returning another option along side dst_buffer
src/codec/lz4/encoder.rs
Outdated
unsafe { | ||
self.maybe_buffer | ||
.as_mut() | ||
.unwrap_unchecked() |
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.
I'm slightly worried about this unwrap_unchecked
, would be good if we can optimize this out the unwrap
completely, i.e. by returning another option along side dst_buffer
This PR adds LZ4 support, drawing from the gzip (encoder) and bzip2 (decoder) implementations.