Replace `hdr_hist` with `log_hist` #12460

ballard26 · 2023-07-26T03:15:58Z

This PR replaces all but one use-case of hdr_hist with log_hist. src/v/cluster/self_test can't be converted to log_hist yet as log_hist doesn't support generating quantiles.

This replacement reduces per-histogram memory usage from potentially hundreds kilobytes to 208 bytes. It also reduces the time to record a value to the histogram by about half.

Public metric histograms generated with;

ssx::metrics::report_default_histogram(hdr_hist);

are now replaced with;

log_hist.public_histogram_logform();

which both produce the same Prometheus histogram given that the same values were recorded to each. Hence it is a drop in replacement with no noticeable effects in the metrics.

The replacement for internal histograms, however, does not produce identical Prometheus histograms as what it replaces. In this case this;

hdr_hist.seastar_histogram_logform();

is now replaced with;

log_hist.internal_histogram_logform();

There are a couple differences between the two Prometheus histograms. The first is that the upper bound for the first bucket will be 8 instead of 10 now. The second is that buckets will now have upper bounds of 2^n -1 , hence comparing [replacement, current] the buckets are now [8, 10], [16, 20], [32, 41], [64, 83], [128, 167], [256, 335]....

This shouldn't be a huge problem as Prometheus supports in-frequent bucket bound changes. And since a upgrade necessitates a restart histogram values will be unstable anyway.

Beyond this the PR also refactors log_hist to add a couple of things;

Outputting a seastar metrics public histogram from log_hist_internal this allows us to share a single internal histogram for both public and internal metrics
Pausing auto latency measures. This allows us to avoid including purposefully added latencies (like the debounce in the fetch handler) in the overall latency.
A benchmark comparing hdr_hist::record to log_hist:record

Backports Required

Release Notes

none

src/v/utils/log_hist.cc

src/v/utils/log_hist.h

StephanDollberg · 2023-07-27T10:40:19Z

src/v/utils/tests/seastar_histogram_bench.cc

+PERF_TEST(log_hist, record) {
+    log_hist_internal h;
+    perf_tests::start_measuring_time();
+#pragma nounroll


btw I think nounroll doesn't actually work on GCC

That's a bummer... is there anything that works on clang and gcc?

we don't use gcc, so this isn't a concern? does it work on clang? can you wrap the loop in do_not_optimize?

StephanDollberg · 2023-07-27T10:40:39Z

src/v/utils/log_hist.h

@@ -185,7 +185,7 @@ class log_hist : public log_hist_base {
     */
    void record(uint64_t val) {
        _sample_sum += val;
-        const int i = std::clamp(
+        const unsigned i = std::clamp(


nice, how did you arrive at this being the cause?

The instruction was sign extending the register holding i into the register used to index the vector. After I switched it to unsigned clang used the register storing i to index into the vector without moving it to a different register. I guess the unsigned was enough to let clang know that i isn't going to be negative and won't need any sign extension when converting to a 64 bit index into the vector.

BenPope · 2023-07-27T10:34:46Z

src/v/utils/log_hist.h

+    seastar::metrics::histogram internal_histogram_logform() const;
+
+protected:
+    int _number_of_buckets;


This looks unused.

Right you are, will remove.

BenPope · 2023-07-27T10:48:40Z

src/v/utils/log_hist.h

+
+protected:
+    int _number_of_buckets;
+    uint64_t _first_bucket_upper_bound;


nitpick: This could probably be a template parameter, with explicit template instantiation for the couple of used values.

Good point, will switch this over to explicit template instantiation.

BenPope · 2023-07-27T10:53:20Z

src/v/utils/log_hist.h

+protected:
+    int _number_of_buckets;
+    uint64_t _first_bucket_upper_bound;
+    std::vector<uint64_t> _counts;


nitpick: With some more templating, this could be a std::array, which takes the runtime of the benchmark from ~128ms to ~110ms.

BenPope@89d274b

Nice! Will make a couple changes to the commit above and add it to the PR with you marked as a co-author.

BenPope

I'd be tempted to switch it around a bit:

pick caf95eb834 utils: remove unneeded movslq from record function
pick eab354af54 utils: refactor seastar_histogram_logform to allow for conversions from log_hist_internal to a public seastar hist
f 0c848330c9 utils/log_hist: Use std::array
f ee75bdb32f utils: remove log_hist_base
pick af9b60e904 utils: allow for pausing auto latency measurements in log_hist
pick 68b4bbf212 utils: add histogram benchmark
pick 0d5e0ec328 treewide: replace hdr_hist with log_hist

emaxerrno · 2023-07-28T15:06:36Z

@ballard26 - i assume this comes with a vtools patch to remove hdrhist - i don't see the cmake changes.

ballard26 · 2023-07-28T15:37:24Z

@ballard26 - i assume this comes with a vtools patch to remove hdrhist - i don't see the cmake changes.

There is still one outstanding use of hdr_hist in src/v/cluster/self_test. Where it's being used to generate quantiles for a statistical summary. I'll be adding support to log_hist for generating quantiles this coming week. At which point I'll get a few PRs up to remove hdr_hist from RP and both of our build systems(public and vtools) entirely .

…om log_hist_internal to a public seastar hist Co-authored-by: Ben Pope <[email protected]>

This reduces the memory required to store a histogram from kilobytes to 208 bytes. It also speeds up recording to the histogram by 2x.

dotnwat

lgtm

src/v/kafka/server/handlers/handler_probe.cc

src/v/utils/log_hist.cc

dotnwat · 2023-08-01T20:09:20Z

src/v/utils/tests/seastar_histogram_bench.cc

+PERF_TEST(log_hist, record) {
+    log_hist_internal h;
+    perf_tests::start_measuring_time();
+#pragma nounroll


we don't use gcc, so this isn't a concern? does it work on clang? can you wrap the loop in do_not_optimize?

dotnwat · 2023-08-03T03:12:30Z

the linter error seems like a false positive i ran format locally on this pr and it seems fine.

ballard26 requested review from travisdowns and StephanDollberg July 26, 2023 03:15

ballard26 changed the title ~~log_hist improvements~~ log_hist improvements Jul 26, 2023

github-actions bot added the area/redpanda label Jul 26, 2023

StephanDollberg previously approved these changes Jul 26, 2023

View reviewed changes

src/v/utils/log_hist.cc Show resolved Hide resolved

src/v/utils/log_hist.h Outdated Show resolved Hide resolved

ballard26 dismissed StephanDollberg’s stale review via 7234833 July 26, 2023 14:48

ballard26 force-pushed the per-handler-hist branch 2 times, most recently from 7234833 to 0934d1b Compare July 26, 2023 14:50

StephanDollberg previously approved these changes Jul 26, 2023

View reviewed changes

ballard26 requested a review from StephanDollberg July 26, 2023 14:50

ballard26 dismissed StephanDollberg’s stale review via 0d5e0ec July 27, 2023 00:21

ballard26 force-pushed the per-handler-hist branch from 0934d1b to 0d5e0ec Compare July 27, 2023 00:21

ballard26 changed the title ~~log_hist improvements~~ Replace hdr_hist with log_hist Jul 27, 2023

ballard26 requested review from dotnwat and BenPope July 27, 2023 00:41

StephanDollberg previously approved these changes Jul 27, 2023

View reviewed changes

BenPope reviewed Jul 27, 2023

View reviewed changes

ballard26 dismissed StephanDollberg’s stale review via ee75bdb July 27, 2023 23:48

ballard26 requested review from BenPope and StephanDollberg July 28, 2023 01:39

StephanDollberg previously approved these changes Jul 28, 2023

View reviewed changes

BenPope reviewed Jul 28, 2023

View reviewed changes

ballard26 dismissed StephanDollberg’s stale review via 5b842fa July 28, 2023 17:36

ballard26 force-pushed the per-handler-hist branch from ee75bdb to 5b842fa Compare July 28, 2023 17:36

ballard26 and others added 4 commits July 28, 2023 13:37

utils: remove unneeded movslq from record function

961178b

utils: refactor seastar_histogram_logform to allow for conversions fr…

6b578bd

…om log_hist_internal to a public seastar hist Co-authored-by: Ben Pope <[email protected]>

utils: allow for pausing auto latency measurements in log_hist

caa4e92

utils: add histogram benchmark

e6e5f37

treewide: replace hdr_hist with log_hist

c2959f5

This reduces the memory required to store a histogram from kilobytes to 208 bytes. It also speeds up recording to the histogram by 2x.

ballard26 force-pushed the per-handler-hist branch from 5b842fa to c2959f5 Compare July 28, 2023 17:37

StephanDollberg approved these changes Jul 31, 2023

View reviewed changes

BenPope approved these changes Jul 31, 2023

View reviewed changes

dotnwat reviewed Aug 1, 2023

View reviewed changes

dotnwat merged commit b050ac4 into redpanda-data:dev Aug 3, 2023
16 of 17 checks passed

BenPope mentioned this pull request Aug 3, 2023

kafka: Abort fetch and list_offsets when client disconnects #12021

Merged

8 tasks

StephanDollberg mentioned this pull request Sep 8, 2023

Backport per-handler metrics to v23.1 #13344

Merged

7 tasks

StephanDollberg mentioned this pull request Sep 28, 2023

[v23.2.x] Replace hdr_hist with log_hist #13759

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `hdr_hist` with `log_hist` #12460

Replace `hdr_hist` with `log_hist` #12460

ballard26 commented Jul 26, 2023 •

edited

Loading

StephanDollberg Jul 27, 2023

ballard26 Jul 27, 2023

dotnwat Aug 1, 2023

StephanDollberg Jul 27, 2023

ballard26 Jul 27, 2023 •

edited

Loading

dotnwat Aug 1, 2023

BenPope Jul 27, 2023

ballard26 Jul 27, 2023

BenPope Jul 27, 2023

ballard26 Jul 27, 2023

BenPope Jul 27, 2023 •

edited

Loading

BenPope Jul 27, 2023

ballard26 Jul 27, 2023

BenPope left a comment •

edited

Loading

emaxerrno commented Jul 28, 2023

ballard26 commented Jul 28, 2023

dotnwat left a comment

dotnwat Aug 1, 2023

dotnwat commented Aug 3, 2023

Replace hdr_hist with log_hist #12460

Replace hdr_hist with log_hist #12460

Conversation

ballard26 commented Jul 26, 2023 • edited Loading

Backports Required

Release Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ballard26 Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenPope Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenPope left a comment • edited Loading

Choose a reason for hiding this comment

emaxerrno commented Jul 28, 2023

ballard26 commented Jul 28, 2023

dotnwat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotnwat commented Aug 3, 2023

Replace `hdr_hist` with `log_hist` #12460

Replace `hdr_hist` with `log_hist` #12460

ballard26 commented Jul 26, 2023 •

edited

Loading

ballard26 Jul 27, 2023 •

edited

Loading

BenPope Jul 27, 2023 •

edited

Loading

BenPope left a comment •

edited

Loading