From d619eadf74a01a008cc8735cb538d98eb906365c Mon Sep 17 00:00:00 2001 From: fcostaoliveira Date: Thu, 27 Aug 2026 15:54:15 +0100 Subject: [PATCH 1/4] perf: block-summed scalar percentile scan (non-AVX2 fallback) + offset-safe dispatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scalar get_value_from_idx_up_to_count path (used on every non-x86 target, where the AVX2 dispatch compiles out) tested the cumulative count after every single add — a loop-carried dependency that serializes the walk. Replace it with a block-summed scan (BLK=4): sum a small fixed block, test the running total once per block, and enter the precise per-element walk only for the block that crosses the target. On ARM Neoverse-V2 this raises hdr_value_at_percentile throughput from 0.12 to 0.26 M q/s (+117%); x86 is unchanged (still takes the AVX2 path). Also make the dispatch offset-safe: the AVX2 (and old scalar) scans read counts[] directly, which is wrong for a non-zero normalizing_index_offset (decoded/rotated histograms). Route offset != 0 to the scalar scan, whose fallback uses the offset-aware counts_get_normalised accessor. Composes with the AVX2-widening PRs (#138/#139): x86 keeps the widened SIMD scan, non-x86 gets the block-summed scalar. Co-Authored-By: Claude Opus 4.8 --- src/hdr_histogram.c | 57 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 52 insertions(+), 5 deletions(-) diff --git a/src/hdr_histogram.c b/src/hdr_histogram.c index 1ec32e6..188b6ca 100644 --- a/src/hdr_histogram.c +++ b/src/hdr_histogram.c @@ -711,12 +711,58 @@ int64_t hdr_min(const struct hdr_histogram* h) static int64_t get_value_from_idx_up_to_count_scalar( const struct hdr_histogram* h, int64_t count_at_percentile) { - int64_t count_to_idx = 0; - for (int32_t idx = 0; idx < h->counts_len; idx++) { - count_to_idx += h->counts[idx]; - if (count_to_idx >= count_at_percentile) + /* Block-summed scan: sum BLK counts, test the running total once per block, + and do the exact per-element walk only for the crossing block. offset != 0 + (decoded/rotated) reads via the offset-aware accessor. */ + enum { BLK = 4 }; + const int64_t* counts = h->counts; + const int32_t n = h->counts_len; + int32_t idx = 0; + int64_t running = 0; + + if (HDR_UNLIKELY(h->normalizing_index_offset != 0)) + { + for (idx = 0; idx < n; idx++) + { + running += counts_get_normalised(h, idx); + if (running >= count_at_percentile) + return hdr_value_at_index(h, idx); + } + return 0; + } + + { + const int32_t blk_limit = n - (n % BLK); + for (; idx < blk_limit; idx += BLK) + { + /* unsigned: avoid signed-overflow UB on corrupted state */ + uint64_t block_sum_u = 0; + int32_t j; + for (j = 0; j < BLK; j++) + block_sum_u += (uint64_t)counts[idx + j]; + if (HDR_UNLIKELY((uint64_t)running + block_sum_u >= (uint64_t)count_at_percentile)) + { + for (j = 0; j < BLK; j++) + { + running += counts[idx + j]; + if (running >= count_at_percentile) + return hdr_value_at_index(h, idx + j); + } + } + else + { + running += (int64_t)block_sum_u; + } + } + } + + for (; idx < n; idx++) + { + running += counts[idx]; + if (running >= count_at_percentile) return hdr_value_at_index(h, idx); } + return 0; } @@ -762,7 +808,8 @@ static int64_t get_value_from_idx_up_to_count(const struct hdr_histogram* h, int { count_at_percentile = count_at_percentile > 0 ? count_at_percentile : 1; #ifdef HDR_HAS_AVX2_DISPATCH - if (__builtin_cpu_supports("avx2")) + /* AVX2 reads counts[] directly; offset != 0 (rotated) must use the scalar scan */ + if (h->normalizing_index_offset == 0 && __builtin_cpu_supports("avx2")) return get_value_from_idx_up_to_count_avx2(h, count_at_percentile); #endif return get_value_from_idx_up_to_count_scalar(h, count_at_percentile); From 61b254cb0730a3569d04823d93aad711aa88c42e Mon Sep 17 00:00:00 2001 From: fcostaoliveira Date: Wed, 2 Sep 2026 12:31:45 +0100 Subject: [PATCH 2/4] test: pin singular==plural percentile under non-zero offset; fix overclaiming comment --- src/hdr_histogram.c | 2 +- test/hdr_histogram_test.c | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/hdr_histogram.c b/src/hdr_histogram.c index 188b6ca..806681d 100644 --- a/src/hdr_histogram.c +++ b/src/hdr_histogram.c @@ -735,7 +735,7 @@ static int64_t get_value_from_idx_up_to_count_scalar( const int32_t blk_limit = n - (n % BLK); for (; idx < blk_limit; idx += BLK) { - /* unsigned: avoid signed-overflow UB on corrupted state */ + /* unsigned block sum: cannot overflow under valid state (matches AVX2 path) */ uint64_t block_sum_u = 0; int32_t j; for (j = 0; j < BLK; j++) diff --git a/test/hdr_histogram_test.c b/test/hdr_histogram_test.c index ca9c855..7e645fa 100644 --- a/test/hdr_histogram_test.c +++ b/test/hdr_histogram_test.c @@ -239,6 +239,42 @@ static char* test_percentiles_by_value_at_percentiles(void) return 0; } +/* Singular and plural percentile APIs both scan counts via counts_get_normalised, so + they must locate the same crossing bucket for any normalizing_index_offset. p0 is + excluded: hdr_value_at_percentile special-cases it to lowest_equivalent_value while + the plural API returns highest_equivalent_value, a documented divergence unrelated to + the offset-aware scan. */ +static char* test_percentile_singular_equals_plural_with_offset(void) +{ + struct hdr_histogram* h = NULL; + double percentiles[5] = { 50.0, 90.0, 99.0, 99.9, 100.0 }; + int64_t values[5] = { 0 }; + int i; + + mu_assert("Failed to allocate hdr_histogram", hdr_init(1, INT64_C(3600000000), 3, &h) == 0); + + for (i = 0; i < 10000; i++) + { + hdr_record_value(h, 1000); + } + hdr_record_value(h, 100000000); + + /* Non-zero offset in [1, counts_len): both scans must still agree. */ + h->normalizing_index_offset = h->counts_len / 2; + + mu_assert("value_at_percentiles return should be 0", + hdr_value_at_percentiles(h, percentiles, values, 5) == 0); + + for (i = 0; i < 5; i++) + { + mu_assert("singular != plural under non-zero offset", + hdr_value_at_percentile(h, percentiles[i]) == values[i]); + } + + free(h); + return 0; +} + static char* test_recorded_values(void) { @@ -572,6 +608,7 @@ static struct mu_result all_tests(void) mu_run_test(test_get_max_value); mu_run_test(test_percentiles); mu_run_test(test_percentiles_by_value_at_percentiles); + mu_run_test(test_percentile_singular_equals_plural_with_offset); mu_run_test(test_recorded_values); mu_run_test(test_linear_values); mu_run_test(test_logarithmic_values); From fb66bbccfa03a85e657eacf20a44e22dbeab39ff Mon Sep 17 00:00:00 2001 From: fcostaoliveira Date: Wed, 2 Sep 2026 13:53:19 +0100 Subject: [PATCH 3/4] fix: clamp decoded normalizing_index_offset into (-counts_len, counts_len) An out-of-range offset from an untrusted log defeats normalize_index's single +/-counts_len wrap and indexes counts[] out of bounds on the offset-aware read paths (now including the percentile scan). Reduce it at decode; no-op for valid logs where |offset| < counts_len. --- src/hdr_histogram_log.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/hdr_histogram_log.c b/src/hdr_histogram_log.c index 39fb96b..6928866 100644 --- a/src/hdr_histogram_log.c +++ b/src/hdr_histogram_log.c @@ -538,6 +538,14 @@ static int hdr_decode_compressed_v1( apply_to_counts(h, word_size, counts_array, counts_limit); h->normalizing_index_offset = be32toh(encoding_flyweight.normalizing_index_offset); + /* Reduce a decoded offset into (-counts_len, counts_len): normalize_index applies + only a single +/-counts_len wrap, so an out-of-range offset (untrusted log input) + would index counts[] out of bounds on the offset-aware read paths. No-op for + valid logs, where |offset| < counts_len. */ + if (h->normalizing_index_offset != 0) + { + h->normalizing_index_offset %= h->counts_len; + } h->conversion_ratio = int64_bits_to_double(be64toh(encoding_flyweight.conversion_ratio_bits)); hdr_reset_internal_counters(h); @@ -640,6 +648,14 @@ static int hdr_decode_compressed_v2( } h->normalizing_index_offset = be32toh(encoding_flyweight.normalizing_index_offset); + /* Reduce a decoded offset into (-counts_len, counts_len): normalize_index applies + only a single +/-counts_len wrap, so an out-of-range offset (untrusted log input) + would index counts[] out of bounds on the offset-aware read paths. No-op for + valid logs, where |offset| < counts_len. */ + if (h->normalizing_index_offset != 0) + { + h->normalizing_index_offset %= h->counts_len; + } h->conversion_ratio = int64_bits_to_double(be64toh(encoding_flyweight.conversion_ratio_bits)); hdr_reset_internal_counters(h); From bb96956eae6633f1f4e69e8f1d793c12360f7609 Mon Sep 17 00:00:00 2001 From: fcostaoliveira Date: Tue, 15 Sep 2026 12:20:20 +0100 Subject: [PATCH 4/4] test: hdr_close instead of free in test_percentile_singular_equals_plural_with_offset hdr_init makes two allocations - the counts array and the histogram struct - so free(h) frees only the struct and leaks counts (188416 bytes here). hdr_close frees both, and is what the rest of this file already uses. The sanitizers job added in #145 enables LeakSanitizer, so this test-only leak now fails CI on this branch; it was written before that job existed. Co-Authored-By: Claude Opus 5 --- test/hdr_histogram_test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/hdr_histogram_test.c b/test/hdr_histogram_test.c index da5e27c..00c0211 100644 --- a/test/hdr_histogram_test.c +++ b/test/hdr_histogram_test.c @@ -309,7 +309,7 @@ static char* test_percentile_singular_equals_plural_with_offset(void) hdr_value_at_percentile(h, percentiles[i]) == values[i]); } - free(h); + hdr_close(h); return 0; }