[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