Revert "[metrics] Add background reporting thread"

This reverts commit 77f7eb9d05b6d05531556882d99ed63688c6e3b0.

Reason for revert: We're seeing consistent 2233-metrics-background-thread failures on target.

Change-Id: Ie394dbe90289b1472ac4c8876d01ece347541852

14 files changed, 18 insertions, 238 deletions

diff --git a/libartbase/base/time_utils.h b/libartbase/base/time_utils.h
index e1273a380a..cb0ab13ef9 100644
--- a/libartbase/base/time_utils.h
+++ b/libartbase/base/time_utils.h
@@ -87,10 +87,6 @@ static constexpr uint64_t UsToNs(uint64_t us) {
   return us * 1000;
 }
 
-static constexpr uint64_t SecondsToMs(uint64_t seconds) {
-  return seconds * 1000;
-}
-
 static constexpr time_t SaturatedTimeT(int64_t secs) {
   if (sizeof(time_t) < sizeof(int64_t)) {
     return static_cast<time_t>(std::min(secs,
diff --git a/runtime/metrics/metrics.cc b/runtime/metrics/metrics.cc
index 0f8bacf679..81ae2b4dab 100644
--- a/runtime/metrics/metrics.cc
+++ b/runtime/metrics/metrics.cc
@@ -18,8 +18,6 @@
 
 #include "android-base/logging.h"
 #include "base/macros.h"
-#include "runtime.h"
-#include "thread-current-inl.h"
 
 #pragma clang diagnostic push
 #pragma clang diagnostic error "-Wconversion"
@@ -113,73 +111,19 @@ void StreamBackend::ReportHistogram(DatumId histogram_type,
   }
 }
 
-std::unique_ptr<MetricsReporter> MetricsReporter::Create(ReportingConfig config, Runtime* runtime) {
+std::unique_ptr<MetricsReporter> MetricsReporter::Create(ReportingConfig config,
+                                                         const ArtMetrics* metrics) {
   std::unique_ptr<MetricsBackend> backend;
 
   // We can't use std::make_unique here because the MetricsReporter constructor is private.
-  return std::unique_ptr<MetricsReporter>{new MetricsReporter{config, runtime}};
+  return std::unique_ptr<MetricsReporter>{new MetricsReporter{config, metrics}};
 }
 
-MetricsReporter::MetricsReporter(ReportingConfig config, Runtime* runtime)
-    : config_{config}, runtime_{runtime} {}
+MetricsReporter::MetricsReporter(ReportingConfig config, const ArtMetrics* metrics)
+    : config_{config}, metrics_{metrics} {}
 
-MetricsReporter::~MetricsReporter() { StopBackgroundThreadIfRunning(); }
-
-void MetricsReporter::StartBackgroundThreadIfNeeded() {
-  if (config_.BackgroundReportingEnabled()) {
-    CHECK(!thread_.has_value());
-
-    thread_.emplace(&MetricsReporter::BackgroundThreadRun, this);
-  }
-}
-
-void MetricsReporter::StopBackgroundThreadIfRunning() {
-  if (thread_.has_value()) {
-    messages_.SendMessage(ShutdownRequestedMessage{});
-    thread_->join();
-  }
-  // Do one final metrics report, if enabled.
-  if (config_.report_metrics_on_shutdown) {
-    ReportMetrics();
-  }
-}
-
-void MetricsReporter::BackgroundThreadRun() {
-  runtime_->AttachCurrentThread("Metrics Background Reporting Thread",
-                                /*as_daemon=*/true,
-                                runtime_->GetSystemThreadGroup(),
-                                /*create_peer=*/true);
-  LOG_STREAM(DEBUG) << "Metrics reporting thread started";
-  bool running = true;
-
-  ResetTimeoutIfNeeded();
-
-  while (running) {
-    messages_.SwitchReceive(
-        [&]([[maybe_unused]] ShutdownRequestedMessage message) {
-          LOG_STREAM(DEBUG) << "Shutdown request received";
-          running = false;
-        },
-        [&]([[maybe_unused]] TimeoutExpiredMessage message) {
-          LOG_STREAM(DEBUG) << "Timer expired, reporting metrics";
-
-          ReportMetrics();
-
-          ResetTimeoutIfNeeded();
-        });
-  }
-
-  runtime_->DetachCurrentThread();
-  LOG_STREAM(DEBUG) << "Metrics reporting thread terminating";
-}
-
-void MetricsReporter::ResetTimeoutIfNeeded() {
-  if (config_.periodic_report_seconds.has_value()) {
-    messages_.SetTimeout(SecondsToMs(config_.periodic_report_seconds.value()));
-  }
-}
-
-void MetricsReporter::ReportMetrics() const {
+MetricsReporter::~MetricsReporter() {
+  // If we are configured to report metrics, do one final report at the end.
   if (config_.dump_to_logcat) {
     LOG_STREAM(INFO) << "\n*** ART internal metrics ***\n\n";
     // LOG_STREAM(INFO) destroys the stream at the end of the statement, which makes it tricky pass
@@ -188,7 +132,7 @@ void MetricsReporter::ReportMetrics() const {
     // dump the metrics.
     [this](std::ostream& os) {
       StreamBackend backend{os};
-      runtime_->GetMetrics()->ReportAllMetrics(&backend);
+      metrics_->ReportAllMetrics(&backend);
     }(LOG_STREAM(INFO));
     LOG_STREAM(INFO) << "\n*** Done dumping ART internal metrics ***\n";
   }
diff --git a/runtime/metrics/metrics.h b/runtime/metrics/metrics.h
index 435970e667..7156d576e0 100644
--- a/runtime/metrics/metrics.h
+++ b/runtime/metrics/metrics.h
@@ -21,14 +21,11 @@
 
 #include <array>
 #include <atomic>
-#include <optional>
 #include <ostream>
 #include <string_view>
-#include <thread>
 #include <vector>
 
 #include "android-base/logging.h"
-#include "base/message_queue.h"
 #include "base/time_utils.h"
 
 #pragma clang diagnostic push
@@ -61,9 +58,6 @@
 // per metric.
 
 namespace art {
-
-class Runtime;
-
 namespace metrics {
 
 /**
@@ -345,63 +339,25 @@ class ArtMetrics {
 // Returns a human readable name for the given DatumId.
 std::string DatumName(DatumId datum);
 
-// Defines the set of options for how metrics reporting happens.
 struct ReportingConfig {
-  // Causes metrics to be written to the log, which makes them show up in logcat.
-  bool dump_to_logcat{false};
-
-  // Indicates whether to report the final state of metrics on shutdown.
-  //
-  // Note that reporting only happens if some output, such as logcat, is enabled.
-  bool report_metrics_on_shutdown{true};
-
-  // If set, metrics will be reported every time this many seconds elapses.
-  std::optional<unsigned int> periodic_report_seconds;
-
-  // Returns whether any options are set that enables metrics reporting.
-  constexpr bool ReportingEnabled() const { return dump_to_logcat; }
-
-  // Returns whether any options are set that requires a background reporting thread.
-  constexpr bool BackgroundReportingEnabled() const {
-    return ReportingEnabled() && periodic_report_seconds.has_value();
-  }
+  bool dump_to_logcat;
+  // TODO(eholk): this will grow to support other configurations, such as logging to a file, or
+  // statsd. There will also be options for reporting after a period of time, or at certain events.
 };
 
 // MetricsReporter handles periodically reporting ART metrics.
 class MetricsReporter {
  public:
   // Creates a MetricsReporter instance that matches the options selected in ReportingConfig.
-  static std::unique_ptr<MetricsReporter> Create(ReportingConfig config, Runtime* runtime);
+  static std::unique_ptr<MetricsReporter> Create(ReportingConfig config, const ArtMetrics* metrics);
 
   ~MetricsReporter();
 
-  // Creates and runs the background reporting thread.
-  void StartBackgroundThreadIfNeeded();
-
-  // Sends a request to the background thread to shutdown.
-  void StopBackgroundThreadIfRunning();
-
  private:
-  explicit MetricsReporter(ReportingConfig config, Runtime* runtime);
-
-  // The background reporting thread main loop.
-  void BackgroundThreadRun();
-
-  // Calls messages_.SetTimeout if needed.
-  void ResetTimeoutIfNeeded();
-
-  // Outputs the current state of the metrics to the destination set by config_.
-  void ReportMetrics() const;
-
-  const ReportingConfig config_;
-  Runtime* runtime_;
-
-  std::optional<std::thread> thread_;
-
-  // A message indicating that the reporting thread should shut down.
-  struct ShutdownRequestedMessage {};
+  explicit MetricsReporter(ReportingConfig config, const ArtMetrics* metrics);
 
-  MessageQueue<ShutdownRequestedMessage> messages_;
+  ReportingConfig config_;
+  const ArtMetrics* metrics_;
 };
 
 
diff --git a/runtime/parsed_options.cc b/runtime/parsed_options.cc
index b05d0f9ce0..98014da4dd 100644
--- a/runtime/parsed_options.cc
+++ b/runtime/parsed_options.cc
@@ -389,11 +389,6 @@ std::unique_ptr<RuntimeParser> ParsedOptions::MakeParser(bool ignore_unrecognize
           .IntoKey(M::UseStderrLogger)
       .Define("-Xwrite-metrics-to-log")
           .IntoKey(M::WriteMetricsToLog)
-      .Define("-Xdisable-final-metrics-report")
-          .IntoKey(M::DisableFinalMetricsReport)
-      .Define("-Xmetrics-reporting-period=_")
-          .WithType<unsigned int>()
-          .IntoKey(M::MetricsReportingPeriod)
       .Define("-Xonly-use-system-oat-files")
           .IntoKey(M::OnlyUseSystemOatFiles)
       .Define("-Xverifier-logging-threshold=_")
diff --git a/runtime/runtime.cc b/runtime/runtime.cc
index c6769bfae5..ac3c39219e 100644
--- a/runtime/runtime.cc
+++ b/runtime/runtime.cc
@@ -227,10 +227,6 @@ metrics::ReportingConfig ParseMetricsReportingConfig(const RuntimeArgumentMap& a
   using M = RuntimeArgumentMap;
   return {
       .dump_to_logcat = args.Exists(M::WriteMetricsToLog),
-      .report_metrics_on_shutdown = !args.Exists(M::DisableFinalMetricsReport),
-      .periodic_report_seconds{args.Exists(M::MetricsReportingPeriod)
-                                   ? std::make_optional(args.GetOrDefault(M::MetricsReportingPeriod))
-                                   : std::nullopt},
   };
 }
 
@@ -451,9 +447,6 @@ Runtime::~Runtime() {
   delete signal_catcher_;
   signal_catcher_ = nullptr;
 
-  // Shutdown metrics reporting.
-  metrics_reporter_.reset();
-
   // Make sure all other non-daemon threads have terminated, and all daemon threads are suspended.
   // Also wait for daemon threads to quiesce, so that in addition to being "suspended", they
   // no longer access monitor and thread list data structures. We leak user daemon threads
@@ -1080,10 +1073,6 @@ void Runtime::InitNonZygoteOrPostFork(
   // before fork aren't attributed to an app.
   heap_->ResetGcPerformanceInfo();
 
-  if (metrics_reporter_) {
-    metrics_reporter_->StartBackgroundThreadIfNeeded();
-  }
-
   StartSignalCatcher();
 
   ScopedObjectAccess soa(Thread::Current());
@@ -1727,7 +1716,8 @@ bool Runtime::Init(RuntimeArgumentMap&& runtime_options_in) {
   // Class-roots are setup, we can now finish initializing the JniIdManager.
   GetJniIdManager()->Init(self);
 
-  InitMetrics(runtime_options);
+  metrics_reporter_ =
+      metrics::MetricsReporter::Create(ParseMetricsReportingConfig(runtime_options), &metrics_);
 
   // Runtime initialization is largely done now.
   // We load plugins first since that can modify the runtime state slightly.
@@ -1827,13 +1817,6 @@ bool Runtime::Init(RuntimeArgumentMap&& runtime_options_in) {
   return true;
 }
 
-void Runtime::InitMetrics(const RuntimeArgumentMap& runtime_options) {
-  auto metrics_config = ParseMetricsReportingConfig(runtime_options);
-  if (metrics_config.ReportingEnabled()) {
-    metrics_reporter_ = metrics::MetricsReporter::Create(metrics_config, this);
-  }
-}
-
 bool Runtime::EnsurePluginLoaded(const char* plugin_name, std::string* error_msg) {
   // Is the plugin already loaded?
   for (const Plugin& p : plugins_) {
diff --git a/runtime/runtime.h b/runtime/runtime.h
index 06f05a177e..7fd731eb0d 100644
--- a/runtime/runtime.h
+++ b/runtime/runtime.h
@@ -980,7 +980,6 @@ class Runtime {
       SHARED_TRYLOCK_FUNCTION(true, Locks::mutator_lock_);
   void InitNativeMethods() REQUIRES(!Locks::mutator_lock_);
   void RegisterRuntimeNativeMethods(JNIEnv* env);
-  void InitMetrics(const RuntimeArgumentMap& runtime_options);
 
   void StartDaemonThreads();
   void StartSignalCatcher();
diff --git a/runtime/runtime_options.def b/runtime/runtime_options.def
index ba208e34cb..441b2f21f5 100644
--- a/runtime/runtime_options.def
+++ b/runtime/runtime_options.def
@@ -183,7 +183,5 @@ RUNTIME_OPTIONS_KEY (bool,                PerfettoHprof,                  false)
 
 // Whether to dump ART metrics to logcat
 RUNTIME_OPTIONS_KEY (Unit,                WriteMetricsToLog)
-RUNTIME_OPTIONS_KEY (Unit,                DisableFinalMetricsReport)
-RUNTIME_OPTIONS_KEY (unsigned int,        MetricsReportingPeriod)
 
 #undef RUNTIME_OPTIONS_KEY
diff --git a/test/2233-metrics-background-thread/check b/test/2233-metrics-background-thread/check
deleted file mode 100755
index f03ca79857..0000000000
--- a/test/2233-metrics-background-thread/check
+++ /dev/null
@@ -1,45 +0,0 @@
-#!/bin/bash
-#
-# 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.
-
-# Inputs:
-# $1: Test's expected standard output
-# $2: Test's actual standard output
-# $3: Test's expected standard error
-# $4: Test's actual standard error
-
-# Check that one of the metrics appears in stderr.
-grep 'Metrics reporting thread started' "$4" >/dev/null
-THREAD_STARTED=$?
-
-grep 'Timer expired, reporting metrics' "$4" >/dev/null
-METRICS_REPORTED=$?
-
-if [[ $THREAD_START -ne 0 ]] ; then
-  # Print out the log and return with error.
-  echo Metrics reporting thread did not start.
-  cat "$4"
-  exit 1
-fi
-
-if [[ $METRICS_REPORTED -ne 0 ]] ; then
-  # Print out the log and return with error.
-  echo Did not detect metrics report.
-  cat "$4"
-  exit 1
-fi
-
-# Success.
-exit 0
diff --git a/test/2233-metrics-background-thread/expected-stderr.txt b/test/2233-metrics-background-thread/expected-stderr.txt
deleted file mode 100644
index e69de29bb2..0000000000
--- a/test/2233-metrics-background-thread/expected-stderr.txt
+++ /dev/null
diff --git a/test/2233-metrics-background-thread/expected-stdout.txt b/test/2233-metrics-background-thread/expected-stdout.txt
deleted file mode 100644
index e69de29bb2..0000000000
--- a/test/2233-metrics-background-thread/expected-stdout.txt
+++ /dev/null
diff --git a/test/2233-metrics-background-thread/info.txt b/test/2233-metrics-background-thread/info.txt
deleted file mode 100644
index c831b35f78..0000000000
--- a/test/2233-metrics-background-thread/info.txt
+++ /dev/null
@@ -1 +0,0 @@
-Tests metrics background reporting thread
diff --git a/test/2233-metrics-background-thread/run b/test/2233-metrics-background-thread/run
deleted file mode 100755
index 20ec96aa10..0000000000
--- a/test/2233-metrics-background-thread/run
+++ /dev/null
@@ -1,22 +0,0 @@
-#!/bin/bash
-#
-# 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.
-
-export ANDROID_LOG_TAGS="*:d"
-exec ${RUN} $@ \
-    --external-log-tags \
-    --runtime-option -Xwrite-metrics-to-log \
-    --runtime-option -Xmetrics-reporting-period=1 \
-    --runtime-option -Xdisable-final-metrics-report \
diff --git a/test/2233-metrics-background-thread/src/Main.java b/test/2233-metrics-background-thread/src/Main.java
deleted file mode 100644
index 30b25baef9..0000000000
--- a/test/2233-metrics-background-thread/src/Main.java
+++ /dev/null
@@ -1,22 +0,0 @@
-/*
- * 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.
- */
-
-public class Main {
-  public static void main(String[] args) throws InterruptedException {
-    // Sleep for 1.5 seconds to give the metrics reporting thread a chance to report.
-    Thread.sleep(1500);
-  }
-}
diff --git a/test/knownfailures.json b/test/knownfailures.json
index b909d7dc31..385db458a9 100644
--- a/test/knownfailures.json
+++ b/test/knownfailures.json
@@ -1351,8 +1351,7 @@
         "description": ["Failing on RI. Needs further investigating."]
     },
     {
-        "tests": ["2232-write-metrics-to-log",
-                  "2233-metrics-background-thread"],
+        "tests": ["2232-write-metrics-to-log"],
         "variant": "jvm",
         "description": ["RI does not support ART metrics."]
     },