Skip to content

perf: single-pass hdr_value_at_percentiles (flat scan, +599%) - #140

Open
fcostaoliveira wants to merge 1 commit into
HdrHistogram:mainfrom
fcostaoliveira:perf/single-pass-value-at-percentiles
Open

perf: single-pass hdr_value_at_percentiles (flat scan, +599%)#140
fcostaoliveira wants to merge 1 commit into
HdrHistogram:mainfrom
fcostaoliveira:perf/single-pass-value-at-percentiles

Conversation

@fcostaoliveira

Copy link
Copy Markdown
Contributor

Summary

hdr_value_at_percentiles resolved the requested percentiles via a per-bucket hdr_iter_next
walk. When normalizing_index_offset == 0 (the common case), replace that with a single tight
prefix-sum scan over the flat counts[] array
— the index→value conversion runs only at the
crossings. Decoded/rotated histograms (normalizing_index_offset != 0) keep the offset-aware
iterator path, so behavior is unchanged there.

Percentiles must be ascending (unchanged contract); results are byte-identical.

Benchmark

Getting all 7 of {50,75,90,95,99,99.9,99.99} in one hdr_value_at_percentiles call, single core
(Intel Granite Rapids), same-session A/B on an identical workload (hdr_init(1, 3.6e9, 3), 1M
Fibonacci-spread values):

calls/sec µs/call
before (iterator) 12,357 80.9
this PR (single-pass) 86,403 11.6

+599% (7×). hdr_record_value and singular hdr_value_at_percentile are unchanged (controls flat).

Correctness

  • ctest green; ASan + UBSan clean.
  • Results byte-identical to the previous implementation (verified via a stable cross-sum over the
    7 percentiles, unchanged).
  • Offset-aware path preserved: only the normalizing_index_offset == 0 case uses the direct
    counts[] scan; decoded histograms still go through the normalizing iterator.
  • HDR_LOG_REQUIRED=DISABLED builds; no intrinsics (MSVC unaffected).

fcostaoliveira pushed a commit to redis-performance/hdr-agent-workspace that referenced this pull request Jul 2, 2026
…% (PR #140)

hdr_value_at_percentiles: flat counts[] scan (offset==0) replacing the per-bucket iterator;
offset-aware iterator fallback kept for decoded histograms. gnr1 same-session A/B: 12,357 ->
86,403 calls/sec (+599%, 7x; 80.9->11.6us). ctest/ASan/nolog green, batch==singular byte-identical,
MSVC-safe. Based off official upstream/main (18c7a32), independent of #138/#139.
PR HdrHistogram/HdrHistogram_c#140. Logged EXPERIMENTS/SUMMARY/RACE/README/memory.
@fcostaoliveira

Copy link
Copy Markdown
Contributor Author

⚠️ Conflict heads-up: this overlaps my open #137, which also rewrites hdr_value_at_percentiles (as a superset — it single-passes this function and replaces the AVX2 dispatch). Same base blob + same function ⇒ they'll textually conflict. Flagging so we don't carry two single-pass-batch PRs; happy to consolidate #137/#140 into one.

hdr_value_at_percentiles resolved percentiles via a per-bucket hdr_iter_next walk.
When normalizing_index_offset == 0 (the common case), replace it with one tight
prefix-sum scan over the flat counts[] array (index->value conversion only at
crossings). Decoded/rotated histograms (offset != 0) keep the offset-aware iterator
path. Percentiles must be ascending, as before; results are byte-identical.

Adds a regression test (test_value_at_percentiles_with_offset) that drives a rotated
histogram with normalizing_index_offset != 0 and asserts identical percentiles to the
unrotated one, so CI covers the offset-aware fallback.
@fcostaoliveira
fcostaoliveira force-pushed the perf/single-pass-value-at-percentiles branch from 7c8af3d to 18ab4ee Compare July 2, 2026 14:40
@fcostaoliveira

Copy link
Copy Markdown
Contributor Author

Added a regression test (test_value_at_percentiles_with_offset) that drives a histogram with normalizing_index_offset != 0 (rotated counts[], as a decoded/foreign histogram would have) and asserts its percentiles match the unrotated histogram — so CI now covers the offset-aware iterator fallback (the branch that regressed in #137). ALL TESTS PASSED (19).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant