IGZIP inflate correctness, statistics, and IAA→IGZIP fallback

[asonje]

Fix raw deflate over-consumption and clean up the correction flag (6a89390, 93f144e)

After reaching ISAL_BLOCK_FINISH for raw deflate, ISA-L's stateful inflate pre-loads input in 8-byte chunks, leaving read_in_length holding the exact bit count consumed beyond the true stream end. Shifting right by 3 gives the over-consumed byte count for all avail_in scenarios — including avail_in == 0, == 8, and multi-frame continuations that the previous avail_in < 8 heuristic missed entirely.

As part of the same fix, the internal flag tracking whether a correction was applied is renamed from trailer_overconsumption_fixed to read_in_correction_applied to reflect its actual semantics (a within-call flag, not a permanent fix marker).

Wire up IGZIP statistics (92eaf59)

Added DEFLATE_IGZIP_COUNT, DEFLATE_IGZIP_ERROR_COUNT, INFLATE_IGZIP_COUNT, and INFLATE_IGZIP_ERROR_COUNT stats; All four are now active.

iaa_fallback_igzip config flag (b48d118)

When IAA inflate or deflate fails, and iaa_fallback_igzip=1 is set (default off), the retry is routed to IGZIP before falling through to software zlib.

[Copilot]

Pull request overview

This PR improves the IGZIP accelerator integration by fixing raw-deflate inflate input over-consumption handling, wiring up IGZIP statistics, and adding an optional IAA→IGZIP fallback path before falling back to software zlib.

Changes:

• Fix IGZIP raw-deflate over-consumption handling and rename the correction flag to read_in_correction_applied.
• Add and activate IGZIP deflate/inflate stats counters and error counters.
• Add iaa_fallback_igzip config option and implement IAA→IGZIP retry logic for both deflate and inflate.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
zlib_accel.cpp	Implements IAA→IGZIP fallback, enables IGZIP stats, adjusts inflate no-input handling, and renames correction flag usage.
igzip.cpp	Updates IGZIP option gating behavior and implements new raw-deflate read-in correction logic.
igzip.h	Renames the correction flag parameter/field in public IGZIP wrapper APIs.
statistics.h	Adds new IGZIP statistics enum entries.
statistics.cpp	Adds IGZIP stat name strings to match new enum entries.
iaa.cpp	Updates IAA decompressibility gating to avoid known ZipInputStream-style over-consumption issues.
config/config.h	Adds IAA_FALLBACK_IGZIP config option.
config/config.cpp	Adds default and parsing support for iaa_fallback_igzip.
tests/zlib_accel_test.cpp	Relaxes deflate path assertions to allow IGZIP behavior changes.
README.md	Documents IGZIP build flags and ISA-L dependency.
cmake.txt	Adds a local build command line (likely unintended to commit).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

read_in_correction_applied is described as being scoped to the current inflate call, but UncompressIGZIP doesn't reset *read_in_correction_applied to 0 at the start of each invocation. This can leave stale state across calls and affect later guard logic; consider setting it to 0 on entry and only setting it to 1 when a non-zero rewind_adjust_bytes is applied.

[Copilot]

inflate_settings->read_in_correction_applied is passed into IGZIPRunInflateAndSelectPathAction without being reset beforehand (unlike the IAA→IGZIP fallback path). If this flag is intended to be per-call, stale values can leak across inflate() calls and change raw-boundary / INPUT_DONE guard behavior; consider resetting it to 0 before each IGZIPRunInflateAndSelectPathAction invocation.

[Copilot]

This file appears to be a local build command with hard-coded absolute paths (e.g., /home/sdp/...). Committing this into the repo is brittle and not generally useful; please remove it or replace with documented, parameterized build instructions in README (or a script) using placeholders/environment variables.

Suggested change

	cmake -DUSE_QAT=ON -DUSE_IAA=ON -DUSE_IGZIP=ON -DQPL_PATH=/home/sdp/qpl -DISAL_PATH=/home/sdp/isa-l/ -DDEBUG_LOG=OFF -DENABLE_STATISTICS=OFF -DCMAKE_BUILD_TYPE=Release ..
	# Example CMake configure command. Set QPL_PATH and ISAL_PATH for your local environment before running.
	cmake -DUSE_QAT=ON -DUSE_IAA=ON -DUSE_IGZIP=ON -DQPL_PATH="${QPL_PATH}" -DISAL_PATH="${ISAL_PATH}" -DDEBUG_LOG=OFF -DENABLE_STATISTICS=OFF -DCMAKE_BUILD_TYPE=Release ..

[Copilot]

The new IAA→IGZIP fallback behavior (controlled by IAA_FALLBACK_IGZIP) is introduced here but doesn't appear to have a regression test. Please add coverage that forces an IAA failure and asserts IGZIP is attempted before falling back to zlib when iaa_fallback_igzip=1 (and not attempted when it is 0).

[Copilot]

Potential null dereference: deflate_stream_settings.Get(strm) can return nullptr (e.g., deflateSetDictionary called without a prior deflateInit*/deflateInit2*). The new mid-stream rejection check dereferences deflate_settings->path unconditionally; please guard for deflate_settings == nullptr and return Z_STREAM_ERROR in that case (matching zlib behavior).

[matt-welch]

Some of these comments may be repetitive with the existing copilot comments, but here's my review:

Code Review: PR #54 - IGZIP inflate correctness, statistics, and IAA->IGZIP fallback

Summary

This PR delivers three improvements to the igzip backend:

1. Raw deflate over-consumption fix - Replaces an 8-byte-trailer heuristic with a precise read_in_length >> 3 calculation using ISA-L's internal shift register state
2. Statistics enablement - Activates 4 IGZIP performance counters (deflate/inflate counts + error counts)
3. IAA->IGZIP fallback - New iaa_fallback_igzip config option allows failed IAA operations to retry through IGZIP before reverting to software zlib

The changes span 11 files with ~230 additions and ~180 deletions. The net effect simplifies several code paths (removing guard logic from SupportedOptionsIGZIPCompress, SupportedOptionsIGZIPUncompress, and IGZIPShouldFallbackDeflate) and introduces a new fallback chain for both compress and decompress paths.

---

Issues to Address

1. cmake.txt should not be committed (new file)

A new file cmake.txt contains a single hardcoded build command with machine-specific absolute paths (/home/sdp/qpl, /home/sdp/isa-l/). This appears to be a local developer note accidentally included in the PR. It should be removed from the commit and either added to .gitignore or converted into parameterized instructions in the README or a helper script.

2. read_in_correction_applied is never reset at the start of UncompressIGZIP() (igzip.cpp)

The struct comment says "set when read_in_length correction was applied in the current inflate call", implying per-call scope. However, UncompressIGZIP() does not reset *read_in_correction_applied = 0 at the top. This means a stale 1 from a previous call persists into subsequent calls on the same stream, which suppresses:

• The ISAL_BLOCK_INPUT_DONE fallback guard at line ~393 (the *read_in_correction_applied == 0 check)
• The raw boundary guard in IGZIPRunInflateAndSelectPathAction() at line ~314

If the flag is intentionally per-stream-lifetime (set once, stays set), the comment and name should reflect that. If it's truly per-call, add *read_in_correction_applied = 0; at the entry of UncompressIGZIP(). Looking at inflateReset which does reset it to 0, the intent appears to be per-stream-session, not per-call. Either way, the documentation is misleading.

Also: In zlib_accel.cpp inflate(), the primary IGZIP path at ~line 730 calls IGZIPRunInflateAndSelectPathAction without resetting read_in_correction_applied beforehand, while the IAA->IGZIP fallback path at ~line 712 does reset it. This asymmetry looks like a bug - the primary path should also reset the flag before each call, or the semantics should be clearly documented as intentionally different.

3. Null pointer dereference risk in deflateSetDictionary (zlib_accel.cpp ~line 296)

DeflateSettings* deflate_settings = deflate_stream_settings.Get(strm);
// New code immediately dereferences:
if (deflate_settings->path != UNDEFINED && deflate_settings->path != ZLIB) {
    return Z_STREAM_ERROR;
}

deflate_stream_settings.Get(strm) can return nullptr if deflateSetDictionary is called without a prior deflateInit. The new mid-stream rejection check dereferences unconditionally. Add a null guard:

if (deflate_settings != nullptr &&
    deflate_settings->path != UNDEFINED && deflate_settings->path != ZLIB) {
    return Z_STREAM_ERROR;
}

4. No test coverage for the IAA->IGZIP fallback path

The iaa_fallback_igzip feature is a meaningful behavioral change but has zero test coverage. Since CI doesn't have IAA hardware, consider adding a unit test that:

• Mocks or forces an IAA failure condition (e.g., by configuring IAA compress but providing data that triggers IAA error)
• Asserts that IGZIP is attempted when iaa_fallback_igzip=1
• Asserts that IGZIP is NOT attempted when iaa_fallback_igzip=0

Even a structural test verifying the config plumbing would be valuable.

5. SupportedOptionsIGZIPCompress, SupportedOptionsIGZIPUncompress, and IGZIPShouldFallbackDeflate are now dead code

All three functions have had their logic entirely removed and now unconditionally return a fixed value:

• SupportedOptionsIGZIPCompress -> always true
• SupportedOptionsIGZIPUncompress -> always true
• IGZIPShouldFallbackDeflate -> always false

The function bodies are replaced with (void) casts on every parameter. Since these are called from the hot path in deflate() and inflate(), they should either be:

• Removed entirely (callers inlined/simplified), or
• Kept but with a comment explaining why the guards were removed and whether they're expected to regain logic in the future

Leaving dead function scaffolding with voided parameters adds confusion about whether the removal is intentional or WIP. The same applies to IsIGZIPSyncFlush and kIGZIPMinFinishOutputSize which are deleted outright - good, but the inconsistency with the other three is worth resolving.

---

Items to Discuss

6. Removal of 6 path assertions from regression tests

Six ASSERT_EQ(GetDeflateExecutionPath(&stream), ZLIB) checks are removed from:

• ResetMustNotStallSyncFlushOnSameStream
• SyncFlushThenFinishDoesNotStall (2 assertions)
• SyncFlushFirstBlockThenFinish (3 assertions)

This is a direct consequence of SupportedOptionsIGZIPCompress and IGZIPShouldFallbackDeflate now allowing all flushes through IGZIP (previously Z_SYNC_FLUSH etc. would pin the stream to ZLIB). The tests still verify correctness of the compress/decompress round-trip, but no longer verify execution path.

Question: Are these tests now expected to run on the IGZIP path? If so, consider adding ASSERT_EQ(GetDeflateExecutionPath(&stream), IGZIP) to validate the new expected behavior rather than simply removing the path assertions. Path assertions are valuable for catching regressions in routing logic.

7. IsIAADecompressible 512-byte magic number (iaa.cpp)

The new guard:

static constexpr uint32_t kZipInputStreamBufferSize = 512;
return input_length > kZipInputStreamBufferSize;

This completely removes the PrependedEmptyBlockPresent() detection and the IAA_PREPEND_EMPTY_BLOCK config handling. The 512-byte threshold is a heuristic tied to Java's ZipInputStream internal buffer size. Concerns:

• If a non-Java caller legitimately passes exactly 512 bytes of valid IAA-compressed data, it will be unnecessarily rejected
• The IAA_PREPEND_EMPTY_BLOCK config option is still in config.h and the config loading code, but is now dead code in iaa.cpp. It should either be cleaned up or documented as deprecated.
• The 512-byte value is a Java implementation detail that could change across JDK versions. A static constexpr with a clear name is good, but adding a brief comment about which JDK versions this was validated against would help future maintainers.

8. inflateReset behavioral change

The old code preserved trailer_overconsumption_fixed = 1 after reset if the stream was previously on the IGZIP path. The new code removes this preservation entirely. This means after inflateReset, the correction flag starts fresh at 0 regardless of prior path.

Is this intentional? The old behavior was presumably there to prevent re-triggering the over-consumption correction on a stream that had already been fixed once. With the new read_in_length-based approach, is this no longer needed?

9. SYNC_FLUSH no-progress detection placement (igzip.cpp ~line 200-210)

The new SYNC_FLUSH no-progress check:

if (isal_strm->avail_in == 0 && isal_strm->flush == SYNC_FLUSH &&
    isal_strm->end_of_stream == 0 &&
    isal_strm->internal_state.state == ZSTATE_NEW_HDR) {
    *output_length = 0;
    *input_length = 0;
    return 0;
}

This check runs before isal_deflate() is called (line ~246 in the new code). By short-circuiting before ISA-L processes the stream, it ensures no sync marker bytes are emitted when there's no pending data. This is correct for matching zlib semantics, but:

• Accessing isal_strm->internal_state.state directly couples to ISA-L internals. If ISA-L changes the state machine in a future version, this check will silently break. Consider documenting the ISA-L version this was validated against.
• The ZSTATE_NEW_HDR check assumes that "at the beginning of a new header" implies "no pending output". Is that always true, or are there edge cases where ISA-L is in ZSTATE_NEW_HDR but has buffered output not yet flushed?

---

Looks Good

• Statistics plumbing is clean and follows the existing QAT/IAA pattern exactly. The enum ordering in statistics.h correctly places IGZIP counts between IAA and ZLIB counts.
• Config plumbing for iaa_fallback_igzip follows established patterns (enum, default value, config file parsing).
• read_in_length >> 3 approach is a clear improvement over the old 8-byte-trailer heuristic. Using ISA-L's actual shift register state eliminates guesswork about how many bytes were pre-fetched.
• Rename tofixed/trailer_overconsumption_fixed -> read_in_correction_applied improves readability significantly.
• README updates are appropriate and consistent with the existing QAT/IAA documentation pattern.
• The IAA->IGZIP fallback architecture in zlib_accel.cpp is well-structured: it restores input/output lengths, resets state, and correctly handles all path actions from IGZIPRunInflateAndSelectPathAction.

---

Summary of Requested Changes

Priority	Item	Action
Must fix	cmake.txt committed with hardcoded paths	Remove from PR
Must fix	Null deref risk in deflateSetDictionary	Add null guard
Should fix	read_in_correction_applied stale state / reset asymmetry	Reset before primary IGZIP path, or clarify semantics
Should fix	Dead function bodies (SupportedOptionsIGZIP*, IGZIPShouldFallbackDeflate)	Remove or document intent
Should fix	Dead IAA_PREPEND_EMPTY_BLOCK config option	Clean up or deprecate
Should add	Test coverage for IAA->IGZIP fallback	Add config-based test
Suggest	Replace removed path assertions with updated expectations	Validate IGZIP path in regression tests
Suggest	Document ISA-L version for internal state access	Future-proof SYNC_FLUSH and read_in_length usage