perf: portable block-summed percentile scan (drops AVX2 dispatch) + single-pass hdr_value_at_percentiles

[fcostaoliveira]

Summary

• Replace the runtime-dispatched AVX2 scan in get_value_from_idx_up_to_count (the _scalar + _avx2 + __builtin_cpu_supports("avx2") dispatcher) with a single portable block-summed scan: sum a fixed block of BLK = 4 counts, then test the running total against the target only once per block; the precise per-element walk runs only for the block that crosses the target. The win is the drop in early-exit branch frequency (one-per-block instead of one-per-element); the block sum is also a plain reduction the compiler can vectorize at the baseline ISA. This removes <immintrin.h>, the __attribute__((target("avx2"))) function, the HDR_HAS_AVX2_DISPATCH machinery and the per-call __builtin_cpu_supports check — and is faster than the AVX2 path it replaces on every Intel CPU tested.
• Rewrite hdr_value_at_percentiles to resolve all requested percentiles in a single ascending pass over counts[], replacing the per-call hdr_iter_next() walk (heavy per-bucket bookkeeping). Same block-summed shape; same non-decreasing-percentiles requirement as before (now documented in the header).
• No public API change, no new compile flags. The scan is memory-bandwidth bound, so the gain comes from branch-frequency reduction, not vector width (-march=native adds nothing).

Benchmark

test/hdr_histogram_benchmark, ns/op (lower is better), median of 8 reps on an isolated core. Base = main (0.11.10, runtime-dispatched AVX2). precision = 4; precision = 3 shows the same pattern. Cross-validated on three Intel microarchitectures.

hdr_value_at_percentile (single percentile)

microarch	Base	This PR	Delta
Ice Lake (Xeon 8360Y)	16507	14703	-10.9%
Cascade Lake (Xeon 6248)	14303	12861	-10.1%
Granite Rapids (Xeon 6972P) †	16153	14887	-7.8%

hdr_value_at_percentiles (batched: p50/p95/p99/p99.9 in one call)

microarch	Base	This PR	Delta
Ice Lake (Xeon 8360Y)	711850	59567	-91.6% (11.9x)
Cascade Lake (Xeon 6248)	682850	48383	-92.9% (14.1x)
Granite Rapids (Xeon 6972P) †	414545	25862	-93.8% (16.0x)

† Granite Rapids test box has no CMake; measured with an equivalent standalone driver compiling the same translation unit (-O3), base vs PR.

Correctness

• Full hdr_histogram_test suite passes.
• The rewritten hdr_value_at_percentiles is behaviorally equivalent to the previous iterator-based implementation. For percentiles in (0, 100] it also matches hdr_value_at_percentile called once per percentile: across 2000 random histograms × random sorted percentile sets (6956 checks), plus the empty-histogram case, the outputs are identical — 0 mismatches.
• Edge note: at exactly p = 0.0 the batched API returns the highest-equivalent value, as it always has; the single API returns the lowest-equivalent value there. This divergence predates and is unchanged by this PR.

Steps to reproduce

# Build baseline (main)
git clone https://github.com/HdrHistogram/HdrHistogram_c.git hdr_baseline
cd hdr_baseline && cmake -S . -B build -DCMAKE_BUILD_TYPE=Release \
  -DHDR_HISTOGRAM_BUILD_PROGRAMS=ON -DBUILD_TESTING=ON -DHDR_HISTOGRAM_BUILD_BENCHMARK=ON
cmake --build build --target hdr_histogram_benchmark -j$(nproc)
./build/test/hdr_histogram_benchmark --benchmark_filter=percentile

# Build this PR
cd .. && git clone -b perf-portable-percentile-scan \
  https://github.com/fcostaoliveira/HdrHistogram_c.git hdr_pr
cd hdr_pr && cmake -S . -B build -DCMAKE_BUILD_TYPE=Release \
  -DHDR_HISTOGRAM_BUILD_PROGRAMS=ON -DBUILD_TESTING=ON -DHDR_HISTOGRAM_BUILD_BENCHMARK=ON
cmake --build build --target hdr_histogram_benchmark -j$(nproc)
./build/test/hdr_histogram_benchmark --benchmark_filter=percentile

🤖 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.