[metrics] Refactor reporting

Bug: 170149255
Test: m test-art-host-gtest-art_libartbase_tests
Change-Id: Ic02dd94f55fb782e1ffb59789bf95e7f53f1f8e8
diff --git a/libartbase/base/metrics.cc b/libartbase/base/metrics.cc
index 5cab3c9..e146856 100644
--- a/libartbase/base/metrics.cc
+++ b/libartbase/base/metrics.cc
@@ -59,13 +59,12 @@
 
 void ArtMetrics::ReportAllMetrics(MetricsBackend* backend) const {
 // Dump counters
-#define ART_COUNTER(name) backend->ReportCounter(DatumId::k##name, name()->Value());
+#define ART_COUNTER(name) name()->Report(backend);
   ART_COUNTERS(ART_COUNTER)
 #undef ART_COUNTERS
 
 // Dump histograms
-#define ART_HISTOGRAM(name, num_buckets, low_value, high_value) \
-  backend->ReportHistogram(DatumId::k##name, low_value, high_value, name()->GetBuckets());
+#define ART_HISTOGRAM(name, num_buckets, low_value, high_value) name()->Report(backend);
   ART_HISTOGRAMS(ART_HISTOGRAM)
 #undef ART_HISTOGRAM
 }
diff --git a/libartbase/base/metrics.h b/libartbase/base/metrics.h
index 119f622..d5a4873 100644
--- a/libartbase/base/metrics.h
+++ b/libartbase/base/metrics.h
@@ -123,11 +123,13 @@
                                int64_t maximum_value,
                                const std::vector<uint32_t>& buckets) = 0;
 
-  friend class ArtMetrics;
-  template <size_t num_buckets, int64_t low_value, int64_t high_value>
+  template <DatumId counter_type>
+  friend class MetricsCounter;
+  template <DatumId histogram_type, size_t num_buckets, int64_t low_value, int64_t high_value>
   friend class MetricsHistogram;
 };
 
+template <DatumId counter_type>
 class MetricsCounter {
  public:
   using value_t = uint64_t;
@@ -140,20 +142,25 @@
   void AddOne() { Add(1u); }
   void Add(value_t value) { value_.fetch_add(value, std::memory_order::memory_order_relaxed); }
 
-  value_t Value() const { return value_.load(std::memory_order::memory_order_relaxed); }
+  void Report(MetricsBackend* backend) const { backend->ReportCounter(counter_type, Value()); }
 
  private:
+  value_t Value() const { return value_.load(std::memory_order::memory_order_relaxed); }
+
   std::atomic<value_t> value_;
   static_assert(std::atomic<value_t>::is_always_lock_free);
 };
 
-template <size_t num_buckets_, int64_t minimum_value_, int64_t maximum_value_>
+template <DatumId histogram_type_,
+          size_t num_buckets_,
+          int64_t minimum_value_,
+          int64_t maximum_value_>
 class MetricsHistogram {
   static_assert(num_buckets_ >= 1);
   static_assert(minimum_value_ < maximum_value_);
 
  public:
-  using value_t = int64_t;
+  using value_t = uint32_t;
 
   constexpr MetricsHistogram() : buckets_{} {
     // Ensure we do not have any unnecessary data in this class.
@@ -165,12 +172,8 @@
     buckets_[i].fetch_add(1u, std::memory_order::memory_order_relaxed);
   }
 
- protected:
-  std::vector<uint32_t> GetBuckets() const {
-    // The loads from buckets_ will all be memory_order_seq_cst, which means they will be acquire
-    // loads. This is a stricter memory order than is needed, but this should not be a
-    // performance-critical section of code.
-    return std::vector<uint32_t>{buckets_.begin(), buckets_.end()};
+  void Report(MetricsBackend* backend) const {
+    backend->ReportHistogram(histogram_type_, minimum_value_, maximum_value_, GetBuckets());
   }
 
  private:
@@ -188,10 +191,15 @@
     return static_cast<size_t>(value - minimum_value_) * num_buckets_ / bucket_width;
   }
 
-  std::array<std::atomic<uint32_t>, num_buckets_> buckets_;
+  std::vector<value_t> GetBuckets() const {
+    // The loads from buckets_ will all be memory_order_seq_cst, which means they will be acquire
+    // loads. This is a stricter memory order than is needed, but this should not be a
+    // performance-critical section of code.
+    return std::vector<value_t>{buckets_.begin(), buckets_.end()};
+  }
 
-  friend class ArtMetrics;
-  static_assert(std::atomic<uint32_t>::is_always_lock_free);
+  std::array<std::atomic<value_t>, num_buckets_> buckets_;
+  static_assert(std::atomic<value_t>::is_always_lock_free);
 };
 
 /**
@@ -272,15 +280,19 @@
 
   void ReportAllMetrics(MetricsBackend* backend) const;
 
-#define ART_COUNTER(name)                      \
-  MetricsCounter* name() { return &name##_; }  \
-  const MetricsCounter* name() const { return &name##_; }
+#define ART_COUNTER(name)                                       \
+  MetricsCounter<DatumId::k##name>* name() { return &name##_; } \
+  const MetricsCounter<DatumId::k##name>* name() const { return &name##_; }
   ART_COUNTERS(ART_COUNTER)
 #undef ART_COUNTER
 
-#define ART_HISTOGRAM(name, num_buckets, low_value, high_value)                      \
-  MetricsHistogram<num_buckets, low_value, high_value>* name() { return &name##_; }  \
-  const MetricsHistogram<num_buckets, low_value, high_value>* name() const { return &name##_; }
+#define ART_HISTOGRAM(name, num_buckets, low_value, high_value)                                \
+  MetricsHistogram<DatumId::k##name, num_buckets, low_value, high_value>* name() {             \
+    return &name##_;                                                                           \
+  }                                                                                            \
+  const MetricsHistogram<DatumId::k##name, num_buckets, low_value, high_value>* name() const { \
+    return &name##_;                                                                           \
+  }
   ART_HISTOGRAMS(ART_HISTOGRAM)
 #undef ART_HISTOGRAM
 
@@ -294,12 +306,12 @@
   int unused_[0];
 #pragma clang diagnostic pop  // -Wunused-private-field
 
-#define ART_COUNTER(name) MetricsCounter name##_;
+#define ART_COUNTER(name) MetricsCounter<DatumId::k##name> name##_;
   ART_COUNTERS(ART_COUNTER)
 #undef ART_COUNTER
 
 #define ART_HISTOGRAM(name, num_buckets, low_value, high_value) \
-  MetricsHistogram<num_buckets, low_value, high_value> name##_;
+  MetricsHistogram<DatumId::k##name, num_buckets, low_value, high_value> name##_;
   ART_HISTOGRAMS(ART_HISTOGRAM)
 #undef ART_HISTOGRAM
 };
diff --git a/libartbase/base/metrics_test.cc b/libartbase/base/metrics_test.cc
index 67ad313..20ac92c 100644
--- a/libartbase/base/metrics_test.cc
+++ b/libartbase/base/metrics_test.cc
@@ -17,6 +17,7 @@
 #include "metrics.h"
 
 #include "gtest/gtest.h"
+#include "metrics_test.h"
 
 #pragma clang diagnostic push
 #pragma clang diagnostic error "-Wconversion"
@@ -24,76 +25,51 @@
 namespace art {
 namespace metrics {
 
+using test::CounterValue;
+using test::GetBuckets;
+using test::TestBackendBase;
+
 class MetricsTest : public testing::Test {};
 
-namespace {
-
-// MetricsHistogram::GetBuckets is protected because we only want limited places to be able to read
-// the buckets. Tests are one of those places, but because making a gTest a friend is difficult,
-// instead we subclass MetricsHistogram and expose GetBuckets as GetBucksForTest.
-template <size_t num_buckets, int64_t low_value, int64_t high_value>
-class TestMetricsHistogram : public MetricsHistogram<num_buckets, low_value, high_value> {
- public:
-  std::vector<uint32_t> GetBucketsForTest() const { return this->GetBuckets(); }
-};
-
-// A trivial MetricsBackend that does nothing for all of the members. This can be overridden by
-// test cases to test specific behaviors.
-class TestBackendBase : public MetricsBackend {
- public:
-  void BeginSession([[maybe_unused]] const SessionData& session_data) override {}
-  void EndSession() override {}
-
-  void ReportCounter([[maybe_unused]] DatumId counter_type,
-                     [[maybe_unused]] uint64_t value) override {}
-
-  void ReportHistogram([[maybe_unused]] DatumId histogram_type,
-                       [[maybe_unused]] int64_t low_value_,
-                       [[maybe_unused]] int64_t high_value,
-                       [[maybe_unused]] const std::vector<uint32_t>& buckets) override {}
-};
-
-}  // namespace
-
 TEST_F(MetricsTest, SimpleCounter) {
-  MetricsCounter test_counter;
+  MetricsCounter<DatumId::kClassVerificationTotalTime> test_counter;
 
-  EXPECT_EQ(0u, test_counter.Value());
+  EXPECT_EQ(0u, CounterValue(test_counter));
 
   test_counter.AddOne();
-  EXPECT_EQ(1u, test_counter.Value());
+  EXPECT_EQ(1u, CounterValue(test_counter));
 
   test_counter.Add(5);
-  EXPECT_EQ(6u, test_counter.Value());
+  EXPECT_EQ(6u, CounterValue(test_counter));
 }
 
 TEST_F(MetricsTest, CounterTimer) {
-  MetricsCounter test_counter;
+  MetricsCounter<DatumId::kClassVerificationTotalTime> test_counter;
   {
     AutoTimer timer{&test_counter};
     // Sleep for 2µs so the counter will be greater than 0.
     NanoSleep(2'000);
   }
-  EXPECT_GT(test_counter.Value(), 0u);
+  EXPECT_GT(CounterValue(test_counter), 0u);
 }
 
 TEST_F(MetricsTest, CounterTimerExplicitStop) {
-  MetricsCounter test_counter;
+  MetricsCounter<DatumId::kClassVerificationTotalTime> test_counter;
   AutoTimer timer{&test_counter};
   // Sleep for 2µs so the counter will be greater than 0.
   NanoSleep(2'000);
   timer.Stop();
-  EXPECT_GT(test_counter.Value(), 0u);
+  EXPECT_GT(CounterValue(test_counter), 0u);
 }
 
 TEST_F(MetricsTest, CounterTimerExplicitStart) {
-  MetricsCounter test_counter;
+  MetricsCounter<DatumId::kClassVerificationTotalTime> test_counter;
   {
     AutoTimer timer{&test_counter, /*autostart=*/false};
     // Sleep for 2µs so the counter will be greater than 0.
     NanoSleep(2'000);
   }
-  EXPECT_EQ(test_counter.Value(), 0u);
+  EXPECT_EQ(CounterValue(test_counter), 0u);
 
   {
     AutoTimer timer{&test_counter, /*autostart=*/false};
@@ -101,17 +77,17 @@
     // Sleep for 2µs so the counter will be greater than 0.
     NanoSleep(2'000);
   }
-  EXPECT_GT(test_counter.Value(), 0u);
+  EXPECT_GT(CounterValue(test_counter), 0u);
 }
 
 TEST_F(MetricsTest, CounterTimerExplicitStartStop) {
-  MetricsCounter test_counter;
+  MetricsCounter<DatumId::kClassVerificationTotalTime> test_counter;
   AutoTimer timer{&test_counter, /*autostart=*/false};
   // Sleep for 2µs so the counter will be greater than 0.
   timer.Start();
   NanoSleep(2'000);
   timer.Stop();
-  EXPECT_GT(test_counter.Value(), 0u);
+  EXPECT_GT(CounterValue(test_counter), 0u);
 }
 
 TEST_F(MetricsTest, DatumName) {
@@ -119,7 +95,7 @@
 }
 
 TEST_F(MetricsTest, SimpleHistogramTest) {
-  TestMetricsHistogram<5, 0, 100> histogram;
+  MetricsHistogram<DatumId::kJitMethodCompileTime, 5, 0, 100> histogram;
 
   // bucket 0: 0-19
   histogram.Add(10);
@@ -142,7 +118,7 @@
   // bucket 4: 80-99
   // leave this bucket empty
 
-  std::vector<uint32_t> buckets{histogram.GetBucketsForTest()};
+  std::vector<uint32_t> buckets{GetBuckets(histogram)};
   EXPECT_EQ(1u, buckets[0u]);
   EXPECT_EQ(2u, buckets[1u]);
   EXPECT_EQ(4u, buckets[2u]);
@@ -152,7 +128,7 @@
 
 // Make sure values added outside the range of the histogram go into the first or last bucket.
 TEST_F(MetricsTest, HistogramOutOfRangeTest) {
-  TestMetricsHistogram<2, 0, 100> histogram;
+  MetricsHistogram<DatumId::kJitMethodCompileTime, 2, 0, 100> histogram;
 
   // bucket 0: 0-49
   histogram.Add(-500);
@@ -161,7 +137,7 @@
   histogram.Add(250);
   histogram.Add(1000);
 
-  std::vector<uint32_t> buckets{histogram.GetBucketsForTest()};
+  std::vector<uint32_t> buckets{GetBuckets(histogram)};
   EXPECT_EQ(1u, buckets[0u]);
   EXPECT_EQ(2u, buckets[1u]);
 }
@@ -219,14 +195,14 @@
 }
 
 TEST_F(MetricsTest, HistogramTimer) {
-  TestMetricsHistogram<1, 0, 100> test_histogram;
+  MetricsHistogram<DatumId::kJitMethodCompileTime, 1, 0, 100> test_histogram;
   {
     AutoTimer timer{&test_histogram};
     // Sleep for 2µs so the counter will be greater than 0.
     NanoSleep(2'000);
   }
 
-  EXPECT_GT(test_histogram.GetBucketsForTest()[0], 0u);
+  EXPECT_GT(GetBuckets(test_histogram)[0], 0u);
 }
 
 }  // namespace metrics
diff --git a/libartbase/base/metrics_test.h b/libartbase/base/metrics_test.h
new file mode 100644
index 0000000..0c6e99f
--- /dev/null
+++ b/libartbase/base/metrics_test.h
@@ -0,0 +1,85 @@
+/*
+ * Copyright (C) 2020 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef ART_LIBARTBASE_BASE_METRICS_TEST_H_
+#define ART_LIBARTBASE_BASE_METRICS_TEST_H_
+
+#include "metrics.h"
+
+#pragma clang diagnostic push
+#pragma clang diagnostic error "-Wconversion"
+
+namespace art {
+namespace metrics {
+namespace test {
+
+// This namespace contains functions that are helpful for testing metrics but should not be used in
+// production code.
+
+// A trivial MetricsBackend that does nothing for all of the members. This can be overridden by
+// test cases to test specific behaviors.
+class TestBackendBase : public MetricsBackend {
+ public:
+  void BeginSession([[maybe_unused]] const SessionData& session_data) override {}
+  void EndSession() override {}
+
+  void ReportCounter([[maybe_unused]] DatumId counter_type,
+                     [[maybe_unused]] uint64_t value) override {}
+
+  void ReportHistogram([[maybe_unused]] DatumId histogram_type,
+                       [[maybe_unused]] int64_t low_value_,
+                       [[maybe_unused]] int64_t high_value,
+                       [[maybe_unused]] const std::vector<uint32_t>& buckets) override {}
+};
+
+template <DatumId counter_type>
+uint64_t CounterValue(const MetricsCounter<counter_type>& counter) {
+  uint64_t counter_value{0};
+  struct CounterBackend : public TestBackendBase {
+    explicit CounterBackend(uint64_t* counter_value) : counter_value_{counter_value} {}
+
+    void ReportCounter(DatumId, uint64_t value) override { *counter_value_ = value; }
+
+    uint64_t* counter_value_;
+  } backend{&counter_value};
+  counter.Report(&backend);
+  return counter_value;
+}
+
+template <DatumId histogram_type, size_t num_buckets, int64_t low_value, int64_t high_value>
+std::vector<uint32_t> GetBuckets(
+    const MetricsHistogram<histogram_type, num_buckets, low_value, high_value>& histogram) {
+  std::vector<uint32_t> buckets;
+  struct HistogramBackend : public TestBackendBase {
+    explicit HistogramBackend(std::vector<uint32_t>* buckets) : buckets_{buckets} {}
+
+    void ReportHistogram(DatumId, int64_t, int64_t, const std::vector<uint32_t>& buckets) override {
+      *buckets_ = buckets;
+    }
+
+    std::vector<uint32_t>* buckets_;
+  } backend{&buckets};
+  histogram.Report(&backend);
+  return buckets;
+}
+
+}  // namespace test
+}  // namespace metrics
+}  // namespace art
+
+#pragma clang diagnostic pop  // -Wconversion
+
+#endif  // ART_LIBARTBASE_BASE_METRICS_TEST_H_
diff --git a/runtime/verifier/method_verifier_test.cc b/runtime/verifier/method_verifier_test.cc
index d7e193c..2e69b35 100644
--- a/runtime/verifier/method_verifier_test.cc
+++ b/runtime/verifier/method_verifier_test.cc
@@ -17,10 +17,11 @@
 #include "method_verifier.h"
 
 #include <stdio.h>
+
 #include <memory>
 
 #include "android-base/strings.h"
-
+#include "base/metrics_test.h"
 #include "base/utils.h"
 #include "class_linker-inl.h"
 #include "class_verifier.h"
@@ -32,6 +33,8 @@
 namespace art {
 namespace verifier {
 
+using metrics::test::CounterValue;
+
 class MethodVerifierTest : public CommonRuntimeTest {
  protected:
   void VerifyClass(const std::string& descriptor)
@@ -75,9 +78,10 @@
 TEST_F(MethodVerifierTest, VerificationTimeMetrics) {
   ScopedObjectAccess soa(Thread::Current());
   ASSERT_TRUE(java_lang_dex_file_ != nullptr);
-  const uint64_t original_time = GetMetrics()->ClassVerificationTotalTime()->Value();
+  auto* class_verification_total_time = GetMetrics()->ClassVerificationTotalTime();
+  const uint64_t original_time = CounterValue(*class_verification_total_time);
   VerifyDexFile(*java_lang_dex_file_);
-  ASSERT_GT(GetMetrics()->ClassVerificationTotalTime()->Value(), original_time);
+  ASSERT_GT(CounterValue(*class_verification_total_time), original_time);
 }
 
 }  // namespace verifier