MB-41510: Tidy HdrHistogram class

Tidy up HdrHistogram class by adding const and static keywords where
applicable and use fmt over std::stringstream. Also ensure we don't
perform self assignment in HdrHistogram's assignment operator.

Change-Id: I5498f3d2bf08afe7cb7269906b9f13076c5e2ab4
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/136612
Tested-by: Build Bot <[email protected]>
Reviewed-by: Dave Rigby <[email protected]>

Author: rdemellow

engines/ep/tests/module_tests/hdrhistogram_test.cc

@@ -80,7 +80,7 @@ TEST(HdrHistogramTest, linearIteratorTest) {
     HdrHistogram::Iterator iter{
             histogram.makeLinearIterator(/* valueUnitsPerBucket */ 1)};
     uint64_t valueCount = 0;
-    while (auto result = histogram.getNextValueAndCount(iter)) {
+    while (auto result = iter.getNextValueAndCount()) {
         EXPECT_EQ(valueCount, result->first);
         ++valueCount;
     }
@@ -104,7 +104,7 @@ TEST(HdrHistogramTest, logIteratorBaseTwoTest) {
 
     uint64_t countSum = 0;
     uint64_t bucketIndex = 0;
-    while (auto result = histogram.getNextValueAndCount(iter)) {
+    while (auto result = iter.getNextValueAndCount()) {
         // Check that the values of the buckets increase exponentially
         EXPECT_EQ(pow(iteratorBase, bucketIndex) - 1, result->first);
         // Check that the width of the bucket is the same number as the count
@@ -138,7 +138,7 @@ TEST(HdrHistogramTest, logIteratorBaseFiveTest) {
 
     uint64_t countSum = 0;
     uint64_t bucketIndex = 0;
-    while (auto result = histogram.getNextValueAndCount(iter)) {
+    while (auto result = iter.getNextValueAndCount()) {
         // Check that the values of the buckets increase exponentially
         EXPECT_EQ(pow(iteratorBase, bucketIndex) - 1, result->first);
         // Check that the width of the bucket is the same number as the count
@@ -161,7 +161,7 @@ TEST(HdrHistogramTest, addValueAndCountTest) {
     histogram.addValueAndCount(0, 100);
     // Need to create the iterator after we have added the data
     HdrHistogram::Iterator iter{histogram.makeLinearIterator(1)};
-    while (auto result = histogram.getNextValueAndCount(iter)) {
+    while (auto result = iter.getNextValueAndCount()) {
         EXPECT_EQ(0, result->first);
         EXPECT_EQ(100, result->second);
     }
@@ -288,7 +288,7 @@ TEST(HdrHistogramTest, addValueParallel) {
             histogram.makeLinearIterator(/* valueUnitsPerBucket */ 1)};
     uint64_t valueCount = 0;
     // Assert that the right number of values were added to the histogram
-    while (auto result = histogram.getNextValueAndCount(iter)) {
+    while (auto result = iter.getNextValueAndCount()) {
         ASSERT_EQ(valueCount, result->first);
         ASSERT_EQ(threads.size() * numOfAddIterations, result->second);
         ++valueCount;
@@ -322,8 +322,8 @@ TEST(HdrHistogramTest, aggregationTest) {
             histogramTwo.makeLinearIterator(/* valueUnitsPerBucket */ 1)};
     uint64_t valueCount = 0;
     for (int i = 0; i < 15; i++) {
-        auto resultOne = histogramOne.getNextValueAndCount(iterOne);
-        auto resultTwo = histogramOne.getNextValueAndCount(iterTwo);
+        auto resultOne = iterOne.getNextValueAndCount();
+        auto resultTwo = iterTwo.getNextValueAndCount();
         // check values are the same for both histograms
         EXPECT_EQ(valueCount, resultTwo->first);
         EXPECT_EQ(valueCount, resultOne->first);
@@ -357,8 +357,8 @@ TEST(HdrHistogramTest, aggregationTestEmptyLhr) {
     // Max value of LHS should be updated too 200 thus counts should be the
     // same for every value in both histograms
     for (int i = 0; i < 200; i++) {
-        auto resultOne = histogramOne.getNextValueAndCount(iterOne);
-        auto resultTwo = histogramOne.getNextValueAndCount(iterTwo);
+        auto resultOne = iterOne.getNextValueAndCount();
+        auto resultTwo = iterTwo.getNextValueAndCount();
         // check values are the same for both histograms
         EXPECT_EQ(resultOne->first, resultTwo->first);
         // check that the counts for each value are the same
@@ -386,7 +386,7 @@ TEST(HdrHistogramTest, aggregationTestEmptyRhs) {
 
     uint64_t valueCount = 0;
     // make sure the histogram has expanded in size for all 200 values
-    while (auto result = histogramOne.getNextValueAndCount(iter)) {
+    while (auto result = iter.getNextValueAndCount()) {
         EXPECT_EQ(valueCount, result->first);
         EXPECT_EQ(1, result->second);
         ++valueCount;
@@ -413,7 +413,7 @@ TEST(HdrHistogramTest, int32MaxSizeTest) {
     EXPECT_EQ(0, histogram.getMinValue());
 
     HdrHistogram::Iterator iter{histogram.getHistogramsIterator()};
-    auto res = histogram.getNextBucketLowHighAndCount(iter);
+    auto res = iter.getNextBucketLowHighAndCount();
     EXPECT_TRUE(res);
     // The 3rd field [2] of the returned tuple is the count
     EXPECT_EQ(limit, std::get<2>(*res));
@@ -429,7 +429,7 @@ TEST(HdrHistogramTest, int32MaxSizeTest) {
     EXPECT_EQ(0, histogram.getMinValue());
 
     HdrHistogram::Iterator iter2{histogram.getHistogramsIterator()};
-    auto res2 = histogram.getNextBucketLowHighAndCount(iter2);
+    auto res2 = iter2.getNextBucketLowHighAndCount();
     EXPECT_TRUE(res2);
     // The 3rd field [2] of the returned tuple is the count
     EXPECT_EQ(limit, std::get<2>(*res2));
@@ -457,7 +457,7 @@ TEST(HdrHistogramTest, int64MaxSizeTest) {
     EXPECT_EQ(0, histogram.getMinValue());
 
     HdrHistogram::Iterator iter{histogram.getHistogramsIterator()};
-    auto res = histogram.getNextBucketLowHighAndCount(iter);
+    auto res = iter.getNextBucketLowHighAndCount();
     EXPECT_TRUE(res);
     // The 3rd field [2] of the returned tuple is the count
     EXPECT_EQ(limit, std::get<2>(*res));
@@ -469,7 +469,7 @@ TEST(HdrHistogramTest, int64MaxSizeTest) {
 static std::vector<std::optional<std::pair<uint64_t, double>>> getAllValues(
         const HdrHistogram& histo, HdrHistogram::Iterator& iter) {
     std::vector<std::optional<std::pair<uint64_t, double>>> values;
-    while (auto pair = histo.getNextValueAndPercentile(iter)) {
+    while (auto pair = iter.getNextValueAndPercentile()) {
         if (!pair.has_value())
             break;
         values.push_back(pair);

statistics/collector.cc

@@ -43,7 +43,7 @@ void StatCollector::addStat(const cb::stats::StatDef& k,
         histData.sampleCount = v.getValueCount();
 
         HdrHistogram::Iterator iter{v.getHistogramsIterator()};
-        while (auto result = v.getNextBucketLowHighAndCount(iter)) {
+        while (auto result = iter.getNextBucketLowHighAndCount()) {
             auto [lower, upper, count] = *result;
 
             histData.buckets.push_back({lower, upper, count});

utilities/hdrhistogram.cc

@@ -20,9 +20,9 @@
 #include <folly/lang/Assume.h>
 #include <nlohmann/json.hpp>
 #include <platform/cb_malloc.h>
+#include <spdlog/fmt/fmt.h>
 #include <iostream>
 #include <optional>
-#include <sstream>
 
 // Custom deleter for the hdr_histogram struct.
 void HdrHistogram::HdrDeleter::operator()(struct hdr_histogram* val) {
@@ -85,6 +85,17 @@ HdrHistogram& HdrHistogram::operator+=(const HdrHistogram& other) {
     return *this;
 }
 
+HdrHistogram& HdrHistogram::operator=(const HdrHistogram& other) {
+    if (this != &other) {
+        // reset this object to make sure we are in a state to copy too
+        this->reset();
+        // take advantage of the code already written in for the addition
+        // assigment operator
+        *this += other;
+    }
+    return *this;
+}
+
 bool HdrHistogram::addValue(uint64_t v) {
     // A hdr_histogram cannot store 0, therefore we add a bias of +1.
     int64_t vBiased = v + 1;
@@ -187,26 +198,26 @@ HdrHistogram::Iterator HdrHistogram::getHistogramsIterator() const {
     return makeIterator(defaultIterationMode);
 }
 
-std::optional<std::pair<uint64_t, uint64_t>> HdrHistogram::getNextValueAndCount(
-        Iterator& iter) const {
+std::optional<std::pair<uint64_t, uint64_t>>
+HdrHistogram::Iterator::getNextValueAndCount() {
     std::optional<std::pair<uint64_t, uint64_t>> valueAndCount;
-    if (hdr_iter_next(&iter)) {
-        auto value = static_cast<uint64_t>(iter.value);
+    if (hdr_iter_next(this)) {
+        auto value = static_cast<uint64_t>(this->value);
         uint64_t count = 0;
-        switch (iter.type) {
+        switch (type) {
         case Iterator::IterMode::Log:
-            count = iter.specifics.log.count_added_in_this_iteration_step;
+            count = specifics.log.count_added_in_this_iteration_step;
             break;
         case Iterator::IterMode::Linear:
-            count = iter.specifics.linear.count_added_in_this_iteration_step;
+            count = specifics.linear.count_added_in_this_iteration_step;
             break;
         case Iterator::IterMode::Percentiles:
-            value = iter.highest_equivalent_value;
-            count = iter.cumulative_count - iter.lastCumulativeCount;
-            iter.lastCumulativeCount = iter.cumulative_count;
+            value = highest_equivalent_value;
+            count = cumulative_count - lastCumulativeCount;
+            lastCumulativeCount = cumulative_count;
             break;
         case Iterator::IterMode::Recorded:
-            count = iter.count;
+            count = this->count;
             break;
         }
 
@@ -220,59 +231,61 @@ std::optional<std::pair<uint64_t, uint64_t>> HdrHistogram::getNextValueAndCount(
 }
 
 std::optional<std::tuple<uint64_t, uint64_t, uint64_t>>
-HdrHistogram::getNextBucketLowHighAndCount(Iterator& iter) const {
+HdrHistogram::Iterator::getNextBucketLowHighAndCount() {
     std::optional<std::tuple<uint64_t, uint64_t, uint64_t>> bucketHighLowCount;
-    auto valueAndCount = getNextValueAndCount(iter);
+    auto valueAndCount = getNextValueAndCount();
     if (valueAndCount.has_value()) {
-        bucketHighLowCount = std::make_tuple(
-                iter.lastVal, valueAndCount->first, valueAndCount->second);
-        iter.lastVal = valueAndCount->first;
+        bucketHighLowCount = {
+                lastVal, valueAndCount->first, valueAndCount->second};
+        lastVal = valueAndCount->first;
     }
     return bucketHighLowCount;
 }
 
 std::optional<std::pair<uint64_t, double>>
-HdrHistogram::getNextValueAndPercentile(Iterator& iter) const {
+HdrHistogram::Iterator::getNextValueAndPercentile() {
     std::optional<std::pair<uint64_t, double>> valueAndPer;
-    if (iter.type == Iterator::IterMode::Percentiles) {
-        if (hdr_iter_next(&iter)) {
+    if (type == Iterator::IterMode::Percentiles) {
+        if (hdr_iter_next(this)) {
             // We subtract one from the lowest value as we have added a one
             // offset as the underlying hdr_histogram cannot store 0 and
             // therefore the value must be greater than or equal to 1.
-            valueAndPer = std::make_pair(iter.highest_equivalent_value - 1,
-                                         iter.specifics.percentiles.percentile);
+            valueAndPer = {highest_equivalent_value - 1,
+                           specifics.percentiles.percentile};
         }
     }
     return valueAndPer;
 }
 
-void HdrHistogram::printPercentiles() const {
-    hdr_percentiles_print(histogram.rlock()->get(), stdout, 5, 1.0, CLASSIC);
+std::string HdrHistogram::Iterator::dumpValues() {
+    fmt::memory_buffer dump;
+    while (auto pair = getNextValueAndCount()) {
+        fmt::format_to(dump,
+                       "Value[{}-{}]\tCount:{}\t\n",
+                       value_iterated_from,
+                       value_iterated_to,
+                       pair->second);
+    }
+    fmt::format_to(dump, "Total:\t{}\n", total_count);
+    return {dump.data(), dump.size()};
 }
 
-void HdrHistogram::dumpLinearValues(int64_t bucketWidth) {
-    auto itr = makeLinearIterator(bucketWidth);
-    std::cout << dumpValues(itr).str();
+void HdrHistogram::printPercentiles() const {
+    hdr_percentiles_print(histogram.rlock()->get(), stdout, 5, 1.0, CLASSIC);
 }
 
-void HdrHistogram::dumpLogValues(int64_t firstBucketWidth, double log_base) {
-    auto itr = makeLogIterator(firstBucketWidth, log_base);
-    std::cout << dumpValues(itr).str();
+void HdrHistogram::dumpLinearValues(int64_t bucketWidth) const {
+    HdrHistogram::Iterator itr = makeLinearIterator(bucketWidth);
+    fmt::print(stdout, itr.dumpValues());
 }
 
-std::stringstream HdrHistogram::dumpValues(Iterator& itr) {
-    std::stringstream dump;
-    while (auto pair = getNextValueAndCount(itr)) {
-        dump << "Value[" << itr.value_iterated_from << "-"
-             << itr.value_iterated_to << "]\tCount:\t" << pair->second
-             << std::endl;
-    }
-    dump << "Total:\t" << itr.total_count << std::endl;
-
-    return dump;
+void HdrHistogram::dumpLogValues(int64_t firstBucketWidth,
+                                 double log_base) const {
+    HdrHistogram::Iterator itr = makeLogIterator(firstBucketWidth, log_base);
+    fmt::print(stdout, itr.dumpValues());
 }
 
-nlohmann::json HdrHistogram::to_json(Iterator::IterMode itrType) {
+nlohmann::json HdrHistogram::to_json() const {
     nlohmann::json rootObj;
 
     // Five is the number of iteration steps per half-distance to 100%.
@@ -285,7 +298,7 @@ nlohmann::json HdrHistogram::to_json(Iterator::IterMode itrType) {
     nlohmann::json dataArr;
 
     int64_t lastval = 0;
-    while (auto pair = getNextValueAndPercentile(itr)) {
+    while (auto pair = itr.getNextValueAndPercentile()) {
         if (!pair.has_value())
             break;
 
@@ -298,7 +311,7 @@ nlohmann::json HdrHistogram::to_json(Iterator::IterMode itrType) {
     return rootObj;
 }
 
-std::string HdrHistogram::to_string() {
+std::string HdrHistogram::to_string() const {
     return to_json().dump();
 }