IGZIP inflate correctness, statistics, and IAA→IGZIP fallback#54
Open
asonje wants to merge 6 commits intointel:igzipfrom
Open
IGZIP inflate correctness, statistics, and IAA→IGZIP fallback#54asonje wants to merge 6 commits intointel:igzipfrom
asonje wants to merge 6 commits intointel:igzipfrom
Conversation
ISAL's stateful inflate pre-loads input in 8-byte word chunks into a 64-bit shift register (read_in). After reaching ISAL_BLOCK_FINISH for raw deflate (window_bits < 0), read_in_length still holds the bit count of bytes fetched beyond the true stream end. Shifting right by 3 gives the exact over-consumed byte count for every avail_in scenario: avail_in in [1,7] : previous avail_in<8 heuristic approximated this avail_in == 0 : previous heuristic was blind; now handled avail_in == 8 : gap in both old heuristics; now handled avail_in > 8 : multi-frame continuation; now handled The corrected consumed count is reported back to the caller via *input_length so that avail_in / next_in on the zlib stream are advanced accurately, preventing downstream compressed-size and CRC mismatches (reproduces as ZipException in Java ZipInputStream). Boundary guard updated: skip the raw-boundary fallback when *tofixed==1 because the correction has already accounted for any remaining bytes, so non-zero remaining is legitimate trailing data rather than un-corrected over-consumption. BLOCK_INPUT_DONE path (output-buffer-limited mid-stream) retains the existing Z_DATA_ERROR->zlib-fallback behaviour unchanged. Previously attempted stateless-probe approach removed entirely; the read_in_length field is simpler, exact, and has no multi-frame failure mode. Validated: 8/8 IGZIPInflateRegressionTest pass; full make run passes with only the pre-existing ordering-sensitive case 37961; JAR repro (250 entries, scan + build-init) passes clean.
…emove stale post-reset pre-set
The old name 'trailer_overconsumption_fixed' was a holdover from the
avail_in < 8 heuristic era. Since EXP-6f the fix is driven entirely by
isal_inflate.read_in_length >> 3; the flag only signals that a correction
was applied *within the current inflate call*.
Rename the field, its parameter aliases, and all local variables to
read_in_correction_applied to match the actual semantics.
Also remove the stale block in inflateReset:
if (was_igzip_path) {
inflate_settings->trailer_overconsumption_fixed = 1;
}
This block predated the read_in_length fix. It pre-armed the flag so
that the old avail_in < 8 boundary guard would not fire on the first
call after a reset. That logic was correct under the old heuristic but
is actively wrong now: ResetUncompressIGZIP already zeros the flag;
re-arming it to 1 would suppress the boundary guard on exactly the call
where it is needed most (first call after reset on a raw stream).
Remove was_igzip_path (now unused after the block removal) and inline
the INPUT_DONE log expression to eliminate an unused-variable warning
when DEBUG_LOG=OFF.
No behaviour change for correct streams; test suite: 8/8 IGZIP inflate
regressions PASS, only pre-existing 37961 flaky failure remains.
…STICS build flag
Statistics previously tracked QAT and IAA paths but left IGZIP with
commented-out placeholders. Two problems with those placeholders: the
enum values DEFLATE_IGZIP_COUNT, DEFLATE_IGZIP_ERROR_COUNT,
INFLATE_IGZIP_COUNT, INFLATE_IGZIP_ERROR_COUNT did not exist, and the
stat_names array had no entries for them.
Add the four missing Statistic enum values (after the IAA entries,
before ZLIB, matching the QAT/IAA grouping pattern) and their
corresponding stat_names strings. Uncomment the INCREMENT_STAT calls
in the deflate and inflate IGZIP dispatch blocks. The error condition
mirrors IAA/QAT: unconditional IGZIP_COUNT on every dispatch, and
IGZIP_ERROR_COUNT when ret != 0 (fallback or hard error).
Enable ENABLE_STATISTICS=ON in cmake.txt so the counters compile in by
default.
Validation (OpenSearch opensearch-core-3.3.1.jar, 250-entry JAR repro,
use_igzip_uncompress=1, log_stats_samples=50):
Thread (main): inflate_count=750 igzip=749 igzip_errors=0 zlib=0
— 99.9% of inflate calls served by IGZIP with zero errors or
zlib fallbacks; one call is the initial probe before path selection.
When IAA inflate or deflate fails (ret != 0), and use_igzip_uncompress / use_igzip_compress is enabled, the new iaa_fallback_igzip config flag (default 0) routes the retry to IGZIP before allowing the call to fall through to software zlib. Motivation: IAA is stateless and leaves input unconsumed on failure, so IGZIP can retry from the same position at no extra cost. This keeps hardware-accelerated decompression in play for inputs that IAA rejects but IGZIP can handle (e.g. streams that fail IsIAADecompressible or exceed IAA's single-call constraint). Implementation: - New ConfigOption IAA_FALLBACK_IGZIP added between IGNORE_ZLIB_DICTIONARY and LOG_LEVEL; default 0 (opt-in), range [0,1]. - inflate(): after IAA dispatch block, if path_selected==IAA && ret!=0 && configs[IAA_FALLBACK_IGZIP] && igzip_available, reset input_len/output_len (IAA may have modified them on failure), clear read_in_correction_applied, and run IGZIPRunInflateAndSelectPathAction with the same path-action handling as the normal IGZIP dispatch. Falls through to zlib if IGZIP also fails. - deflate(): identical pattern; initialises isal_strm via InitCompressIGZIP if needed (matching normal IGZIP deflate path). - Both fallback blocks are wrapped in #ifdef USE_IGZIP and guarded by igzip_available so they compile away and are unreachable without IGZIP. - Existing INFLATE/DEFLATE_IGZIP_COUNT stats capture fallback calls. Test suite: 44756/44757 PASS (37961 pre-existing flaky only).
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.