feat: add ZipFileBuilder for parallelizing compression - #911
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new “prepare then append” workflow to support parallelizing per-file compression work outside of ZipWriter, while still producing correct ZIP metadata when appending the pre-compressed payloads.
Changes:
- Introduces
ZipFileBuilderto compress a single file entry into memory while tracking CRC-32 and sizes, producing aPreparedZipFile. - Adds
ZipWriter::add_prepared_fileto append aPreparedZipFileby copying its already-compressed bytes verbatim (including handling ZIP64 header patching and stream/data-descriptor mode). - Adds tests covering roundtrip reading, duplicate-name rejection (and writer recovery), stream mode behavior, and encryption rejection.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Its-Just-Nans
left a comment
Its-Just-Nans
left a comment
There was a problem hiding this comment.
Hi,
Thanks for the MR, a little question in comment.
Also, what do you think about adding a test with a thread (like the documentation)?
|
I originally had a test case with some threads (similar to the doc comment), but I removed it on the theory that the borrow checker meant it had to work :-) Happy to add it back. |
Compresses a file's contents independently of any ZipWriter, so that multiple files can be compressed on separate threads and then appended serially with ZipWriter::add_prepared_file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed, thanks! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/write.rs:1801
- add_prepared_file() writes the already-compressed payload via ZipWriter::write_all(), which unconditionally updates ZipWriterStats (CRC32 + byte count) on the compressed bytes (see ZipWriter::write at write.rs:761+). For prepared files the CRC/sizes are already known, so this does extra CPU work proportional to the compressed size and can partially defeat the purpose of off-thread compression. Writing directly to the inner writer avoids the redundant hashing and bookkeeping.
// start_entry leaves the inner writer as a bare Storer (a compression encoder is only
// installed by start_file*), so the already-compressed bytes pass through unchanged.
let result = self.write_all(&data);
self.ok_or_abort_file(result)?;
Skips ZipWriter::write's per-byte stats bookkeeping, which is redundant for pre-compressed data whose CRC and sizes are already known. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/write.rs:2332
- Doc comment reads as if encryption-free options would fail. It likely meant that creating a builder will fail when encryption is requested; adding a comma clarifies the intended meaning.
/// Encryption is not supported; pass options without encryption or creation will fail.
|
Would you mind pressing the button so CI runs? |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Looks like you made some fixes/improvements here, thanks! Let me know if there's anything more needed from me. |
sorry for the delay @Pr0methean if you want to review |
|
No need to apologize!
All that is necessary for evil to succeed is for good people to do nothing.
…On Thu, Sep 3, 2026, 11:41 AM n4n5 ***@***.***> wrote:
***@***.**** approved this pull request.
—
Reply to this email directly, view it on GitHub
<#911?email_source=notifications&email_token=AAAAGBHAB4O5QSPWH76Y3GL5NGGJFA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMJQGM4DQNJXHEZKM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-5103885792>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAGBHNDYS3A3EOZZC7QLD5NGGJFAVCNFSNUABFKJSXA33TNF2G64TZHM3DGMJWHAYDQOJWHNEXG43VMU5TKMBZG43DEMJUGY2KC5QC>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/AAAAGBG7OERWWLM2SC2BZET5NGGJFA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMJQGM4DQNJXHEZKM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG>
and Android
<https://github.com/notifications/mobile/android/AAAAGBHSCR4GT3NBJ72AAD35NGGJFA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMJQGM4DQNJXHEZKM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>.
Download it today!
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
I'll take a look when I can, but it may take a few days. I'm busy with a move that's taking longer than expected, and the new top for my computer desk hasn't yet arrived as it should have (I threw out the old top rather than move it because it was damaged). Then I'll have to catch up on all the things I've had to postpone for the move (which was originally planned to be finished at the end of August), such as getting my Internet connection transferred. Feel free to ping me again in a week if I haven't gotten to it by then. |
|
It's now merged Thanks for the MR |
|
Thanks for the reviews! |
Compresses a file's contents independently of any ZipWriter, so that multiple files can be compressed on separate threads and then appended serially with ZipWriter::add_prepared_file.