Remove BinaryReader and BinaryWriter references from ZipArchive #103153
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We now read the data into a (sometimes stack-allocated) byte array and slice it up with BinaryPrimitives. This reduces the number of reads and writes required to read and write a ZipArchive. It also makes future work to enable async APIs easier, since BinaryReader and BinaryWriter lack this support. Also changed approach to reading central file directory headers. Rather than performing X reads per header, we read 4KB of data at a time and look for all applicable headers in that data. This should improve performance when dealing with many small files.
This allowed the removal of the ArchiveReader property from ZipArchive.
Now pooling the file IO buffers and the temporary buffers for extra fields of the CD file header (which would otherwise be allocated and deallocated in a loop.)
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-io-compression |
bartonjs
reviewed
Jun 7, 2024
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs
Outdated
Show resolved
Hide resolved
This handles 64x entries with 19-character filenames (and thus, 65-byte file headers.) As a result, it straddles two 4KB read buffers. Also corrected the seek logic while reading the central directory header
|
@edwardneal the |
carlossanlop
reviewed
Aug 5, 2024
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs
Show resolved
Hide resolved
src/libraries/System.IO.Compression/tests/ZipArchive/zip_ReadTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/tests/ZipArchive/zip_ReadTests.cs
Outdated
Show resolved
Hide resolved
|
Thanks @carlossanlop - I've addressed the merge conflict and made those test changes. I've got a number of post-merge test failures to deal with - I'll look at those shortly, ready for review post-14th. |
This was referenced Aug 8, 2024
The buffer returned from the ArrayPool contained older data (including a ZIP header.) When reading the last chunk of the file (i.e a chunk which was less than BackwardsSeekingBufferSize) the buffer's Span wasn't resized to account for this. SeekBackwardsToSignature would thus find the older data, try to seek beyond the end of the stream and fail to read the file.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Relates to #83909, #31460.
This removes all references to BinaryReader and BinaryWriter from ZipArchive and ZipArchiveEntry. It also changes the way that the ZIP central directory header is read from the source stream and makes one tweak to the way that the EOCD header is detected.
I've removed BinaryReader and BinaryWriter for three reasons:
The second change is to adjust the way that the ZIP central directory header is read. Previously, this was read from the source stream file-by-file. This PR now reads from the source in 4KB blocks and tries to read the headers from there. This is much faster. I've chosen not to implement it when writing the CD headers because they contain dynamic data and I didn't want to copy buffers around; I'm open to doing so.
The detection of the end-of-central-directory header is very similar too. It was already doing something similar, but with only 16 bytes at a time. I've tweaked this to read 4KB block instead, and changed the way it searches for the EOCD signature to use an intrinsic rather than byte-by-byte bit shuffling.
In both cases, I've picked 4KB because it feels like a small enough buffer to not make a massive difference to wait times, and it aligns with the 4KB buffer on FileStream (which I imagine would be the most common use case.)
There are performance improvements across the board. To benchmark this, I used a test wrapping stream which simulates the worst case - an Xms Thread.Sleep on every Read and Write call. Results are below, but in short:
Reads
Creation
Benchmark results - Reads
Benchmark results - Reads (scaling up by number of ZipArchiveEntry children)
Benchmark results - Creation
Benchmark header
NB: because this changes the number of reads/writes to a stream, it'll have an impact on the tests for #102704. I'll change these depending on the order the PRs are merged in.