CombinedStream and Managed Decompression Stream optimization#10
CombinedStream and Managed Decompression Stream optimization#10neon-nyan wants to merge 20 commits into
Conversation
| Method | Mean | Error | StdDev | Allocated | |--------- |---------:|--------:|--------:|-----------:| | BZip2New | 661.7 ms | 3.29 ms | 2.91 ms | 1.38 KB | | BZip2Old | 738.1 ms | 2.29 ms | 2.15 ms | 5481.11 KB |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
neon-nyan
changed the title
Also switching to bulk Fill instead of scalar
| int dicSize = MemoryMarshal.Read<int>(props.AsSpan(1)); | ||
| HDiffPatch.Event.PushLog($"[PatchCore::CreateLzmaStream] Assigning LZMA stream with dictionary size: {dicSize}", Verbosity.Verbose); | ||
| return new LzmaStream(props, rawStream, -1, -1, rawStream, false); | ||
| return new LzmaInputStream(props, rawStream, -1, -1, rawStream, false, true); |
There was a problem hiding this comment.
Bug: CreateLzmaStream passes the same stream as both inputStream and presetDictionary, causing _outWindow.Train() to advance the stream position, which corrupts the subsequent _rangeDecoder.Init() call.
Severity: CRITICAL
Suggested Fix
The presetDictionary should not be the same stream instance as the inputStream. The call to the LzmaInputStream constructor should be changed to pass null for the presetDictionary argument to prevent the _outWindow.Train() method from being called on the input stream, thus avoiding the stream position corruption. Alternatively, if a dictionary is needed, the stream's position must be reset between the _outWindow.Train() and _rangeDecoder.Init() calls.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: SharpHDiffPatch.Core/Binary/Compression/CompressionStreamHelper.cs#L131
Potential issue: When creating an LZMA stream (not LZMA2), `CreateLzmaStream` passes the
same `rawStream` object as both the `inputStream` and the `presetDictionary` to the
`LzmaInputStream` constructor. The constructor first calls `_outWindow.Train()` with the
dictionary stream, which seeks and reads from `rawStream`, advancing its position to the
end. Immediately after, `_rangeDecoder.Init()` is called with the same `rawStream`, but
its position is now incorrect. This causes the range decoder to initialize with invalid
data from the end of the stream, leading to decompression failure for all
LZMA-compressed patches.
Updated result: | Method | Mean | Error | StdDev | Gen0 | Gen1 | Allocated | |--------- |---------:|---------:|---------:|-------:|-------:|----------:| | Lzma2New | 22.28 us | 0.043 us | 0.038 us | - | - | 544 B | | Lzma2Old | 33.30 us | 0.142 us | 0.132 us | 0.9766 | 0.1221 | 49688 B |
| ReadOnlySpan<byte> sourceSpan = new(Unsafe.AsPointer(ref source), count); | ||
| Span<byte> destinationSpan = new(Unsafe.AsPointer(ref destination), count); | ||
|
|
There was a problem hiding this comment.
Bug: The CopyBytes method uses Unsafe.AsPointer on a managed array without pinning it, creating a risk of memory corruption if the garbage collector relocates the array during the operation.
Severity: LOW
Suggested Fix
Wrap the code that creates spans from pointers in a fixed block. This will pin the managed array in memory, ensuring the pointers remain valid for the duration of the CopyTo operation and preventing potential memory corruption from garbage collection.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: SharpHDiffPatch.Core/Binary/Compression/Lzma/LZ/LzOutWindow.cs#L146-L148
Potential issue: The `CopyBytes` method creates a `Span` from a pointer obtained via
`Unsafe.AsPointer` on a managed `byte[]` buffer. This is done without using a `fixed`
statement to pin the array in memory. This pattern violates .NET's memory safety
contract. If a garbage collection cycle with heap compaction were to occur during the
`CopyTo` operation, the underlying array could be moved. This would invalidate the
pointer and could lead to memory corruption or unpredictable behavior.
1 participant
What has been changed?
CombinedStream Hot-Path Optimization
This changes includes some optimizations on each Stream dependent position tracking and removes unused LINQ iteration while finding the index of the targeted Stream to Read or Write from. Also introducing the usage of
RandomAccess.Read/Writemethods to perform position-specific Read/Write operations without changing the base Stream's.Positionitself.Removing Unused Hashing Code and Code Cleanups
As per now, SharpHDiffPatch doesn't utilize hashing routine to check the integrity of the patch file. While the current implementation is capable of doing that, the usage of the routine is currently unused. Thus, we removed the
Adler64andCrc32hashing code. Some minor code quality changes are also included as well.BZip2InputStream Optimizations
BZip2InputStreamshould have performed better with much more reduced memory footprint, thanks to the changes on how it handles allocated buffers. The code now rent the buffers from the sharedArrayPoolinstead of allocating it within the instance itself.The benchmark shows massive 3971.8x reduction in memory allocation and performs 1.11x faster than the initial implementation from SharpZipLib.
LzmaInputStream Optimizations
Same attempt as other decompression stream, we have some optimization on LZMA2 decompression stream as well, resulting 91.33x reduction in memory allocation and performs 1.49x faster than the initial implementation from managed-lzma and sharpcompress
Sidenotes
These commit involves vibe-coded changes. However, the changes have been reviewed and benchmarked manually as to avoid any regression or bugs introduced by it.