3 files changed, 32 insertions, 23 deletions

diff --git a/services/surfaceflinger/TimeStats/TimeStats.cpp b/services/surfaceflinger/TimeStats/TimeStats.cpp
index 12c98da663..130e99a514 100644
--- a/services/surfaceflinger/TimeStats/TimeStats.cpp
+++ b/services/surfaceflinger/TimeStats/TimeStats.cpp
@@ -40,13 +40,18 @@ namespace impl {
 status_pull_atom_return_t TimeStats::pullAtomCallback(int32_t atom_tag,
                                                       pulled_stats_event_list* data, void* cookie) {
     impl::TimeStats* timeStats = reinterpret_cast<impl::TimeStats*>(cookie);
+    status_pull_atom_return_t result = STATS_PULL_SKIP;
     if (atom_tag == android::util::SURFACEFLINGER_STATS_GLOBAL_INFO) {
-        return timeStats->populateGlobalAtom(data);
+        result = timeStats->populateGlobalAtom(data);
     } else if (atom_tag == android::util::SURFACEFLINGER_STATS_LAYER_INFO) {
-        return timeStats->populateLayerAtom(data);
+        result = timeStats->populateLayerAtom(data);
     }
 
-    return STATS_PULL_SKIP;
+    // Enable timestats now. The first full pull for a given build is expected to
+    // have empty or very little stats, as stats are first enabled after the
+    // first pull is completed for either the global or layer stats.
+    timeStats->enable();
+    return result;
 }
 
 status_pull_atom_return_t TimeStats::populateGlobalAtom(pulled_stats_event_list* data) {
@@ -167,13 +172,19 @@ TimeStats::TimeStats(std::unique_ptr<StatsEventDelegate> statsDelegate,
     }
 }
 
+TimeStats::~TimeStats() {
+    std::lock_guard<std::mutex> lock(mMutex);
+    mStatsDelegate->unregisterStatsPullAtomCallback(
+            android::util::SURFACEFLINGER_STATS_GLOBAL_INFO);
+    mStatsDelegate->unregisterStatsPullAtomCallback(android::util::SURFACEFLINGER_STATS_LAYER_INFO);
+}
+
 void TimeStats::onBootFinished() {
-    // Temporarily enable TimeStats by default. Telemetry is disabled while
-    // we move onto statsd, so TimeStats is currently not exercised at all
-    // during testing without enabling by default.
-    // TODO: remove this, as we should only be paying this overhead on devices
-    // where statsd exists.
-    enable();
+    std::lock_guard<std::mutex> lock(mMutex);
+    mStatsDelegate->registerStatsPullAtomCallback(android::util::SURFACEFLINGER_STATS_GLOBAL_INFO,
+                                                  TimeStats::pullAtomCallback, nullptr, this);
+    mStatsDelegate->registerStatsPullAtomCallback(android::util::SURFACEFLINGER_STATS_LAYER_INFO,
+                                                  TimeStats::pullAtomCallback, nullptr, this);
 }
 
 void TimeStats::parseArgs(bool asProto, const Vector<String16>& args, std::string& result) {
@@ -735,10 +746,6 @@ void TimeStats::enable() {
     mEnabled.store(true);
     mTimeStats.statsStart = static_cast<int64_t>(std::time(0));
     mPowerTime.prevTime = systemTime();
-    mStatsDelegate->registerStatsPullAtomCallback(android::util::SURFACEFLINGER_STATS_GLOBAL_INFO,
-                                                  TimeStats::pullAtomCallback, nullptr, this);
-    mStatsDelegate->registerStatsPullAtomCallback(android::util::SURFACEFLINGER_STATS_LAYER_INFO,
-                                                  TimeStats::pullAtomCallback, nullptr, this);
     ALOGD("Enabled");
 }
 
@@ -751,9 +758,6 @@ void TimeStats::disable() {
     flushPowerTimeLocked();
     mEnabled.store(false);
     mTimeStats.statsEnd = static_cast<int64_t>(std::time(0));
-    mStatsDelegate->unregisterStatsPullAtomCallback(
-            android::util::SURFACEFLINGER_STATS_GLOBAL_INFO);
-    mStatsDelegate->unregisterStatsPullAtomCallback(android::util::SURFACEFLINGER_STATS_LAYER_INFO);
     ALOGD("Disabled");
 }
 
diff --git a/services/surfaceflinger/TimeStats/TimeStats.h b/services/surfaceflinger/TimeStats/TimeStats.h
index 71f06af794..67b9d1055c 100644
--- a/services/surfaceflinger/TimeStats/TimeStats.h
+++ b/services/surfaceflinger/TimeStats/TimeStats.h
@@ -182,6 +182,8 @@ public:
               std::optional<size_t> maxPulledLayers,
               std::optional<size_t> maxPulledHistogramBuckets);
 
+    ~TimeStats() override;
+
     void onBootFinished() override;
     void parseArgs(bool asProto, const Vector<String16>& args, std::string& result) override;
     bool isEnabled() override;
diff --git a/services/surfaceflinger/tests/unittests/TimeStatsTest.cpp b/services/surfaceflinger/tests/unittests/TimeStatsTest.cpp
index 68e6697ce0..f65af77748 100644
--- a/services/surfaceflinger/tests/unittests/TimeStatsTest.cpp
+++ b/services/surfaceflinger/tests/unittests/TimeStatsTest.cpp
@@ -258,22 +258,25 @@ TEST_F(TimeStatsTest, disabledByDefault) {
     ASSERT_FALSE(mTimeStats->isEnabled());
 }
 
-TEST_F(TimeStatsTest, enabledAfterBoot) {
+TEST_F(TimeStatsTest, registersCallbacksAfterBoot) {
     mTimeStats->onBootFinished();
-    ASSERT_TRUE(mTimeStats->isEnabled());
-}
-
-TEST_F(TimeStatsTest, canEnableAndDisableTimeStats) {
-    EXPECT_TRUE(inputCommand(InputCommand::ENABLE, FMT_STRING).empty());
-    ASSERT_TRUE(mTimeStats->isEnabled());
     EXPECT_THAT(mDelegate->mAtomTags,
                 UnorderedElementsAre(android::util::SURFACEFLINGER_STATS_GLOBAL_INFO,
                                      android::util::SURFACEFLINGER_STATS_LAYER_INFO));
+}
 
+TEST_F(TimeStatsTest, unregistersCallbacksOnDestruction) {
     EXPECT_CALL(*mDelegate,
                 unregisterStatsPullAtomCallback(android::util::SURFACEFLINGER_STATS_GLOBAL_INFO));
     EXPECT_CALL(*mDelegate,
                 unregisterStatsPullAtomCallback(android::util::SURFACEFLINGER_STATS_LAYER_INFO));
+    mTimeStats.reset();
+}
+
+TEST_F(TimeStatsTest, canEnableAndDisableTimeStats) {
+    EXPECT_TRUE(inputCommand(InputCommand::ENABLE, FMT_STRING).empty());
+    ASSERT_TRUE(mTimeStats->isEnabled());
+
     EXPECT_TRUE(inputCommand(InputCommand::DISABLE, FMT_STRING).empty());
     ASSERT_FALSE(mTimeStats->isEnabled());
 }