Pluggable inflate backends, with ISA-L behind a feature - #10
Pluggable inflate backends, with ISA-L behind a feature#10BenjaminDEMAILLE wants to merge 32 commits into
Conversation
Every entry point required a `ReadAt` source, so gzip arriving on a pipe could not be decoded at all. Path 1 only ever moves forward through the compressed bytes, so extract the operations it uses into an internal `InputCursor` trait and implement it for both the positional `SourceCursor` and a new forward-only `StreamCursor`. Member framing, footer verification, trailing-garbage detection, and the output limit are then literally the same code for a stream as for a file, rather than a second implementation that could drift. Adds `Decoder::decode_stream` and `Decoder::stream_reader`, mirroring `Decoder::decode` and `Decoder::reader`. `Decoder::open` routes a path that cannot be read positionally to the streaming driver, which turns a call that failed before into one that succeeds. The CLI accepts `-` for standard input and follows the same routing. The streaming runtime is configured with one worker so the telemetry reports the concurrency actually in use. A streaming reader does not join its coordinator on drop, because that coordinator can be parked in a read against a producer that never writes again. No new dependency and no new unsafe code. Refs COMBINE-lab#6 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every case is fed through a source that implements only `Read`, so none of them can silently fall back to positional reads the way a `Cursor` would, and each is compared against the positional decode of the same bytes. Covers single-member, concatenated and empty members, BGZF with its EOF member, a fully stored stream, truncation inside DEFLATE and inside the footer, a corrupt CRC32 and a corrupt ISIZE naming the offending member, trailing garbage, the output limit, telemetry, `Box<dyn Read + Send>` through paraseq, dropping a reader against a stalled producer, and a slow producer writing in small chunks. On unix it also decodes a real `cat` pipe and checks that `Decoder::open` routes a FIFO to the streaming path while leaving a regular file on a parallel one. Refs COMBINE-lab#6 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Records what streaming input supports, what it does not, and why: the same verification as a regular file because it is the same sequential code, but no parallelism because every parallel path needs positional reads. ARCHITECTURE.md explains that the length snapshot is absent for a stream rather than mutable, that end of input plus the existing trailing-garbage check is what makes it authoritative, and why a streaming reader does not join on drop. SAFETY.md is unchanged; this feature added no unsafe code. Refs COMBINE-lab#6 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reading into an empty buffer tail returns `Ok(0)`, which the cursor would read as end of input and turn into silent truncation. Callers only refill a drained window so this is unreachable today, but the failure mode is bad enough to be worth closing. Refs COMBINE-lab#6 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
First of four sub-projects reimplementing PR COMBINE-lab#5 in reviewable pieces. Covers index construction as a by-product of decoding, the native, GZIDX, .gzi, and gztool on-disk formats, and a separate IndexedReader for random access. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Thirteen tasks from index types through the four on-disk formats, the IndexedReader, decode-path wiring, interop tests, and documentation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The wrapper keeps the crate's inflate-side unsafe in one place and now also accepts a predecessor window as a plain byte slice, which the indexed reader needs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Index building is opt-in through DecoderBuilder::build_index and arrives in DecodeReport::index. Member starts are offered by the sequential and streaming paths, which every source reaches. DecodeReport is no longer Copy because it can now carry an index. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Advanced SIMD arm always returns, which made the scalar fallback unreachable on AArch64, and the mask load had no safety comment. Both are invisible on x86_64, where CI runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The estimated-grid path offers each chunk start with its resolved predecessor window, which is where interior, non-byte-aligned resume points come from. BGZF blocks are independent members, so every non-empty block start is offered with no window, using each block's ISIZE footer for its decompressed offset. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six ignored tests exercise both directions for bgzip, indexed_gzip, and gztool, with a CI job that installs all three. The first run found a real bug: a checkpoint without a predecessor window is not necessarily a member start, since indexed_gzip records its first point at the DEFLATE start, so the reader now detects a gzip header instead of assuming one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
deflateBound takes a c_ulong, which is 32 bits on Windows, and the pinned gztool tag does not exist; the project's tags reach v1.8.2. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A single-stream loop over InputCursor covers both containers for positional and non-seekable sources alike. Format selection is explicit through DecoderBuilder::format, with auto-detection between gzip and zlib; raw DEFLATE has no header and must be requested. Raw streams accept an optional expected decompressed size, the only end-to-end check that format allows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both containers are a single DEFLATE stream, so the estimated-grid path handles them by starting at the format's DEFLATE offset and verifying the format's trailer where a gzip footer would go. Accounting keeps an Adler-32 for zlib instead of a CRC32. Index checkpoints record the DEFLATE start, as indexed_gzip does, so IndexedReader resumes there without change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The sequential gzip loop, the single-stream zlib and raw DEFLATE loop, and the BGZF block decoder now inflate through InflateBackend, so an alternative implementation plugs in without touching a call site. The marker/window path and IndexedReader keep the concrete zlib-rs inflater, because they resume at arbitrary bit offsets and need zlib's Z_BLOCK contract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The member loop still drove zlib-rs directly, so it was not actually pluggable. It now goes through the trait like the other whole-stream paths. Making that work exposed a hole in the abstraction: `RawInflater` kept inherent `new`, `reset`, and `message` alongside its trait methods, so every call site resolved to the inherent version and the trait was dead code. Those three now live only in the trait impl. What stays inherent is what the trait deliberately omits, `prime` and the two dictionary calls, which belong to the paths that resume mid-stream. Drop `set_dictionary` from the trait for the same reason: no whole-stream path installs a window, since each stream it inflates starts at its own beginning. Backends report DEFLATE errors without a stream position, having seen only the buffer they were handed. `at_bit_offset` lets the call site, which knows the position, fill it in.
`isal_backend.rs` implements the backend trait over ISA-L, so the whole-stream paths decode through it when the feature is on. The default build is untouched: `isal-sys` is an optional dependency and nothing else changed. ISA-L is linked, not vendored. `use-system-isal` and `shared` keep the build from compiling the library from source, which would need autotools and an assembler. Two details the C interface forces: `inflate_state` embeds a 64 KiB scratch buffer, so it is boxed rather than held inline. It is zeroed before `isal_inflate_init`, which sets every scalar but leaves the scratch buffers alone. ISA-L reads ahead into a bit buffer, so at the end of a stream it has taken input the stream does not own. Whole bytes still in that buffer are given back, otherwise the gzip footer and the next member start at the wrong offset. The concatenated-member and BGZF tests are what catch this. The full suite passes with `--features isal`, including the indexed-seek tests, which is the check that the bit-accurate paths still run on zlib-rs.
A job installing libisal-dev, then running clippy and the suite with the feature on. The default jobs cannot reach this code: the feature is off and the library is not vendored.
`benches/inflate_backend.rs` decodes a 16 MiB corpus single-threaded through the two pluggable paths reachable from a byte slice. The backend is a compile-time choice, so comparing means two runs against a criterion baseline; the benchmark identifiers stay backend-agnostic so those runs line up. The corpus carries pseudo-random fields. Verbatim-repeating text compresses into a few very long matches that both backends copy at memory speed, which hid the Huffman decoding entirely: an earlier draft measured 3.9 GiB/s and would have compared nothing. On Apple M-series against isa-l 2.32.1, ISA-L is 20% slower on both paths, well outside the noise. That is the finding, and it is recorded rather than buried. The CI job now also runs both benchmarks on x86-64, where ISA-L's assembly decoder is strongest and where the project has no local hardware to measure. The feature stays off by default either way. Documentation covers the split in ARCHITECTURE.md, the ISA-L unsafe argument in SAFETY.md, the measurement in PERFORMANCE_AUDIT.md, and how to enable and reproduce in README.md and the crate docs. SAFETY.md also gets a stale path corrected: the zlib wrapper moved to inflate.rs.
|
As you can see, I've now reviewed (modified and extended where appropriate) and merged in all of the other PRs. Thank you for those! They were all useful ideas and some of them led to very useful API updates and decisions, as well as the supporting features. However, for this PR, I don't think I want to include this in the build, even as an optional feature. The main reasons are that this would make the crate a non-Rust build (again, I understand it's just opt-in), and worse than that, ISA-L is quite a hairy C-related build (the standard build needs specific assembler goodies). This might be worthwhile if it provided a substantial performance improvement. However, in the benchmark here, as well as several others that I've done during development and the other PRs, the pure Rust based zlib-rs is at least as fast if not faster than ISA-L. So, I don't really see a reason to add this feature. It makes the building more complicated, adds more code to maintain, and doesn't provide a tangible benefit. If that changes in the future, we can revisit it. --Rob |
Sub-project 3 of 4 replacing #5. Stacked on #9, which stacks on #8. GitHub cannot base a cross-fork pull request on a fork branch, so this targets
mainand its diff contains both parents; review the top three commits only (Route the sequential gzip loop...,Add an ISA-L raw-inflate backend...,Cover the ISA-L backend in CI,Measure the inflate backends...). Merge #8 and #9 first.What this does
Raw inflate now sits behind one crate-internal trait,
InflateBackend. A type alias,ActiveInflater, picks the implementation at compile time: zlib-rs by default, ISA-L under the new off-by-defaultisalfeature.Only the paths that decode a whole stream from its start go through the alias:
inflatePrimeandZ_BLOCKIndexedReaderThe two on the right name
RawInflaterdirectly, so a backend that cannot express a bit-accurate resume is not reachable from them by construction rather than by convention. The trait carries no dictionary call for the same reason: every stream the pluggable paths inflate starts at its own beginning.The default build is unchanged: same dependencies, same behaviour, and since the alias is a concrete type, the same generated code.
Measurement
benches/inflate_backend.rsdecodes a 16 MiB semi-structured log corpus single-threaded through each backend, compared through criterion baselines.On Apple M-series against Homebrew isa-l 2.32.1:
ISA-L loses on both, consistently, p < 0.05, confidence intervals disjoint. That is the honest result on the hardware available here, and the feature stays off by default. The
isalCI job runs the same two benchmarks on x86-64, where ISA-L's assembly decoder is strongest and where this project has no local machine to measure; read its logs for that number.One benchmark note worth stating plainly: an earlier corpus of verbatim-repeating text measured 3.9 GiB/s and compared nothing, because both backends spend that workload copying a handful of very long matches at memory speed. The corpus now carries pseudo-random fields so the Huffman decoding is actually on the clock.
Two things ISA-L's interface forces
inflate_stateembeds a 64 KiB scratch buffer, so it is boxed rather than held inline.More importantly, ISA-L reads ahead into a bit buffer, so at the end of a stream it has consumed input the stream does not own. Whole bytes still in that buffer are given back, or the gzip footer and the next member would be read from the wrong offset. The concatenated-member and BGZF tests are what catch this.
Verification
--features isal, including the indexed-seek tests, which is the check that the bit-accurate paths still run on zlib-rs.libisal-devand runs clippy plus the suite with the feature on.Documentation
The split is in
ARCHITECTURE.md, the ISA-L unsafe argument inSAFETY.md, the measurement inPERFORMANCE_AUDIT.md, and how to enable and reproduce it inREADME.mdand the crate docs.SAFETY.mdalso gets a stale path corrected: the zlib wrapper moved toinflate.rs.🤖 Generated with Claude Code