perf: block-summed scalar percentile scan (non-AVX2 fallback) + offset-safe dispatch

[fcostaoliveira]

What

Replaces the scalar get_value_from_idx_up_to_count (the non-AVX2 percentile-scan fallback) with a block-summed scan, and makes the dispatch offset-safe.

Note: this supersedes the earlier version of this PR (which dropped the AVX2 dispatch entirely). A 3-arch head-to-head showed that was the wrong trade — see below — so this revision keeps AVX2 for x86 and only upgrades the scalar fallback. It composes with #138/#139 rather than competing with them.

Why

The scalar scan tested the cumulative count after every single add — a loop-carried dependency that serializes the walk. It runs on every non-x86 target (where the AVX2 dispatch compiles out) and for decoded/rotated histograms. The block-summed scan (BLK=4) sums a small fixed block, tests the running total once per block, and enters the precise per-element walk only for the block that crosses the target.

Separately, the AVX2 (and the old scalar) scan read counts[] directly, which is wrong when normalizing_index_offset != 0 (decoded / rotated histograms). The dispatcher now routes offset != 0 to the scalar scan, whose fallback uses the offset-aware counts_get_normalised accessor.

Results — hdr_percentile_bench best M queries/sec (identical sink, i.e. bit-identical results)

Host	baseline (main)	this PR	Δ
Intel Granite Rapids (x86)	0.24	0.24	unchanged (keeps AVX2)
AMD Zen 5 (x86)	0.42	0.42	unchanged (keeps AVX2)
ARM Neoverse-V2	0.12	0.26	+117%

x86 is untouched (still takes the AVX2 path) and this composes with the AVX2-widening PRs #138/#139 — on x86 those take the scan to ~+125% while non-x86 gets the block-summed scalar. The head-to-head that motivated this split (portable-only #137 gave only +7–12% on x86 vs AVX2's +83–125%, but +117% on ARM where AVX2 is absent) is why the two are kept orthogonal.

Correctness

ctest 5/5; ASan+UBSan clean (no real errors). For offset == 0 the block-sum is a straight prefix-sum equivalent to the old scalar (the percentile assertions pass); for offset != 0 it now uses the offset-aware accessor — strictly more correct than the previous direct-counts[] read. Unsigned block accumulation avoids signed-overflow UB on corrupted/fuzzed state.

🤖 Generated with Claude Code

[filipecosta90]

The force-push at 91fb98b3 drops a correctness fallback that hdr_value_at_percentiles had pre-PR. Flagging in case it's unintentional.

The regression

The old hdr_value_at_percentiles walked buckets via hdr_iter_next, which dereferences counts through counts_get_normalised (src/hdr_histogram.c:822) — i.e. honors h->normalizing_index_offset. The new single-pass code reads h->counts[idx] directly. When normalizing_index_offset != 0 the bucket layout is rotated in counts[], so the cumulative scan crosses the target at the wrong physical bucket and hdr_value_at_percentiles returns the wrong percentile values.

The single-percentile hdr_value_at_percentile already had this issue pre-PR (via get_value_from_idx_up_to_count), so that one is not a regression — it's pre-existing. The batched API is the one this PR newly breaks.

When it triggers

normalizing_index_offset is set non-zero only by the decode paths in hdr_histogram_log.c:540,642 (hdr_log_read / hdr_decode_compressed) when the wire format carries a non-zero offset. The C library encoder only emits what it sees (always 0 from hdr_init), so the trigger is realistic only for cross-language interop (e.g. decoding a histogram emitted by Java HdrHistogram after a shiftValuesLeft/Right) or other encoders. Corner-case in C-only deployments; load-bearing for anyone reading wire-format logs from polyglot pipelines.

Suggested fixes (any one)

1. Add an offset-aware fallback at the top of both scan functions — what c644509 (force-pushed away on this branch) did. Fast path stays identical; the HDR_UNLIKELY(h->normalizing_index_offset != 0) branch walks via counts_get_normalised. Preserves the perf claim.
2. Hoist normalizing_index_offset into the inner loop via counts_get_normalised unconditionally. Slower across the board; the compiler may or may not LICM-hoist the offset==0 check out of the loop.
3. Document the precondition on hdr_value_at_percentiles ("undefined behavior on histograms with non-zero normalizing_index_offset") and add an assert. Cheapest, but silently changes the contract.

Happy with any of these — option 1 is what I'd pick (preserves both correctness and perf). If you're OK with the change of contract, option 3 is fine too as long as it's documented in the header alongside the non-decreasing-percentiles note.

Side notes

• The running += block_sum made unconditional (with the new comment) is a nice tweak; compiler can schedule independently of the crossing-branch.
• The uint64_t cast hardening on the block sum (added during PR #134's review to guard signed-overflow UB on fuzzed/corrupted histograms) was also dropped in the force-push. Restoring is a one-liner — doesn't change valid-state behavior; just keeps the prior intent for fuzz robustness.
• HDR_UNLIKELY on the cold crossing branch matches the file's convention introduced by recent PRs. Not load-bearing.

[fcostaoliveira]

Good catch, and correct on all counts — the 91fb98b force-push was a mistake on my end; it dropped the offset-aware fallback (and the uint64 hardening). I've restored c644509, so the branch head is back to the version that addresses this. CI is green (15/15).

c644509 covers all three of your suggestions:

1. Offset-aware fallback (option 1) — HDR_UNLIKELY(h->normalizing_index_offset != 0) branch at the top of both get_value_from_idx_up_to_count and hdr_value_at_percentiles, walking via counts_get_normalised; the offset == 0 fast path is unchanged. Validated by hdr_histogram_log_test, which exercises non-zero offset through encode/decode.
2. uint64_t hardening restored on the block sum and crossing test.
3. HDR_UNLIKELY on the cold crossing branch.

On the running += block_sum side note: I benchmarked both forms on the regressing config (precision 4, max 86.4M) on Cascade Lake (Xeon 6248) — with HDR_UNLIKELY present the else and the unconditional form are statistically identical (~−9.5% vs base; the plain else without the hint was the one that regressed ~+11%). So the hint is doing the real work here. Happy to switch to the unconditional form if you prefer it for readability.

Cross-µarch numbers for the offset == 0 fast path (vs main 0.11.10, base = runtime-dispatched AVX2): single-percentile −8% to −10% (Ice Lake / Cascade Lake / Granite Rapids), batched hdr_value_at_percentiles 12×–16×.

[fcostaoliveira]

⚠️ Conflict heads-up (coordinating my own open PRs): this rewrites hdr_value_at_percentiles — the same function as #140 — and it removes the AVX2 dispatch in get_value_from_idx_up_to_count that #138 (widen) and #139 (prefetch) optimize. So #137 and #138/#139 are alternative directions for the read path, and #137 overlaps #140's batch work. These can't all merge cleanly; flagging so we can pick one direction rather than carry duplicates.

[github-actions]

🤖 Automated first-pass review — a human maintainer's review is still required before merge.

The offset-safe routing looks like the real substance here, more than the perf. Every other read path (hdr_count_at_index, the iterators) already goes through counts_get_normalised; the percentile scan was the one that didn't, and the new singular-vs-plural test does fail without this change. I couldn't tell from the diff alone whether that direct counts[] read predates the AVX2 work in #134 or arrived with it — worth establishing, since a released version being affected changes how this gets handled.

The same defect appears to survive two lines below the new clamp: hdr_reset_internal_counters (src/hdr_histogram.c:317) scans with counts_get_direct(h, i) and then passes the raw i to hdr_value_at_index, so min_value/max_value come out wrong for a decoded rotated log — and both decoders call it right after assigning the offset. Java's establishInternalTackingValues reads via the offset-aware getCountAtIndex as I read it, but I'd want that checked against the reference implementation rather than taken on my word.

The decode clamp is the highest-value change in the diff and the only one with no test: the added test pokes normalizing_index_offset directly with an in-range value, so nothing exercises the out-of-range path the clamp exists for. Decoding a log is the only way to get a non-zero offset in this port, so an encode/decode round-trip with an out-of-range offset would cover both the clamp and the scan. Worth a deliberate call, too, on whether silently reducing a malformed offset beats rejecting the log — mod-reduction does preserve the intended rotation semantics, so it's defensible, but it means a corrupt log now decodes as valid.

On the block-summed scalar: for offset == 0 the running total ends up identical whether or not a block triggers the precise walk, and the unsigned accumulation can only cause a spurious block hit, never a missed crossing, so I agree it's equivalent to the old loop. The +117% is on a platform ci.yml has no runner for — that's the normal situation here and the author's own measurement is the verification, but it does mean CI can't confirm the part of this PR that motivates it.

Last, scope: the perf change, the dispatch correctness fix, and the decoder bounds fix are three independent things. The latter two may be worth landing separately if they'd want to go out without the perf change attached.