feat: add hdr_record_value_capped and hdr_total_count (memtier_benchmark compatible api)

[fcostaoliveira]

What

Three small additive APIs:

bool hdr_record_value_capped(struct hdr_histogram* h, int64_t value);
bool hdr_record_value_capped_atomic(struct hdr_histogram* h, int64_t value);
int64_t hdr_total_count(const struct hdr_histogram* h);

• hdr_record_value_capped clamps the value into [0, highest_trackable_value] and records it, so out-of-range samples saturate instead of being rejected. It delegates to hdr_record_value, so the write path itself is unchanged.
• hdr_record_value_capped_atomic is the same over hdr_record_value_atomic, safe to call from several threads.
• hdr_total_count returns the total recorded count, and 0 for a NULL histogram. It uses an atomic load, so it can be called while other threads use the *_atomic record functions.

Why

All three are already carried as local patches in memtier_benchmark, which records latencies from worker threads into a shared histogram while its main thread reads the total and resets or merges it. Upstreaming them lets that project use the library unmodified, and lets the amalgamated core in #164 match what it vendors.

The clamping is related to #126, which asks for out-of-range values to be capped. This PR does not change hdr_record_value, which still rejects them, so it does not close #126.

Behaviour, checked against the other implementations

memtier's version also raised 0 (and anything below lowest_discernible_value) up to lowest_discernible_value. I dropped that, because none of the other implementations do it. I ran the same probe (lowest 10, highest 1000, 3 significant figures) against each:

	0	3 (below lowest)	far above highest	negative
Java 2.2.2 recordValue	recorded	recorded	ArrayIndexOutOfBoundsException	ArrayIndexOutOfBoundsException
Python hdrh 0.10.3 record_value	recorded	recorded	False	False
Rust record	recorded	recorded	Err(ValueOutOfRangeResizeDisabled)	not representable (u64)
Rust saturating_record	recorded as 0	recorded	clamped to the highest value	not representable (u64)
C hdr_record_value	recorded	recorded	rejected	rejected

So only the top end is clamped here, like Rust's saturating_record. Negatives are clamped to 0, so the call cannot fail for any input.

One difference to be aware of: Java, Python and Rust enforce the size of the counts array, which is rounded up to a power of two, while C checks highest_trackable_value exactly. With the bounds above, 5000 is accepted by the other three but rejected by hdr_record_value here. That is existing behaviour and this PR does not change it.

The atomic load in hdr_total_count

The existing getters read total_count plainly. That is fine single-threaded, but this one is meant to be polled while writers run. With a plain read, ThreadSanitizer reports a data race between hdr_total_count and the atomic increment in counts_inc_normalised_atomic; with hdr_atomic_load_64 it reports none (the new concurrent test, run both ways). The result is identical single-threaded.

Tests / gates

• test_record_value_capped covers 0, a value below the lowest discernible value, in-range, above-range and negative input, and checks where each lands.
• test_record_value_capped_atomic checks the atomic variant produces the same histogram as the plain one, including INT64_MIN and INT64_MAX.
• test_recording_capped_concurrently runs two writers with a mix of in-range, above-range and negative values while the main thread polls hdr_total_count, then compares against a single-threaded reference. It checks the total never goes backwards and no record is lost.
• test_hdr_total_count covers empty, weighted records and NULL.
• ctest gcc and clang 9/9, ASan + UBSan 9/9, HDR_LOG_REQUIRED=DISABLED build 6/6 with no warnings. The concurrent test is also clean under ThreadSanitizer.
• Separately, a property check of capped(v) against record(clamp(v)) over 112 configurations (including a rotated histogram and INT64_MIN / INT64_MAX) found them byte-identical and never returned false.

No SOVERSION bump here, since that looks like a release-time decision (see the discussion on #165).

🤖 Generated with Claude Code

[github-actions]

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

Looks good to me. The change only adds API: struct hdr_histogram is untouched and no existing signatures change. Both capped variants just clamp and then call the existing record path, and the clamp itself can't overflow, including at INT64_MIN/INT64_MAX.

A few small things:

• SOVERSION: three new exported symbols means HDR_SOVERSION_CURRENT and _AGE both go up under the 4-step process in CMakeLists.txt. It's fine to leave that until release, as the PR says, but it shouldn't get lost.
• Commit history: the first commit's message still says the clamp is to [lowest_discernible_value, highest_trackable_value], which is no longer true. With the merge/revert/merge sequence on top, this is probably worth a squash-merge.
• Java behaviour: I couldn't check this independently. On bool hdr_record_value(struct hdr_histogram* h, int64_t value) can record out of bounds values and should be capped with min & max values #126 the open question was the lower bound, pending a comparison with Java. The probe table here says Java records 0 and sub-lowest values unchanged, and this PR matches that. Someone who knows the Java side may want to confirm before this is cited as settling the lower-bound half of bool hdr_record_value(struct hdr_histogram* h, int64_t value) can record out of bounds values and should be capped with min & max values #126.

[fcostaoliveira]

Thanks, agreed on all three.

• NULL handling: left as is. hdr_total_count is NULL-safe because that is what memtier's copy already does, so it can drop its patch unchanged. hdr_record_value_capped follows hdr_record_value, which also dereferences h without a check. Happy to add a NULL check to the new function if you would rather they were consistent.
• Commit message: the first commit's message is stale, as you say. The clamp was changed to [0, highest_trackable_value] in 4a74ae3, and the PR description is up to date. I would rather not force-push to reword it, so squashing on merge is the cleanest way to keep it out of history.
• SOVERSION: agreed, left for release time. The 0.12.0 bump will cover the new symbols from this PR, feat: add hdr_iter_linear_set_value_units_per_bucket (redis-benchmark / valkey-benchmark) #165 and fix: reject negative decoded bucket counts in V0/V1 logs #162 together.

[paulorsousa]

Looks good to me
Just noticed a possible situation on 32bit Windows (commented on the code), but this issue already exists in the codebase, so it may be worth fixing it on a separate PR
(update_min_max_atomic() and hdr_phaser_flip_phase() via _hdr_phaser_get_epoch())

[paulorsousa]

Maybe we have a portability issue here for 32-bit Windows
There, this 64-bit read may combine parts of different updates and return an incorrect count.
Microsoft documents this limitation here

[fcostaoliveira]

Confirmed, and thanks for catching it. In src/hdr_atomic.h the MSVC hdr_atomic_load_64 is _ReadBarrier(); return *field;, a plain read, and hdr_atomic_store_64 is the matching plain write. On 32-bit x86 a plain int64_t access is two 32-bit accesses, so a concurrent hdr_atomic_add_fetch_64 (the _InterlockedCompareExchange64 loop) can be observed half-applied. The GCC/Clang __atomic_load_n path is not affected.

So hdr_total_count inherits this from the helper rather than introducing it, but it does expose it through a public getter. The other users I can see on main are update_min_max_atomic (lines 143 and 154) and the phaser epoch (_hdr_phaser_get_epoch/_set_epoch), as you listed.

I have not reproduced a torn read and I have no 32-bit Windows machine to test on; the existing Windows x86 CI legs would not detect it either. The likely fix is in the helpers only: on !_WIN64 use _InterlockedCompareExchange64(field, 0, 0) for the load and the existing exchange for the store, leaving _WIN64 as is. I have not written it. Agreed that it belongs in a separate PR rather than here, since #166 is already merged.