Add extraction benchmarks and fix per-entry temp file spilling - #209
Open
gfs wants to merge 2 commits into
Open
Conversation
Issue #208 claims RecursiveExtractor is slow and blames the quine check. Adds a BenchmarkDotNet project that measures this rather than guessing, and fixes the real problem the measurements exposed. Benchmarks (RecursiveExtractor.Benchmarks) compare RecursiveExtractor against raw SharpCompress on identical in-memory archives, isolate the quine check and type detection so their share of runtime can be read directly, and spread the same payload across zip/tar/tar.gz/tar.bz2/7z. Findings on a 1000 entry x 4 KiB deflate zip: - IsQuine over all entries costs ~20 us with zero allocations, ~0.06% of extraction. AreIdentical compares Length and Name before any bytes. - Type detection is ~1%. - RecursiveExtractor adds ~10-20% over a raw reader doing the same buffering; the format library is the rest. - The container spans two orders of magnitude for the same payload. The measurements also exposed a real bug. StreamFactory treats a stream whose Length throws as "assume large" and returns a delete-on-close FileStream. Archive entry streams cannot seek, so this fired on every entry, writing each one to a temporary file. ZipExtractor's async path hit it directly and 7z/Rar/Ace/Arj/Arc/Xz hit it through FileEntry. Fixed in two places: - ZipExtractor now passes the entry size it already has, matching what the synchronous path has always done. - The unknown-length fallback now returns a SpillOverStream that buffers in memory and moves to a delete-on-close FileStream only if the content actually exceeds MemoryStreamCutoff, preserving the memory guard. ExtractAsync on a 1000 entry zip goes from 930 ms to 38 ms (24.8x), and 7z extraction from 320 ms to 53 ms (6.1x). Co-authored-by: Copilot App <[email protected]> Copilot-Session: 8aea0ccd-2ed2-4d36-ba1e-b69a21b4a9ef
The non-indexed entry scan walks local file headers with a forward-only reader. For entries written with a data descriptor the local header reports an uncompressed size of 0, so sizing the backing store from Entry.Size buffered arbitrarily large hidden content in memory no matter what MemoryStreamCutoff was set to. Measured: a 4 MB hidden entry landed in a MemoryStream under a 64 KB cutoff, on both the sync and async paths. Pass the entry stream instead and let SpillOverStream make the decision from the bytes actually observed. That keeps the guard intact without reintroducing a temporary file per entry, which is what the stream overload used to cost before SpillOverStream existed. The two cataloged ZipArchive call sites read from the central directory and keep using Entry.Size. Also: - Open the entry stream before reading Size on the sync cataloged path. SharpCompress refreshes Size from the local file header on open, so reading it first used the stale central directory value; the async path already had the safer ordering. - Dispose the spill target in SpillOverStream if the copy into it throws, and use MemoryStream.WriteTo rather than a chunked CopyTo. - Override the span and memory overloads on SpillOverStream. These are what CopyToAsync actually calls on modern targets; the base implementations routed to the byte[] overloads correctly but rented and copied through an ArrayPool buffer on every write. - Document that entries of unknown size are buffered in memory up to the cutoff, and that Parallel bounds peak usage at BatchSize * cutoff. Co-authored-by: Copilot App <[email protected]> Copilot-Session: 991fa3eb-2417-4a56-80b3-d56a7c87b9c0
This file contains hidden or 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
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.
Closes #208 (as measured, not as reported).
Issue #208 claims RecursiveExtractor is slow and blames
Extractor.AreIdenticaldoing a full content compare. I didn't want to guess, so this adds a BenchmarkDotNet project to measure it — and then fixes the real problem the measurements exposed, which turned out to be something else entirely.The benchmarks
New
RecursiveExtractor.Benchmarksproject. It compares RecursiveExtractor against raw SharpCompress on identical in-memory archives, isolates the quine check and type detection so their share of runtime can be read directly, and spreads the same payload across zip (stored and deflate), tar, tar.gz, tar.bz2 and 7z. All archives are generated at setup time bySyntheticArchives, so nothing depends on the test project's assets.dotnet run -c Release -f net8.0 --project RecursiveExtractor.Benchmarks -- --filter "*" --job shortFindings
On a 1000 entry × 4 KiB deflate zip (ShortRun, net8.0):
Extract(recurse)IsQuineover all 1000 entriesDetectFileTypeover all 1000 entriesAreIdenticalcomparesLengthandNamebefore it ever looks at bytes, so the full content compare only runs when an ancestor has both the same size and the same name. Removing it would save nothing and would lose the zip bomb protection that dependent scanners rely on.ExtractorOptions.Parallelalready gives ~1.8× onExtractToDirectory(500 entries: 421 ms → 245 ms), which covers the second half of the issue.The bug the benchmarks found
ExtractAsyncwas ~28× slower thanExtractfor zip.StreamFactory.GenerateAppropriateBackingStream(int?, Stream, int)readstargetStream.Lengthinside atry, and on any exception returns a delete-on-closeFileStream. That treats "I can't determine the size" as "assume it's huge". SharpCompress entry streams can't seek and always throw, so this fired on every entry, writing each one to a temporary file on disk.Two paths hit it:
ZipExtractor's async path passed the raw entry stream (3 call sites), while the sync path had always passedzipEntry.Size.entry.OpenEntryStream()straight intoFileEntry, landing in the same fallback from the constructor.Fix
ZipExtractorpasses the size it already has (zipEntry.Size/forwardReader.Entry.Size) — the value is literally on the precedingCheckResourceGovernorline.SpillOverStream: buffers in memory and moves to a delete-on-closeFileStreamonly if the content actually exceedsMemoryStreamCutoff. This keeps the memory guard intact, fixes every extractor at once, and needs no public API change.ExtractAsync, 1000 × 4 KiB zipExtractAsync, 100 × 4 KiB zipTesting
RecursiveExtractor.Tests/StreamFactoryTests.cscovering the memory/disk decision, the spill transition, content integrity across the transition, the async and byte-at-a-time write paths,SetLength, and the null-cutoff opt-out.Not addressed here
entry.OpenEntryStream()rather than anything in this repo — worth a separate look.Ratiocolumns are the reliable part.