Initial draft PR to establish igzip integration branch for ongoing IGZIP fixes and improvements

[asonje]

Summary

Initial draft PR to establish igzip integration branch for ongoing IGZIP fixes and improvements before merge to main.

Scope (Initial Push)

Brings current igzip branch baseline into review visibility.
Includes integration of igzip shim logic as well as functional tests.
Serves as the collaboration branch for follow-up PRs targeting igzip.

Purpose

Provide a shared upstream branch for iterative development, validation, and review.

Tracking

References #46

[Copilot]

Pull request overview

This PR brings an initial ISA-L/igzip integration baseline into the main review flow, adding IGZIP as an additional execution path and enabling it as an intermediate fallback between hardware accelerators (IAA/QAT) and software zlib.

Changes:

• Add IGZIP as a selectable execution path for deflate/inflate, plus optional accelerator→IGZIP fallback controlled by config.
• Introduce ISA-L (IGZIP) build integration (CMake option, version gating) and expose new config options/statistics counters.
• Expand the test suite substantially with IGZIP-focused regression tests and new parameter combinations.

Reviewed changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
zlib_accel.h	Adds IGZIP to ExecutionPath enum for path reporting/testing.
zlib_accel.cpp	Integrates IGZIP into deflate/inflate routing, fallback logic, and stream lifecycle handling.
igzip.h	Declares the IGZIP shim API used by zlib_accel.cpp.
igzip.cpp	Implements ISA-L-backed compress/uncompress helpers and stream handling.
config/default_config	Adds default toggles for IGZIP enablement.
config/config.h	Adds config enums for IGZIP enablement and fallback behavior.
config/config.cpp	Wires parsing/defaults for IGZIP-related configuration options.
statistics.h	Adds IGZIP counters for deflate/inflate success/error stats.
statistics.cpp	Adds IGZIP stat name strings to match new counters.
tests/zlib_accel_test.cpp	Adds IGZIP coverage across parametrized tests + large regression test suite.
tests/CMakeLists.txt	Minor update to run target command (commented filter).
README.md	Documents new CMake options, ISA-L dependency, and igzip_fallback; updates IAA marker docs.
iaa.cpp	Updates comments and changes IAA decompressibility heuristics (512-byte gate) tied to new fallback approach.
sharded_map.h	Adds missing <mutex> include (supports locking types used in the header).
common.cmake	Adds USE_IGZIP, ISA-L version enforcement, and ISA-L link/include setup.
CMakeLists.txt	Adds igzip.cpp to the shared library build.
.gitignore	Updates ignored build/artifact paths.

---

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

[matt-welch]

Inline comments left where appropriate.

PR #53 Review: IGzip Integration

Adds ISA-L (igzip) as a third accelerator backend, wired into deflate / inflate / compress2 / uncompress2 / gzwrite / gzread, with IAA/QAT → IGZIP fallback and ~1600 lines of regression tests. Substantial, well-structured work. Findings below are what to address before merge.

---

Correctness

1. UncompressIGZIP returns 1 instead of Z_DATA_ERROR on ISA-L errors : Medium

igzip.cpp:424 initializes ret = 1 and only reassigns it for ISAL_DECOMP_OK, ISAL_END_INPUT, and ISAL_NEED_DICT. Any other decomp code (e.g. ISAL_INVALID_BLOCK, ISAL_INVALID_SYMBOL, ISAL_INVALID_LOOKBACK) leaves ret == 1: not a valid zlib return code, and the log message reads "inflate finished with error code 1".

Downstream consequence: IGZIPRunInflateAndSelectPathAction at igzip.cpp:365-366 explicitly branches on *ret == Z_DATA_ERROR to return IGZIP_INFLATE_PATH_FALLBACK_DATA_ERROR. Because UncompressIGZIP never returns Z_DATA_ERROR from a real ISA-L error, that branch is only reachable via the null-arg guards at lines 339/349 : the intended "raw trailer fallback" path is effectively dead code.

Fix: map ISAL_INVALID_BLOCK/ISAL_INVALID_SYMBOL/ISAL_INVALID_LOOKBACK/default → Z_DATA_ERROR in the UncompressIGZIP return-code switch.

2. compress2 and uncompress2 use different completeness heuristics: Low

zlib_accel.cpp:926 checks ret == 0 && input_len != sourceLen to detect partial compression. zlib_accel.cpp:1028 mirrors this for uncompress2 but checks ret == 0 && !end_of_stream. Both paths are single-shot (no retry loop), matching the IAA/QAT stateless behavior, that's fine, but the two checks are testing different things for the same class of failure. Consider unifying or at least adding a comment.

3. Stateless compress2 / uncompress2 do not honor IGZIP_FALLBACK: Low

Streaming deflate() / inflate() have IAA/QAT → IGZIP fallback gated on configs[IGZIP_FALLBACK] (zlib_accel.cpp:421, :736). The stateless paths (zlib_accel.cpp:900-932, :998-1034) pick exactly one accelerator via if/else-if and never retry with IGZIP on failure. If IAA is preferred but fails, the caller falls straight to zlib, silently skipping IGZIP even when IGZIP_FALLBACK=1. Either wire fallback into the stateless paths or document the divergence.

4. gzread / gzwrite reverse the accelerator preference: Low (pre-existing, but propagates)

• compress2 / uncompress2 / deflate / inflate: IAA first, then QAT, then IGZIP.
• GzwriteAcceleratorCompress (zlib_accel.cpp:1289) / GzreadAcceleratorUncompress (zlib_accel.cpp:1363): QAT first, then IAA, then IGZIP.

The QAT-first ordering pre-dates this PR (git blame confirms), but the PR wires IGZIP in as a third-choice fallback in both orderings without noting the inconsistency. Worth documenting or unifying.

---

Dead / Unused Code

5. crc32 / adler32 are dead code: Low (delete)

igzip.cpp:265-272 defines crc32() and adler32() at global scope, routed to ISA-L equivalents.

• Nothing in the project calls either function (grep-verified).
• They are not exported: the project builds with -fvisibility=hidden (common.cmake:82), and only zlib_accel.h has #pragma GCC visibility push(default). nm -D on the built .so shows no crc32/adler32 in the dynamic symbol table.
• zlib_accel.cpp does not load crc32/adler32 via dlsym: there are no orig_crc32/orig_adler32 pointers.

So there is no LD_PRELOAD shadowing hazard, but the functions are pure dead code. Delete them and the #include "crc.h" at igzip.cpp:11.

6. Unused struct typedefs in igzip.h: Low

• igzip.h:10-15: internal_state2 / inflate_state2 are declared but never referenced anywhere.
• igzip.h:17-22: deflate_state is declared but never referenced. Also, deflate_state is a well-known zlib-internal name; even if it were used, the collision would be worth avoiding.

Delete both.

7. Dead method = 0 assignments: Low

zlib_accel.cpp:401 and :428 set deflate_settings->method = 0 before calling InitCompressIGZIP. DeflateSettings::method is written in the constructor (line 190) and at these two sites, and read nowhere. Remove.

8. Dead parameter assignments in CompressIGZIP / UncompressIGZIP: Low

igzip.cpp:193-194:

input = isal_strm->next_in;
output = isal_strm->next_out;

and igzip.cpp:417-418:

input = isal_strm_inflate->next_in;
output = isal_strm_inflate->next_out;

Both assign to by-value function parameters that are never read after. Dead stores: delete.

9. IGZIPHandleActiveStreamNoInput return IGZIP_NO_INPUT_NOT_HANDLED is unreachable: Low

igzip.cpp:301 returns IGZIP_NO_INPUT_NOT_HANDLED only when strm->avail_in != 0 or on null args. The sole caller (zlib_accel.cpp:606) already guards strm->avail_in == 0 before calling, so that return value is effectively dead. Either simplify the function to bool / void or drop the caller guard.

10. Commented-out #define in header: Low

igzip.h:61: // #define Z_DEFAULT_COMPRESSION 6. Delete.

---

Style / Consistency

11. __LINE__ in log messages: Low

Log calls throughout igzip.cpp include __LINE__ as a positional argument, e.g. Log(..., "InitCompressIGZIP() Line ", __LINE__, ...). iaa.cpp, qat.cpp, and most of zlib_accel.cpp don't do this. Line numbers rot with every edit and the logs are noisier than the rest of the codebase. Match the existing style.

12. Hardcoded windowBits = 15 / 31 in stateless paths: Low

zlib_accel.cpp:917, :1018, :1316, :1390 (compress2, uncompress2, gzip accelerator helpers) pass bare 15 or 31 to InitCompressIGZIP / InitUncompressIGZIP. Values are correct (15 = zlib format, 31 = gzip format), but they should be named constants. Note: the same magic numbers already appear in the IAA/QAT branches (SupportedOptionsIAA(15, ...), CompressQAT(..., 31, ...), etc.): if cleaned up, do all three backends in one pass.

---

Test Coverage

• IGZIP tests are gated #ifdef USE_IGZIP and CI does not build with USE_IGZIP=ON (.github/workflows/* matrix builds the zlib-only path). Consider adding a CI job stub for when hardware/library is available, or at least a README note.
• compress2 / uncompress2 IGZIP paths are covered: ZlibCompressUtility2 / ZlibUncompressUtility2 (tests/zlib_accel_test.cpp:153-189) wrap them and are exercised by the parameterized matrix with IGZIP as one of the paths (tests/zlib_accel_test.cpp:299, :333). No gap here.

---

Summary

#	Severity	File:Line	Issue
1	Medium	igzip.cpp:424	Missing Z_DATA_ERROR mapping; makes raw-trailer fallback branch dead code
2	Low	zlib_accel.cpp:926, 1028	compress2 vs uncompress2 completeness checks diverge
3	Low	zlib_accel.cpp:900-932, 998-1034	Stateless paths ignore IGZIP_FALLBACK
4	Low	zlib_accel.cpp:1289, 1363	gz* paths reverse QAT/IAA preference vs other paths
5	Low	igzip.cpp:265-272, :11	crc32/adler32 and crc.h include are dead code
6	Low	igzip.h:10-15, :17-22	Unused inflate_state2 / deflate_state typedefs
7	Low	zlib_accel.cpp:401, :428	Dead deflate_settings->method = 0 writes
8	Low	igzip.cpp:193-194, :417-418	Dead parameter assignments
9	Low	igzip.cpp:301 / zlib_accel.cpp:606	IGZIP_NO_INPUT_NOT_HANDLED unreachable
10	Low	igzip.h:61	Commented-out #define
11	Low	igzip.cpp (throughout)	__LINE__ log style diverges
12	Low	zlib_accel.cpp:917, :1018, :1316, :1390	Magic 15/31 for windowBits

Blocking: #1 (silently disables the raw-trailer fallback). Everything else is cleanup and can be addressed in a follow-up if preferred.

[matt-welch]

compress2 and uncompress2 use different completeness heuristics — Low

zlib_accel.cpp:926 checks ret == 0 && input_len != sourceLen to detect partial compression. zlib_accel.cpp:1028 mirrors this for uncompress2 but checks ret == 0 && !end_of_stream. Both paths are single-shot (no retry loop), matching the IAA/QAT stateless behavior — that's fine — but the two checks are testing different things for the same class of failure. Consider unifying or at least adding a comment.

[matt-welch]

UncompressIGZIP returns 1 instead of Z_DATA_ERROR on ISA-L errors: Medium

igzip.cpp:424 initializes ret = 1 and only reassigns it for ISAL_DECOMP_OK, ISAL_END_INPUT, and ISAL_NEED_DICT. Any other decomp code (e.g. ISAL_INVALID_BLOCK, ISAL_INVALID_SYMBOL, ISAL_INVALID_LOOKBACK) leaves ret == 1: not a valid zlib return code, and the log message reads "inflate finished with error code 1".

Downstream consequence: IGZIPRunInflateAndSelectPathAction at igzip.cpp:365-366 explicitly branches on *ret == Z_DATA_ERROR to return IGZIP_INFLATE_PATH_FALLBACK_DATA_ERROR. Because UncompressIGZIP never returns Z_DATA_ERROR from a real ISA-L error, that branch is only reachable via the null-arg guards at lines 339/349: the intended "raw trailer fallback" path is effectively dead code.

Fix: map ISAL_INVALID_BLOCK/ISAL_INVALID_SYMBOL/ISAL_INVALID_LOOKBACK/default → Z_DATA_ERROR in the UncompressIGZIP return-code switch.

[matt-welch]

IGZIPHandleActiveStreamNoInput potential total_out double-counting

igzip.cpp:316 does strm->total_out += output_len after UncompressIGZIP returns. However, UncompressIGZIP already sets isal_strm_inflate->total_out = *total_out before calling isal_inflate, and inflate() in zlib_accel.cpp also increments strm->total_out at zlib_accel.cpp:787 after IGZIPHandleActiveStreamNoInput returns. Verify these update paths are mutually exclusive and total_out is not double-counted.

[matt-welch]

Unused struct typedefs in igzip.h — Low

• igzip.h:10-15 — internal_state2 / inflate_state2 are declared but never referenced anywhere.
• igzip.h:17-22 — deflate_state is declared but never referenced. Also, deflate_state is a well-known zlib-internal name; even if it were used, the collision would be worth avoiding.

Delete both.

[matt-welch]

Stateless compress2 / uncompress2 do not honor IGZIP_FALLBACK — Low

Streaming deflate() / inflate() have IAA/QAT → IGZIP fallback gated on configs[IGZIP_FALLBACK] (zlib_accel.cpp:421, :736). The stateless paths (zlib_accel.cpp:900-932, :998-1034) pick exactly one accelerator via if/else-if and never retry with IGZIP on failure. If IAA is preferred but fails, the caller falls straight to zlib, silently skipping IGZIP even when IGZIP_FALLBACK=1. Either wire fallback into the stateless paths or document the divergence.

[matt-welch]

crc32 / adler32 are dead code: Low (delete)

igzip.cpp:265-272 defines crc32() and adler32() at global scope, routed to ISA-L equivalents.

• Nothing in the project calls either function (grep-verified).
• They are not exported: the project builds with -fvisibility=hidden (common.cmake:82), and only zlib_accel.h has #pragma GCC visibility push(default). nm -D on the built .so shows no crc32/adler32 in the dynamic symbol table.
• zlib_accel.cpp does not load crc32/adler32 via dlsym: there are no orig_crc32/orig_adler32 pointers.

So there is no LD_PRELOAD shadowing hazard, but the functions are pure dead code. Delete them and the #include "crc.h" at igzip.cpp:11.

[matt-welch]

Dead method = 0 assignments — Low

zlib_accel.cpp:401 and :428 set deflate_settings->method = 0 before calling InitCompressIGZIP. DeflateSettings::method is written in the constructor (line 190) and at these two sites, and read nowhere. Remove.

[matt-welch]

Dead parameter assignments in CompressIGZIP / UncompressIGZIP: Low

igzip.cpp:193-194:

input = isal_strm->next_in;
output = isal_strm->next_out;

and igzip.cpp:417-418:

input = isal_strm_inflate->next_in;
output = isal_strm_inflate->next_out;

Both assign to by-value function parameters that are never read after. Dead stores: delete.

[matt-welch]

IGZIPHandleActiveStreamNoInput return IGZIP_NO_INPUT_NOT_HANDLED is unreachable: Low

igzip.cpp:301 returns IGZIP_NO_INPUT_NOT_HANDLED only when strm->avail_in != 0 or on null args. The sole caller (zlib_accel.cpp:606) already guards strm->avail_in == 0 before calling, so that return value is effectively dead. Either simplify the function to bool / void or drop the caller guard.

[matt-welch]

Commented-out #define in header — Low

igzip.h:61: // #define Z_DEFAULT_COMPRESSION 6. Delete.

[mulugetam]

Looks like there are a few bugs that predate this PR. I'll open a separate PR to address them.

[matt-welch]

PR #53 Review Summary — HEAD e814856

Actionable punch list from the second-pass review, verified against the current tree. More per-finding detail is available if needed.

Must fix

(3) — compress2() returns Z_OK on truncated output  ·  zlib_accel.cpp:929

The check input_len != sourceLen verifies only input consumption. If avail_out runs out while ISA-L emits the trailer, isal_deflate returns COMP_OK with all input consumed, so the check passes and compress2 reports success on a truncated stream. Mirror uncompress2()'s !end_of_stream guard:

if (ret == 0 && (input_len != sourceLen ||
                 !IsIGZIPDeflateFinished(isal_strm))) {
  ret = 1;
}

Empirically reproduced. 64 bytes of 'A' → correct compressed size 11 bytes; calling compress2() with destLen=10 returns Z_OK but the output fails uncompress() with Z_DATA_ERROR. Reproducer: IGZIPCompress2RegressionTest.Compress2ReturnsZOkForTruncatedOutput in tests/zlib_accel_test.cpp.

Should fix

(1) — gz->path = IGZIP set even when init failed  ·  zlib_accel.cpp:1338, 1413

Move the assignment inside the success else branch. Second-pass review originally tagged this "Must fix"; downgraded here — gz->path is only consulted as != ZLIB in outer gzwrite/gzread logic, so the effect is a benign retry, not state corruption.

(2) — implicit null tolerance in deflate() IGZIP init  ·  zlib_accel.cpp:401–413, 426–441

CompressIGZIP currently handles a null isal_strm in its own guard. Add an explicit skip to the zlib fallback path when InitCompressIGZIP() returns nullptr.

(4) — IGZIP_NO_INPUT_NOT_HANDLED is dead  ·  igzip.cpp:278, igzip.h:17, caller zlib_accel.cpp:610

The only path returning this value is a null-arg guard whose preconditions the caller already ensures. Simplify IGZIPHandleActiveStreamNoInput to a bool return and drop the caller's if (action == IGZIP_NO_INPUT_RETURN) check.

(5) — unreachable Z_STREAM_END log in CompressIGZIP  ·  igzip.cpp:200–202

ret is set to Z_OK (0) or Z_STREAM_ERROR (−2), never Z_STREAM_END (1). Delete the branch.

(6) — same unreachable branch in UncompressIGZIP  ·  igzip.cpp:411–413

ret is 0, Z_NEED_DICT (2), or Z_DATA_ERROR (−3), never Z_STREAM_END (1). Delete the branch.

Nice to have

(8) — unused strm / reason params  ·  zlib_accel.cpp:245–263

SetDeflatePath / SetInflatePath cast both parameters to (void). Either wire them into the log messages or drop from the signature.

(9) — magic 15 / 31 windowBits literals  ·  12 call sites in zlib_accel.cpp

Replace with named constants (kWindowBitsZlib = 15, kWindowBitsGzip = 31) across all three backends in one pass.

Skip

(7) — add SupportedOptionsIGZIP() predicate

Proposed as a stub returning true for symmetry with the IAA/QAT gates. A no-op stub adds no filtering; the existing explicit configs[USE_IGZIP_COMPRESS] check is clearer. Skip unless a real gate (min input size, unsupported-format detection) is defined.