Revert^2 "[metrics] Always enable background thread"
Having the thread enabled in some cases and not in others was causing
issues this tests. To avoid needing to handle multiple configurations,
this CL enables the background reporting thread always. In general,
metrics reporting will be enabled, so this better reflects the default
state of things.
This reverts commit 3f64940d7c6ce4d3f06ea5dbe195c244ca7a8fbc.
Reason for revert: Deflaking tests
The issue was that sometimes the metrics background thread would start
late and get captured by the thread life cycle callbacks instead of
the correct thread. The fix is to request a synchronous metrics report
before starting the test, which forces us to wait until the metrics
thread is up and running.
Test: ./test.py --run-test --host
Test: art_runtime_tests \
--gtest_filter="ThreadLifecycleCallbackRuntimeCallbacksTest.*"
Bug: 170149255
Change-Id: I007437abd1a3404960f79cf1596c95b8da917286
diff --git a/runtime/metrics/reporter.cc b/runtime/metrics/reporter.cc
index 92ae7ab..5148b20 100644
--- a/runtime/metrics/reporter.cc
+++ b/runtime/metrics/reporter.cc
@@ -49,9 +49,6 @@
}
bool MetricsReporter::MaybeStartBackgroundThread(SessionData session_data) {
- if (!config_.ReportingEnabled()) {
- return false;
- }
CHECK(!thread_.has_value());
thread_.emplace(&MetricsReporter::BackgroundThreadRun, this);
messages_.SendMessage(BeginSessionMessage{session_data});
diff --git a/runtime/metrics/reporter.h b/runtime/metrics/reporter.h
index f2f7af5..eff6e47 100644
--- a/runtime/metrics/reporter.h
+++ b/runtime/metrics/reporter.h
@@ -46,11 +46,6 @@
// 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 || dump_to_file.has_value() || dump_to_statsd;
- }
};
// MetricsReporter handles periodically reporting ART metrics.
diff --git a/runtime/runtime_callbacks_test.cc b/runtime/runtime_callbacks_test.cc
index 7d4f26c..e49deae 100644
--- a/runtime/runtime_callbacks_test.cc
+++ b/runtime/runtime_callbacks_test.cc
@@ -118,6 +118,10 @@
struct Callback : public ThreadLifecycleCallback {
void ThreadStart(Thread* self) override {
+ {
+ ScopedObjectAccess soa(self);
+ LOG(DEBUG) << "ThreadStart callback for thread: " << self->GetThreadName();
+ }
if (state == CallbackState::kBase) {
state = CallbackState::kStarted;
stored_self = self;
@@ -127,6 +131,10 @@
}
void ThreadDeath(Thread* self) override {
+ {
+ ScopedObjectAccess soa(self);
+ LOG(DEBUG) << "ThreadDeath callback for thread: " << self->GetThreadName();
+ }
if (state == CallbackState::kStarted && self == stored_self) {
state = CallbackState::kDied;
} else {
@@ -150,6 +158,10 @@
// Make sure the workers are done starting so we don't get callbacks for them.
runtime_->WaitForThreadPoolWorkersToStart();
+ // The metrics reporting thread will sometimes be slow to start. Synchronously requesting a
+ // metrics report forces us to wait until the thread has started.
+ runtime_->RequestMetricsReport(/*synchronous=*/true);
+
cb_.state = CallbackState::kBase; // Ignore main thread attach.
{
@@ -187,7 +199,7 @@
env->CallVoidMethod(thread.get(), join_id);
ASSERT_FALSE(env->ExceptionCheck());
- EXPECT_TRUE(cb_.state == CallbackState::kDied) << static_cast<int>(cb_.state);
+ EXPECT_EQ(cb_.state, CallbackState::kDied);
}
TEST_F(ThreadLifecycleCallbackRuntimeCallbacksTest, ThreadLifecycleCallbackAttach) {
diff --git a/test/911-get-stack-trace/src/art/PrintThread.java b/test/911-get-stack-trace/src/art/PrintThread.java
index 94f3a33..f843cc3 100644
--- a/test/911-get-stack-trace/src/art/PrintThread.java
+++ b/test/911-get-stack-trace/src/art/PrintThread.java
@@ -42,7 +42,8 @@
// may not exist depending on the environment.
public final static String IGNORE_THREAD_NAME_REGEX =
"Binder:|RenderThread|hwuiTask|Jit thread pool worker|Instr:|JDWP|Profile Saver|main|" +
- "queued-work-looper|InstrumentationConnectionThread|intel_svc_streamer_thread|ForkJoinPool";
+ "queued-work-looper|InstrumentationConnectionThread|intel_svc_streamer_thread|" +
+ "ForkJoinPool|Metrics Background Reporting Thread";
public final static Matcher IGNORE_THREADS =
Pattern.compile(IGNORE_THREAD_NAME_REGEX).matcher("");
diff --git a/test/925-threadgroups/expected-stdout.txt b/test/925-threadgroups/expected-stdout.txt
index 9dfa37d..3cc36f2 100644
--- a/test/925-threadgroups/expected-stdout.txt
+++ b/test/925-threadgroups/expected-stdout.txt
@@ -12,7 +12,7 @@
[Thread[main,5,main]]
[]
system:
- [Thread[FinalizerDaemon,5,system], Thread[FinalizerWatchdogDaemon,5,system], Thread[HeapTaskDaemon,5,system], Thread[ReferenceQueueDaemon,5,system], Thread[Signal Catcher,5,system]]
+ [Thread[FinalizerDaemon,5,system], Thread[FinalizerWatchdogDaemon,5,system], Thread[HeapTaskDaemon,5,system], Thread[Metrics Background Reporting Thread,5,system], Thread[ReferenceQueueDaemon,5,system], Thread[Signal Catcher,5,system]]
[java.lang.ThreadGroup[name=main,maxpri=10]]
art.Test925$CustomThreadGroup[name=TEST GROUP,maxpri=10]
java.lang.ThreadGroup[name=main,maxpri=10]
diff --git a/test/925-threadgroups/src/art/Test925.java b/test/925-threadgroups/src/art/Test925.java
index a63f4ce..78068ae 100644
--- a/test/925-threadgroups/src/art/Test925.java
+++ b/test/925-threadgroups/src/art/Test925.java
@@ -47,7 +47,7 @@
printThreadGroupInfo(curGroup);
printThreadGroupInfo(rootGroup);
- waitGroupChildren(rootGroup, 5 /* # daemons */, 30 /* timeout in seconds */);
+ waitGroupChildren(rootGroup, 6 /* # daemons */, 30 /* timeout in seconds */);
checkChildren(curGroup);