perf: use box instead of vec, write_all instead of loop #467
Conversation
|
The second change would require the first call to write to have at least 8 bytes in the buffer or fail while it could previously succeed with less by only writing part of the header and continuing in a later call with more output buffer available so I don't know if the change would be okay. It might not be something that is tested for currently and probably not something that comes up very often but it it is something that could happen I guess. It's not really anything specific to gzip. Is there some substantial overhead from how it's implemented currently? As a side note I think you could further reduce the write struct site slightly by using a smaller data type for |
There was a problem hiding this comment.
Thanks a lot for contributing, it's wonderful to see this codebase getting an in-depth review and the improvements coming out of that.
I think it's better to make this PR about reducing the size of the GzEncoder struct, there should be no objections to merging it when done.
Thanks again
| while !self.header.is_empty() { | ||
| let n = self.inner.get_mut().write(&self.header)?; | ||
| self.header.drain(..n); | ||
| } |
There was a problem hiding this comment.
This change we should probably avoid for safety - in practice I'd expect the entire header to written anyway, but as a corner case, this fine-grained write handling is certainly preferable.
There was a problem hiding this comment.
write_all makes sense here. It is essentially the same as the loop. However the drain or reassignment of self.header is not necessary. We can just leave that for the final deallocation of the encoder.
There was a problem hiding this comment.
Thanks for sharing!
I think the original concern comes from what happens if the write fails. Semantically, the current version would be able to continue exactly where it left off, when seen just by itself. I don't know if that is anything to be concerned about though, or if it's anything that would happen in practice.
Since you suggested to do the header draining/clearing on drop essentially, you don't seem to share the concern so maybe it's save to just go with the most obvious implementation?
But if so, why would someone write the current version in the first place, if not with the intend that it is granular.
There was a problem hiding this comment.
So it appears that the code is written this way to allow a write to fail with an error, but the caller can call write again and, if the error does not re-occur, then the file gets written correctly.
That might happen for non-blocking writes where EAGAIN gets returned. write_all isn't so useful in this case since it loses how many bytes were written before the EAGAIN (or an EINTR).
I agree then that it is safer to keep this as it is.
| @@ -57,7 +57,7 @@ pub fn gz_encoder<R: BufRead>(header: Vec<u8>, r: R, lvl: Compression) -> GzEnco | |||
| let crc = CrcReader::new(r); | |||
| GzEncoder { | |||
| inner: deflate::bufread::DeflateEncoder::new(crc, lvl), | |||
| header, | |||
| header: header.into_boxed_slice(), | |||
There was a problem hiding this comment.
Generally I expect users to be more concerned by performance than storage size for these types. Converting a Vec into a boxed slice potentially reallocates, and I suspect the possible performance loss is not really worth the memory saving.
There was a problem hiding this comment.
Right! It's probably the shedding of excess capacity that can cause the reallocation.
Let's keep the Vec then.
|
Thanks everyone for the feedback regarding these changes, will close this as the proposed changes aren't right/needed. |
In
bufread:headeris used only for reading the data, so Boxing the slice instead of using a Vec is more memory efficient (16 vs 24).In
write:As we just write until we encounter an error or empty the data, we can use
write_allto simplify things and just reset the header afterwards, but I'm not sure if this is ok to do so, we are draining the header as we write and halt if we encounter an error, whereas this PR does not drain the data 'progressively', only if writing as a whole is successful (additional question: is this considered to be a breaking change if we change this behaviour?).Unfortunately I don't know enough about Gzip to conclude if this is ok or problematic, can someone clarify? While looking through the code I do have found this exact behaviour in other places and wanted to make perf PRs, but tests pass successfully and I'm not sure if these might pose problems eventually.