iaa: stop offering a stream to IAA after a history-window rejection - #76
Open
asonje wants to merge 1 commit into
Open
iaa: stop offering a stream to IAA after a history-window rejection#76asonje wants to merge 1 commit into
asonje wants to merge 1 commit into
Conversation
IAA's decompressor has a fixed 4 kB history buffer. A stream produced by
zlib, whose window is up to 32 kB, therefore cannot be decoded at all:
QPL returns QPL_STS_BAD_DIST_ERR (217) as soon as it meets a match
distance above 4096. Today the shim has no way to tell that failure apart
from any other. Within one stream that costs nothing extra, because the
zlib fall-through pins the stream to zlib and nothing is resubmitted until
it is reset -- but the reset is exactly what the workload does. Lucene
reuses one Inflater and resets it once per stored-field document, so the
shim pays a device round trip per document, forever, and never learns.
This is not a corner case. In an OpenSearch/Lucene stored-fields read
workload over a zlib-written index, 99.96% of 7,397,239 IAA inflate
submissions came back with status 217, and the wasted work made the IAA
path measurably slower than plain software zlib -- the only accelerator
configuration in that campaign that lost to it.
Measured with this branch against its own base, built from one cmake line
and run in one session on one host over one restored index: the search
throughput ceiling goes from 11,487.8 to 11,739.4 ops/s, +2.19%. Traffic
to the device falls 92.0% per device-second -- 6,424 to 511 work-queue
requests, 9.02 MB to 0.72 MB -- with the mean bytes per request unchanged
at about 1,400, which is what whole submissions disappearing looks like.
At a load both builds absorb completely, so the delivered work is equal
and the cost is directly comparable, server CPU falls from 73.3% to 70.1%
and median service time from 2.97 to 2.64 ms.
This does not turn IAA inflate into a win on zlib-written data and is not
meant to. The 4 kB window is a property of the device and no bookkeeping
changes it. Unshimmed zlib measured 11,852.9 ops/s in the same session,
so the fixed path is still 0.96% below it; what remains is the shim's own
interposition plus the one submission per stream the fix has to spend to
learn the answer from the device. What the change buys is that the path
stops paying for work it cannot use: it closes most of a 3.08% regression
against plain zlib, and at equal delivered load it now costs the same CPU
as zlib rather than 3.2 points more.
The window is a property of whichever compressor produced the bytes, not
of the individual block, so the rejection is worth remembering:
* UncompressIAA() gains an optional out-parameter that distinguishes
QPL_STS_BAD_DIST_ERR from every other failure. Other statuses stay
lumped together, since only this one predicts the next call.
* Per stream, a flag on InflateSettings suppresses further IAA
submissions. It is deliberately NOT cleared by inflateReset(): a
reset begins a new stream from the same producer, and Lucene resets
its Inflater once per stored-field document, so clearing it would
make the flag useless. ResetInflateStreamState() carries a comment
saying so, next to the fields that are cleared there.
* InflateStreamSettings::SetFromCopy() copies the flag explicitly. It
copies field by field rather than by value, so a new member is
otherwise silently dropped and a stream from inflateCopy() would go
back to submitting. A copy shares the producer, so it inherits the
verdict.
Two other inflate entry points need nothing. uncompress2() reaches
IsIAADecompressible() with zlib-format window bits, where the real window
is read out of the two-byte header: that answer is authoritative, nothing
is guessed, and there is no waste to remove. gzread() already pins a file
to zlib for good on its first accelerator failure of any kind, and
nothing clears that for the life of the GzipFile, so one submission per
file is all that path can spend and there is no second one to suppress.
gzread() carries a comment saying so; uncompress2() leans on the rationale
already recorded in IsIAADecompressible().
The remaining exposure is raw deflate and gzip, the two formats where
IsIAADecompressible() has no header to read and is guessing from input
length alone. Raw deflate is what Lucene uses.
The bet is one-directional. Declining to offload can only cost
throughput, never correctness, and the fallback path is the one that was
already producing every byte of output.
Tests: four new cases in IAAWindowRejectionTest. One calls UncompressIAA
directly on the software QPL path, so it runs without a device and pins
the out-parameter contract in both directions -- set on a 32 kB-window
stream, left alone on a 12 kB one, and safely omitted. Three go through
the public API and assert what the fix depends on: the flag survives
inflateReset(), it propagates through inflateCopy(), and it stays
per-stream, with a second stream still served by IAA afterwards. Those
three need a working device to produce a 217 at all and skip with a
message without one. Both traps were checked by mutation: clearing the
flag in ResetInflateStreamState() or dropping the SetFromCopy() line
fails the matching test.
A unit-level probe that replays Lucene's call shape -- 480 raw-deflate
streams of period 20000, each inflated whole through one reused z_stream
with inflateReset() between them -- goes from 480 rejected submissions to
1, with 480/480 byte-correct output in both arms. The one that remains is
the fix working as designed: it has to learn the answer from the device
once before it can stop asking.
The existing suite is unchanged: 12,836 checks with USE_IAA=ON, 0
failures, and the same skip list before and after; the only new results
are the four above.
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Persisted rejection state can incorrectly reject valid reset streams or unnecessarily disable IAA after narrowing the window.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds per-stream tracking to avoid repeatedly submitting IAA-incompatible deflate streams.
Changes:
- Reports IAA history-window rejections separately.
- Persists rejection state across resets and stream copies.
- Adds hardware and software-path regression tests.
File summaries
| File | Description |
|---|---|
zlib_accel.h |
Exposes rejection-state test accessor. |
zlib_accel.cpp |
Tracks and applies per-stream IAA rejection state. |
iaa.h |
Extends the decompression API with an optional result parameter. |
iaa.cpp |
Detects QPL_STS_BAD_DIST_ERR. |
tests/zlib_accel_test.cpp |
Tests detection, reset persistence, copying, and isolation. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
1307
to
+1308
| iaa_available = configs[USE_IAA_UNCOMPRESS] && | ||
| !inflate_settings->iaa_window_too_large && |
Comment on lines
+533
to
+535
| // iaa_window_too_large deliberately does NOT belong here. It records what IAA | ||
| // said about the compressor that produced these bytes, and a reset stream is | ||
| // almost always the same caller decoding more output from the same producer -- |
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
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.
IAA's decompressor has a fixed 4 kB history buffer. A stream produced by zlib, whose window is up to 32 kB, therefore cannot be decoded at all: QPL returns QPL_STS_BAD_DIST_ERR (217) as soon as it meets a match distance above 4096. Today the shim has no way to tell that failure apart from any other. Within one stream that costs nothing extra, because the zlib fall-through pins the stream to zlib and nothing is resubmitted until it is reset but the reset is exactly what the workload does.
The window is a property of whichever compressor produced the bytes, not of the individual block, so the rejection is worth remembering:
UncompressIAA() gains an optional out-parameter that distinguishes QPL_STS_BAD_DIST_ERR from every other failure. Other statuses stay lumped together, since only this one predicts the next call.
Per stream, a flag on InflateSettings suppresses further IAA submissions. It is deliberately NOT cleared by inflateReset(): a reset begins a new stream from the same producer, so clearing it would make the flag useless. ResetInflateStreamState() carries a comment saying so, next to the fields that are cleared there.
InflateStreamSettings::SetFromCopy() copies the flag explicitly. It copies field by field rather than by value, so a new member is otherwise silently dropped and a stream from inflateCopy() would go back to submitting. A copy shares the producer, so it inherits the verdict.
Tests: four new cases in IAAWindowRejectionTest. One calls UncompressIAA directly on the software QPL path, so it runs without a device and pins the out-parameter contract in both directions -- set on a 32 kB-window stream, left alone on a 12 kB one, and safely omitted. Three go through the public API and assert what the fix depends on: the flag survives inflateReset(), it propagates through inflateCopy(), and it stays per-stream, with a second stream still served by IAA afterwards. Those three need a working device to produce a 217 at all and skip with a message without one. Both traps were checked by mutation: clearing the flag in ResetInflateStreamState() or dropping the SetFromCopy() line fails the matching test.
The existing suite is unchanged: 12,836 checks with USE_IAA=ON, 0 failures, and the same skip list before and after; the only new results are the four above.