Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refines the handling of uncompressed (Stored) files within the ZIP archive reading logic. By introducing a dedicated reader path for these files, it eliminates redundant buffering and decompression steps, leading to more efficient memory usage and faster data access. The changes are focused on optimizing resource consumption when dealing with files that do not require actual decompression.

Highlights

• Performance Optimization for Stored Files: Introduced a specialized ZipFileReader::Stored variant to bypass unnecessary BufReader and Decompressor allocations for files with CompressionMethod::Stored. This significantly reduces memory allocations and slightly improves read performance for uncompressed entries.
• Reduced Allocations: The change reduces allocations from 2.0 allocs/entry (8,632 bytes each) to 1.0 allocs/entry (256 bytes each) for by_index() operations on Stored files.
• Improved Read Speed: Benchmarking shows a 5.3% improvement in read speed for 1 MiB Stored files, from 106,657 ns/iter to 101,276 ns/iter.
• Debug Implementation: Added a Debug implementation for CryptoReader to support the derive(Debug) on ZipFileReader after the introduction of the new Stored variant.

Hi

Thanks for the MR

I'm curious, which tools do you use to get this result?

  by_index() allocations:
    before: 2.0 allocs/entry, 8,632 bytes alloc'd each
    after:  1.0 allocs/entry,   256 bytes alloc'd each

My harness is here: https://github.com/lovasoa/zip2/blob/reduce-allocations/examples%2Fmemory_bench_alloc.rs

/Gemini review

@gemini-code-assist Will this cause a performance regression when reading from a file?

Hi @Pr0methean,

That's a great question! The benchmark results provided in the pull request description show a 5.3% improvement in read speed for 1 MiB Stored files. This suggests that the changes should not cause a performance regression when reading from a file, and may even improve it in some cases.

However, it's important to consider that these benchmarks are just one data point, and performance may vary depending on the specific file being read, the hardware being used, and other factors. It would be helpful to run additional benchmarks on a wider variety of files and systems to get a more complete picture of the performance impact of these changes.

Additionally, it's worth noting that the primary goal of this pull request is to reduce memory allocations, which can have a positive impact on overall performance even if read speed is not significantly affected. The reduced allocations can lead to less garbage collection overhead and improved memory locality, which can improve performance in memory-constrained environments.

@gemini-code-assist Can this change increase the number of I/O syscalls?

Hi @Pr0methean,

That's another excellent question! Whether this change increases the number of I/O syscalls depends on a few factors:

• Buffering: The original code uses BufReader, which performs its own internal buffering. By removing BufReader for stored files, we might reduce buffering in some cases, potentially leading to more frequent, smaller reads from the underlying CryptoReader.
• Read Sizes: If the read sizes requested by the user are smaller than the internal buffer size of BufReader, then removing it could lead to more syscalls. Conversely, if the read sizes are larger, the impact might be negligible or even beneficial.
• Underlying Reader: The CryptoReader itself might perform buffering, depending on its implementation and the underlying reader it wraps. If CryptoReader already buffers efficiently, the impact of removing BufReader might be minimal.

To get a definitive answer, it would be necessary to:

1. Profile the code: Use profiling tools to measure the number of syscalls before and after the change, under various scenarios (different file sizes, read sizes, etc.).
2. Analyze CryptoReader: Examine the implementation of CryptoReader to understand its buffering behavior.

In summary, it's possible that this change could increase the number of I/O syscalls in certain situations, but it's not guaranteed. Profiling and further analysis are needed to determine the actual impact.