-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Change ZipBlocks structs to classes #113453
Conversation
Tagging subscribers to this area: @dotnet/area-system-io-compression |
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.
Pull Request Overview
This PR converts several ZipBlock types from structs to classes to prevent unintended copying in async scenarios. The key changes include:
- Converting multiple structs (e.g. ZipGenericExtraField, Zip64ExtraField, etc.) into sealed classes.
- Changing default initializations from “default” to “new()” with object initializers.
- Adjusting nullability of fields to support class semantics.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs | Converted various Zip* types from structs to sealed classes, updating initialization and field nullability. |
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.FieldLengths.cs | Updated type declarations from struct to sealed class for consistency. |
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.FieldLocations.cs | Converted type declarations from struct to sealed class for consistency. |
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs | Updated default instantiation of Zip64ExtraField references to use “new()”. |
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs | Updated instantiation of ZipCentralDirectoryFileHeader from struct to class. |
Comments suppressed due to low confidence (1)
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs:780
- The lazy initialization in the ArchiveComment getter guarantees a non-null return but may hide cases where the archive comment was expected to be explicitly set. Please confirm that this behavior is intended and that it does not mask potential errors in data handling.
public byte[] ArchiveComment { get { _archiveComment ??= []; return _archiveComment; } }
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs
Show resolved
Hide resolved
Can you point to an example for why these need to change to support async? |
Of course. We had the exact same problem with Tar: #72472 |
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs
Outdated
Show resolved
Hide resolved
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.
LGTM
This reverts commit 994e451.
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs
Outdated
Show resolved
Hide resolved
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.
Thanks for the mention @carlossanlop. Some of these classes are fairly large and I think we could save allocating a few instances, but I could easily follow up with those separately if we want to keep this PR clear.
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs
Outdated
Show resolved
Hide resolved
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.
Looks pretty good
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs
Outdated
Show resolved
Hide resolved
/ba-g failures are timeouts: |
Contributes to #1541
Structs cannot be passed around in async code (a copy gets created), we need to convert them to classes so they can be used in the incoming async code.
Here's the perf impact comparison before and after:
(struct)
(class)
I used the benchmarks that I am proposing in this PR dotnet/performance#4764
cc @edwardneal you might be interested in this change.