Skip to content

Commit d2ddf39

Browse files
committed
fixes the cli failure case
1 parent 69e1cee commit d2ddf39

9 files changed

Lines changed: 192 additions & 8 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,9 @@ Version numbers follow [Semantic Versioning](https://semver.org/).
1010
## [Unreleased] — 2.0.0
1111

1212
### Fixed
13+
- **`setBklen()` rounded to an even value, which is only sufficient for a 2-byte symbol; `uint8_t` codebooks at a bklen not divisible by 4 silently decoded to wrong symbols.** The reverse codebook occupies `4*(2*32) + sizeof(T)*bklen` bytes and the bitstream begins immediately after it, so the bitstream is 4-aligned only when `sizeof(T)*bklen` is. Even-rounding satisfies that for `uint16_t` (the case it was written for) and `uint32_t` is aligned at any bklen, but a `uint8_t` book needs a multiple of **4**. At bklen 30 the bitstream landed at offset 2 mod 4, the decode kernel read it as `uint32*`, and a lossless round-trip returned symbol 0 where symbol 1 began — **reporting success**. Worse than wrong data: the misaligned access poisons the CUDA context, so every later allocation in the process fails with `cudaMalloc failed: misaligned address`, pointing nowhere near this stage (observed as 5 unrelated Huffman tests failing after the first). `setBklen()` now rounds up until `sizeof(T)*bklen % 4 == 0`. Not reachable through any shipped preset — they use bklen 256 and 1024, both aligned — so this was latent. `deserializeHeader()` takes the archive's bklen verbatim rather than rounding it (decode must reproduce the encoder's layout, so rounding there would describe a different codebook) and instead **throws** on an unaligned value, since such archives are malformed at the source. Test `HuffmanStage.PerBlockFallsBackToAdaptiveOnOverlongCode` uses bklen 30 deliberately and covers both this and the fallback below.
14+
- **A histogram that could not fit the 27-bit code field failed the whole compression instead of falling back.** `PerBlock` threw whenever some symbol needed a code wider than `HuffmanWord<4>`'s field. Failing loudly was right compared to the silent clamping it replaced, but it is the wrong answer for *data* — it cost the cuSZ preset every wide-dynamic-range field in the corpus (HACC/vy, HACC/xx, EXAFEL/data all hard-failed at multiple error bounds). The stage now falls back to an `Adaptive` (frequency-floored) book, which is **not** a relaxation of the error bound — the bound belongs to the quantizer, and flooring frequencies changes only how symbols are spelled. Measured: HACC/vy at 1e-2 gives CR 15.27x at 44.78 dB, HACC/xx at 1e-3 gives 13.79x at 64.77 dB, both with `max_abs_err` exactly equal to the bound, and both deterministic across repeated runs. `buildAdaptiveBook()` halves the floor shift until the book fits and shift 0 is uniform, so the fallback always terminates. The fallback is sticky per stage, logs one `FZ_LOG(WARN)`, and is queryable via `getAdaptiveFallbackUsed()`; the configured `getBookSource()` is left untouched so it still reports what was asked for. A **user-supplied** frequency table that cannot be coded is a configuration error and still throws (test HF30 unchanged).
15+
- **The CLI installed no log sink unless `-v` was passed, so every `FZ_LOG(WARN)` in the library went nowhere.** WARN is exactly the level used for "recoverable, but you need to know" — outlier drops, codebook fallbacks, deprecated-mode remapping — and none of it reached the user or the benchmark logs. This project has already paid for that twice: the outlier-overflow corruption was invisible for precisely this reason and had to be promoted to a `throw` to be seen at all. Stderr logging is now on at WARN by default; `-v`/`-vv`/`-vvv` raise it to INFO/DEBUG/TRACE as before. Turning it on immediately surfaced two pre-existing warnings that had never been visible in practice: the `Fine` encode falling back to the coarse path, and `MemoryPool` growing past its configured size.
1316
- **`QuantizerStage<double, …>` quantized at float32 precision, silently violating the error bound on f64 fields.** All four ABS/NOA forward kernels rounded with an unconditional `__float2int_rn(static_cast<float>(x) * static_cast<float>(ebx2_r))`, and the inverse reconstructed through a `float` — so `TInput = double` was nominal: the stage is templated and instantiated for `double`, but the arithmetic was never wider than f32. Any f64 field whose range is small next to its offset is unrepresentable that way. S3D/N2 spans 1.10e-5 about 0.7369, where the float32 spacing (5.96e-08) is **54x** the `abs_eb` a 1e-4 NOA bound asks for (1.103e-09) — the rounding error alone blew the bound before quantization did any work. Four cells (`cuszp2`/`cuszp3` x {plain, outlier}) round-tripped S3D/N2 at 48 dB with `status: ok` and `err_over_bound` **81.08 on all four**, the identical figure being the tell that the shared quantizer, not the coders, was at fault. Observed `max_abs_err` 8.941e-08 ~ 1.5x the float32 spacing, i.e. two roundings. Kernels now round via `quantRound<TInput>` (`__double2int_rn` for f64) and compare/reconstruct in `TInput`; `fabsf` on the threshold check was the same downcast and is gone. **Reachable only in `linear_mode`** — the radius-clamped path needs `|x|/abs_eb < 2*quant_radius` for an inlier, which forces `abs_eb` above the f32 spacing at `|x|`, so the radius check always escalates to an outlier first. That is why the cuSZp-style presets were hit and the others were not. The bound is also carried at full precision now: `computeValueBase()` returns `TInput` instead of narrowing the NOA range to `float`, and `QuantizerConfig` gained `abs_error_bound_f64` / `value_base_f64`, because decode computes `recon = q * 2*abs_eb` with `q ~ 3.3e8` — a 6e-08 *relative* error in a float `abs_eb` lands as 4.4e-08 absolute, 40x the bound. Old headers (<= 56 B) leave the new fields 0 and fall back to the float copies. **Throughput is unchanged**: the kernel is memory-bound, and a 64 Mi-element probe over f32/f64 x linear/radius x ABS/NOA moves every case by under 1.2%, within run-to-run noise (f64 linear NOA 0.4943 -> 0.4941 ms; f32 linear ABS 0.2275 -> 0.2271). Tests `QuantizerLinear.DoubleTinyRangeLargeOffsetRespectsBound` (fails at 61.4x over bound without the fix) and `QuantizerNOA.RefusesBoundBelowInputTypeSpacing`.
1417
- **`LorenzoQuantStage` narrowed a computed f64 error bound to float32 twice, violating the bound on relative-mode f64 fields.** Its quantization kernel was already correct (`round(input[i] * ebx2_r)` in `TInput`), but the bound reaching that kernel was not: `computeValueBase()` returned `float`, so `abs_eb = user_eb * value_base` was formed from a narrowed range, and `LorenzoQuantConfig::error_bound` then serialized the result as a `float`, so the decompressor read back a bound with ~6e-08 relative error. **Only reachable in a relative mode** — under `ABS` the bound comes straight from `config_.error_bound`, which is already a `float`, so the header narrowing is a no-op there and an ABS test passes vacuously. `cusz` on S3D/N2 (f64, values ~0.7369, range 1.10e-5) at a 1e-4 `rel_range` bound came back **22.73x over bound at 53.27 dB with `status: ok`**. Both narrowings are independently causal, isolated by reverting one at a time on the real cell: with neither fix 22.73x / 53.27 dB, with only the `TInput` value_base **6.62x / 64.98 dB**, with both **1.0x / 84.75 dB**. `computeValueBase()` now returns `TInput`, `computed_value_base_` is `TInput`, and the header carries `error_bound_f64` / `value_base_f64` (pre-2026-08-08 headers leave them 0 and fall back to the float copies). The v2-field check now compares against the literal 44-byte v2 size instead of `sizeof(LorenzoQuantConfig)`, which the new fields would otherwise have pushed past every existing archive. Test `LorenzoQuantHeader.DoubleNoaTinyRangeLargeOffsetRespectsBound` — a **file** round-trip, deliberately: an in-process compress/decompress reuses the live stage object and never reads the serialized header, so it cannot observe this at all. Fails at 11.9x over bound without the fix.
1518
- **Relative error-bound modes accepted bounds no implementation could satisfy.** NOA/PREL derive `abs_eb = eb * value_base`, which can land below the input type's own resolution at the data's magnitude — the input's round-trip error through `TInput` then exceeds the bound before any compression happens. `QuantizerStage` and `LorenzoQuantStage` now compare `abs_eb` against `epsilon(TInput) * max(|data|)` (the scan already had `max(|data|)`; `computeValueBase()` just discarded it) and throw rather than emitting a stream that decodes cleanly into out-of-bound values.

‎include/pipeline/compressor.h‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@
1616
#include <array>
1717
#include <memory>
1818
#include <stdexcept>
19+
#include <string>
1920
#include <unordered_map>
21+
#include <vector>
2022

2123
namespace fz {
2224

@@ -391,6 +393,34 @@ class Pipeline {
391393
bool isColoringEnabled() const { return dag_->isColoringEnabled(); }
392394
size_t getColorRegionCount() const { return dag_->getColorRegionCount(); }
393395

396+
/**
397+
* What coloring was *asked for*, as opposed to isColoringEnabled(), which
398+
* reports whether the DAG actually applied it. The two differ under
399+
* MINIMAL (nothing to color) and before finalize(). A benchmark row wants
400+
* the requested value, so a colored/uncolored pair can be identified even
401+
* when a topology gave coloring nothing to do.
402+
*/
403+
bool isColoringRequested() const { return coloring_enabled_; }
404+
405+
/**
406+
* Per-stage notes from the last run — see Stage::getRunNotes(). Keyed by
407+
* stage name; stages with nothing to report are omitted, so an empty map is
408+
* the normal case and means "nothing surprising happened."
409+
*
410+
* Exists so benchmark rows can record events that change how a result should
411+
* be compared (currently: a Huffman codebook fallback) rather than losing
412+
* them to a log line.
413+
*/
414+
std::unordered_map<std::string, std::vector<std::string>> collectRunNotes() const {
415+
std::unordered_map<std::string, std::vector<std::string>> notes;
416+
for (const auto& s : stages_) {
417+
if (!s) continue;
418+
auto n = s->getRunNotes();
419+
if (!n.empty()) notes.emplace(s->getName(), std::move(n));
420+
}
421+
return notes;
422+
}
423+
394424
// ── CUDA Graph Capture (compression-only) ─────────────────────────────────
395425

396426
/**

‎include/stage/stage.h‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,26 @@ class Stage {
178178
virtual void saveState() {}
179179
virtual void restoreState() {}
180180

181+
/**
182+
* Notes about *what this stage actually did* on the last run, when that
183+
* differs from what was configured in a way that affects comparability.
184+
*
185+
* Motivating case: `HuffmanStage` silently falls back to an Adaptive book
186+
* when a `PerBlock`/`Fixed` build drives a symbol past the 27-bit code
187+
* field. The fallback is correct — it does not relax the error bound — but
188+
* a field encoded with a different codebook is not compression-ratio
189+
* comparable to one that was not, and `getBookSource()` deliberately keeps
190+
* reporting what was *asked for*. Without a channel like this, a benchmark
191+
* row records the two cases identically and the difference is unrecoverable
192+
* after the fact.
193+
*
194+
* Returns short stable machine-readable tokens (e.g. `"adaptive_fallback"`),
195+
* not prose — these are meant to land in a benchmark row and be grouped on.
196+
* Empty by default; a stage that never surprises its caller need not
197+
* implement it.
198+
*/
199+
virtual std::vector<std::string> getRunNotes() const { return {}; }
200+
181201
/**
182202
* Called once by Pipeline::finalize() so stages can react to the dataset
183203
* dimensions set via Pipeline::setDims() after construction.

‎modules/coders/huffman/huffman_stage.h‎

Lines changed: 43 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -150,14 +150,25 @@ class HuffmanStage : public Stage {
150150
* Default: 256 for uint8_t, 1024 for uint16_t/uint32_t.
151151
*/
152152
/**
153-
* An **odd** bklen is rounded up to the next even value. The reverse
154-
* codebook occupies `4*(2*32) + sizeof(SYM)*bklen` bytes, so with a 2-byte
155-
* symbol an odd bklen makes it an odd number of 16-bit words; that shifts
156-
* the bitstream section to a 2-mod-4 offset, and the decode kernel reads
157-
* the bitstream as `uint32*` and faults on the misaligned load. Rounding up
158-
* is always safe -- a larger codebook only adds unused symbol slots.
153+
* bklen is rounded **up until `sizeof(T) * bklen` is a multiple of 4**.
154+
*
155+
* The reverse codebook occupies `4*(2*32) + sizeof(T)*bklen` bytes — a
156+
* 4-aligned 256-byte prefix plus the symbol table — and the bitstream begins
157+
* immediately after it. The decode kernel reads that bitstream as `uint32*`,
158+
* so the symbol table must not push it off a 4-byte boundary.
159+
*
160+
* This used to round odd up to even, which is only sufficient for a 2-byte
161+
* symbol. With `uint8_t` symbols the requirement is a multiple of **4**, and
162+
* a bklen of, say, 30 left the bitstream at offset 2 mod 4. That did not
163+
* fault — it decoded to **wrong symbols, silently**: an all-`uint8_t`
164+
* round-trip at bklen 30 returned symbol 0 where symbol 1 began and reported
165+
* success. `uint32_t` symbols are aligned at any bklen. Rounding up is always
166+
* safe — a larger codebook only adds unused symbol slots.
159167
*/
160-
void setBklen(uint32_t bklen) { bklen_ = bklen + (bklen & 1u); }
168+
void setBklen(uint32_t bklen) {
169+
constexpr uint32_t kMul = (4u / sizeof(T)) ? (4u / sizeof(T)) : 1u;
170+
bklen_ = ((bklen + kMul - 1u) / kMul) * kMul;
171+
}
161172
uint32_t getBklen() const { return bklen_; }
162173

163174
/**
@@ -292,6 +303,14 @@ class HuffmanStage : public Stage {
292303
*/
293304
bool getAdaptiveFallbackUsed() const { return adaptive_fallback_; }
294305

306+
/// Surfaces getAdaptiveFallbackUsed() to Pipeline::collectRunNotes(), so a
307+
/// benchmark row can tell a fallback-encoded field from a normally-encoded
308+
/// one. See Stage::getRunNotes().
309+
std::vector<std::string> getRunNotes() const override {
310+
if (adaptive_fallback_) return {"huffman_adaptive_fallback"};
311+
return {};
312+
}
313+
295314
/**
296315
* Refit trigger for `Adaptive`: rebuild the codebook once the encoded bit rate
297316
* degrades past `ratio` times the rate the book achieved when it was fitted.
@@ -465,6 +484,23 @@ class HuffmanStage : public Stage {
465484
if (size >= 3) {
466485
uint16_t bk;
467486
std::memcpy(&bk, buf + 1, sizeof(uint16_t));
487+
// Take the archive's bklen verbatim — NOT through setBklen(). Decode has
488+
// to reproduce the layout the encoder actually used, so rounding here
489+
// would describe a different reverse codebook than the one in the stream.
490+
//
491+
// But an archive written before setBklen enforced 4-byte alignment can
492+
// carry a bklen that puts the bitstream at a non-4 offset, and the decode
493+
// kernel reads it as uint32*. That does not merely decode wrong: it
494+
// poisons the CUDA context, and every later allocation in the process
495+
// fails with "misaligned address" — a failure that points nowhere near
496+
// this stage. Refuse it here, where the cause is still legible.
497+
if ((sizeof(T) * static_cast<size_t>(bk)) % 4u != 0)
498+
throw std::runtime_error(
499+
"HuffmanStage: archive declares bklen=" + std::to_string(bk) +
500+
" with a " + std::to_string(sizeof(T)) + "-byte symbol, which puts "
501+
"the bitstream at a non-4-byte offset. Such archives were written "
502+
"by a build predating the alignment fix and cannot be decoded "
503+
"correctly — the stream itself is malformed, not just this reader.");
468504
bklen_ = bk;
469505
}
470506
if (size >= 11)

‎src/pipeline/config.cpp‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1152,6 +1152,15 @@ void Pipeline::loadConfig(const std::string& path) {
11521152

11531153
if (auto v = (*pl)["num_streams"].as_integer())
11541154
setNumStreams(static_cast<int>(v->get()));
1155+
1156+
// Buffer coloring. Read *after* memory_strategy on purpose: setMemoryStrategy()
1157+
// rebuilds the DAG and re-applies the stored coloring flag, so setting coloring
1158+
// first would be re-applied rather than overwritten — but relying on that
1159+
// ordering silently is how this setting got lost once already (a plain
1160+
// setColoringEnabled(false) before loadConfig() was discarded, making the
1161+
// uncolored arm of the peak-memory sweep a no-op). Keep it last and explicit.
1162+
if (auto v = (*pl)["coloring"].as_boolean())
1163+
setColoringEnabled(v->get());
11551164
}
11561165

11571166
// ── Stage construction pass ───────────────────────────────────────────────
@@ -1249,6 +1258,9 @@ void Pipeline::saveConfig(const std::string& path) const {
12491258
out << "[pipeline]\n";
12501259
out << "input_size = " << static_cast<int64_t>(input_size_hint_) << "\n";
12511260
out << "memory_strategy = \"" << strategyToString(strategy_) << "\"\n";
1261+
// Only emitted when non-default, so round-tripping a normal pipeline does not
1262+
// grow a key nobody set. Round-trips through loadConfig()'s "coloring" reader.
1263+
if (!coloring_enabled_) out << "coloring = false\n";
12521264
out << std::setprecision(std::numeric_limits<double>::max_digits10);
12531265
out << "pool_multiplier = " << static_cast<double>(pool_multiplier_) << "\n";
12541266
out << "num_streams = " << static_cast<int64_t>(num_streams_) << "\n";

0 commit comments

Comments
 (0)