Reduce memory usage when updating ZipArchives #102704

edwardneal · 2024-05-26T21:30:26Z

There are a handful of different issues surrounding ZipArchive, so this PR touches on a few. It makes ZipArchive more conservative when writing in WriteFile. It does this by tracking the type of dirty state of each archive entry and of the archive itself.

Previously, every ZipArchiveEntry's data would be loaded into memory, the output stream would have its length set to zero, then the archive entries would be written out in sequence. This naturally causes problems when working with very large ZIP files.

I've changed this behaviour by forcing the ZipArchive to track the offset its first deleted entry, _firstDeletedEntryOffset. When writing a file:

Find the offset of the first entry with any changes at all (startingOffset), and the first entry with changes to its dynamic-length metadata or entry data (dynamicDirtyStartingOffset).
If any entries have an offset greater than startingOffset, add them to the list (entriesToWrite) of entries to persist.
If any entries have an offset greater than dynamicDirtyStartingOffset, load their contents into memory.
Position the stream offset at startingOffset, then start writing each entry in entriesToWrite. When writing:
- If the entry is dirty at all, (or beyond dynamicDirtyStartOffset) write the header. If not, seek past it.
- If the entry has changes to its data, write the data (loaded in an earlier step). If not, seek past it.
If necessary, write (or seek past) each entry's central directory header. Track whether or not any of these were written.
If any central directory headers were written (or if there are no entries in the archive), write the end-of-central-directory block. If not, seek past it.
Shrink the stream to its current position.

This relies upon the list of the ZipArchive's existing ZipArchiveEntry records being sorted in offset order, which is now guaranteed on load.

Issue links:

Resolves Add support for adding new entries to large zip archives #49149: appending a new entry to the ZipArchive is now "free", as long as nothing else has changed in there.
Contributes to System.IO.Compression: ZipArchive loads entire file in memory on .Dispose #1543: the original triage was that should limit ourselves to only loading entries which have changes. This PR doesn't do this - it loads the changed entry and everything after it, since the changed entry could easily write data which would extend it into the next entry. I've made an improvement, but I think a complete fix will be more complex (and might look quite like a compacting heap.)
Resolves Bug? - ZipArchive modifies underlying stream even if no archive changes made #34506: if there are no changes to the ZipArchive, nothing is written.

There's also an issue (1544) which relates to ZipArchiveEntry storing uncompressed data in memory unnecessarily. This PR doesn't fix that issue, but it means that somebody won't fix it and discover that ZipArchive.Dispose is loading the contents into memory anyway.

I've added a number of test cases which cover the corner cases I've thought of, verifying the correct number of writes and the contents of the files as they go. It's quite a core part of ZipArchive though and I'm conscious that a number of libraries depend upon it, so I'm open to adding more.

Edit: From benchmarking the "append case", my results were pretty much as I expected: performance improves, but becomes more variable. Updating/deleting entries at the end of an archive (or after the largest entries in the archive) becomes faster and uses less memory, while updating/deleting entries at the start of an archive takes about as long as it did before.


BenchmarkDotNet v0.13.12, Windows 11 (10.0.22631.3593/23H2/2023Update/SunValley3)
Intel Core i7-8565U CPU 1.80GHz (Whiskey Lake), 1 CPU, 8 logical and 4 physical cores
.NET SDK 9.0.100-preview.3.24204.13
  [Host]     : .NET 9.0.0 (9.0.24.17209), X64 RyuJIT AVX2
  Job-QKCECQ : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2

Toolchain=CoreRun

Benchmark	Mean	Error	StdDev	Gen0	Gen1	Gen2	Allocated
Pre-PR	5.128 s	0.1346 s	0.3925 s	1000.0000	1000.0000	1000.0000	2 GB
Post-PR	8.169 ms	0.2386 ms	0.7036 ms				7.24 KB

WriteFile now takes a more granular approach to writing the ZipArchive to its stream. Adding a new file to the end of the archive will no longer require every file in the archive to be loaded into memory. Changing fixed-length metadata inside a file will no longer require the entire archive to be re-written.

ericstj

I like the premise of making ZipArchive writes more incremental - would like someone from @dotnet/area-system-io-compression to have a look and see if they can give more guidance for direction in what they'd like to see in this PR. Added a couple comments around what stuck out.

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs

Renamed Dirty and DirtyState to Changed and ChangeState. Explicitly assigned Unchanged as a zero-value ChangeState.

Reset _originallyInArchive and _offsetOfLocalHeader to private members, exposed as internal properties. Also changed _versionToExtract to be private - this isn't ever used in System.IO.Compression outside of ZipArchiveEntry.

ericstj · 2024-07-22T21:15:17Z

@carlossanlop can you please review?

udlose · 2024-10-08T20:10:37Z

@carlossanlop @ericstj

is there any update on this?
possibly coming in .net 9?

ericstj · 2024-10-09T20:44:18Z

It's too late for .NET 9 but we'll have a look at getting into main for .NET 10.0. There's potential fallout from this change and we need to make sure it gets a closer look and plenty of time to bake before going out stable.

edwardneal · 2024-10-10T06:50:49Z

Thanks for the update @ericstj.

To expand on the note in the original description:

I've made an improvement, but I think a complete fix will be more complex (and might look quite like a compacting heap.)

This PR makes an improvement improvement to the average case and resolves the case of appending new entries to the ZIP archive, but it's limited: if the user changes the contents of the first entry in the archive, WriteFile will write the entire archive again - even if the entry content changes have resulted in the entry becoming smaller. The same thing will happen if the user renames the file, even if the encoded name is shorter.

A mechanism similar to a compacting heap would fix this, but I felt that needed more design work. The primary tradeoff which I was thinking about was between unused space in the archives, and the time/IO required to start moving entries around (if that's even possible - ZipArchive deals with unseekable streams.) That tradeoff naturally changes as we move towards the end of an archive, since the IO to append a large entry to the end of an archive is less expensive than the IO which reshuffles enough entries to make free space at the start.

If there's already a data structure which handles this case in .NET and which accounts for the cost of IO, then this PR can use that directly and reduce the risk.

steveharter · 2024-10-17T19:29:11Z

From the description's first step:

Find the offset of the first entry with any changes at all (startingOffset), and the first entry with changes to its dynamic-length metadata or entry data (dynamicDirtyStartingOffset).

I assume then that all of the entries from that first changed entry to the last entry will be re-written and thus have no fragmentation and thus be the same physically on disk as before the PR? If so, then we shouldn't need to add a new enum value to ZipArchiveMode but would just need to update the documentation for ZipArchiveMode.Append where it says "The content of the entire archive is held in memory."

steveharter · 2024-10-17T19:33:06Z

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs

@@ -415,7 +427,7 @@ internal void AcquireArchiveStream(ZipArchiveEntry entry)
            {
                if (!_archiveStreamOwner.EverOpenedForWrite)
                {
-                    _archiveStreamOwner.WriteAndFinishLocalEntry();
+                    _archiveStreamOwner.WriteAndFinishLocalEntry(true);


nit:

Suggested change

_archiveStreamOwner.WriteAndFinishLocalEntry(true);

_archiveStreamOwner.WriteAndFinishLocalEntry(forceWrite: true);

steveharter · 2024-10-17T19:35:18Z

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs

@@ -31,6 +31,8 @@ public class ZipArchive : IDisposable
        private byte[] _archiveComment;
        private Encoding? _entryNameAndCommentEncoding;



nit: unnecessary extra line here

steveharter · 2024-10-17T19:43:25Z

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs

@@ -295,6 +302,12 @@ public long Length
        /// </summary>
        public string Name => ParseFileName(FullName, _versionMadeByPlatform);

+        internal ZipArchive.ChangeState Changed { get; private set; }


nit: "Changed" -> "Changes"?

"Changed" sounds like a bool, while "Changes" sounds like the flags it is.

steveharter · 2024-10-17T19:45:23Z

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs

-                ZipGenericExtraField.WriteAllBlocks(_cdUnknownExtraFields, _archive.ArchiveStream);
+            if (_originallyInArchive && Changed == ZipArchive.ChangeState.Unchanged && !forceWrite)
+            {
+                long centralDirectoryHeaderLength = 46


nit: add a const for 46 perhaps with the +'s for readability.

steveharter · 2024-10-17T19:50:04Z

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs

@@ -633,41 +653,108 @@ private void WriteFile()
            // if we are in update mode, we call EnsureCentralDirectoryRead, which sets readEntries to true
            Debug.Assert(_readEntries);

+            // Entries starting after this offset have had a dynamically-sized change. Everything on or after this point must be rewritten.
+            long dynamicDirtyStartingOffset = 0;


"dynamicDirtyStartingOffset" -> "rewriteStartingOffset"?

steveharter · 2024-10-17T19:57:03Z

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs

+                    }
+                    else
+                    {
+                        entriesToWrite.Add(entry);


nit: for understandability of this long method, swap\move entriesToWrite.Add(entry) to the beginning of the block above (e.g. if (!entry.OriginallyInArchive))

edwardneal added 2 commits May 26, 2024 18:15

Added tests for changed WriteFile behaviour

9c03bef

dotnet-issue-labeler bot added the area-System.IO.Compression label May 26, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 26, 2024

edwardneal mentioned this pull request May 29, 2024

Add Flush to ZipArchive #24149

Open

edwardneal mentioned this pull request Jun 7, 2024

Remove BinaryReader and BinaryWriter references from ZipArchive #103153

Open

ericstj requested review from a team and carlossanlop June 24, 2024 15:34

ericstj reviewed Jun 24, 2024

View reviewed changes

edwardneal added 2 commits June 25, 2024 07:05

Changes following code review

03f7fcd

Renamed Dirty and DirtyState to Changed and ChangeState. Explicitly assigned Unchanged as a zero-value ChangeState.

Changed field protection levels

15eb743

Reset _originallyInArchive and _offsetOfLocalHeader to private members, exposed as internal properties. Also changed _versionToExtract to be private - this isn't ever used in System.IO.Compression outside of ZipArchiveEntry.

ericstj assigned carlossanlop Sep 30, 2024

carlossanlop added this to the 10.0.0 milestone Sep 30, 2024

steveharter reviewed Oct 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce memory usage when updating ZipArchives #102704

Reduce memory usage when updating ZipArchives #102704

edwardneal commented May 26, 2024 •

edited

Loading

ericstj left a comment

ericstj commented Jul 22, 2024

udlose commented Oct 8, 2024 •

edited

Loading

ericstj commented Oct 9, 2024

edwardneal commented Oct 10, 2024

steveharter commented Oct 17, 2024

steveharter Oct 17, 2024

steveharter Oct 17, 2024

steveharter Oct 17, 2024

steveharter Oct 17, 2024

steveharter Oct 17, 2024

steveharter Oct 17, 2024

	_archiveStreamOwner.WriteAndFinishLocalEntry(true);
	_archiveStreamOwner.WriteAndFinishLocalEntry(forceWrite: true);

		@@ -31,6 +31,8 @@ public class ZipArchive : IDisposable
		private byte[] _archiveComment;
		private Encoding? _entryNameAndCommentEncoding;

Reduce memory usage when updating ZipArchives #102704

Are you sure you want to change the base?

Reduce memory usage when updating ZipArchives #102704

Conversation

edwardneal commented May 26, 2024 • edited Loading

ericstj left a comment

Choose a reason for hiding this comment

ericstj commented Jul 22, 2024

udlose commented Oct 8, 2024 • edited Loading

ericstj commented Oct 9, 2024

edwardneal commented Oct 10, 2024

steveharter commented Oct 17, 2024

steveharter Oct 17, 2024

Choose a reason for hiding this comment

steveharter Oct 17, 2024

Choose a reason for hiding this comment

steveharter Oct 17, 2024

Choose a reason for hiding this comment

steveharter Oct 17, 2024

Choose a reason for hiding this comment

steveharter Oct 17, 2024

Choose a reason for hiding this comment

steveharter Oct 17, 2024

Choose a reason for hiding this comment

edwardneal commented May 26, 2024 •

edited

Loading

udlose commented Oct 8, 2024 •

edited

Loading