commit 46471e6147246d31aaadcd42c2e9bb10ae09d41c
author Dominik Laskowski <domlaskowski@google.com> Fri Jan 14 15:34:03 2022 -0800
committer Dominik Laskowski <domlaskowski@google.com> Fri Jan 14 16:35:48 2022 -0800
tree b4cc46f1acf2cfbd76ed4042d3d0c6e8d2296469
parent bd2fbcb8f8b568882eac6e6cb2839c2915270df7

SF: Use RAII for TransactionTracing

Ensure the thread joins on destruction, which had been suppressed by a
leak fixed by ag/16613874.

Bug: 214486764
Test: libsurfaceflinger_arc_test
Test: libsurfaceflinger_unittest
Change-Id: Ib766735dc51e89d63080dd6b9afe8464da7c1c6a

services/surfaceflinger/Tracing/TransactionTracing.cpp

diff --git a/services/surfaceflinger/Tracing/TransactionTracing.cpp b/services/surfaceflinger/Tracing/TransactionTracing.cpp
index b5966d5..7d75718 100644
--- a/services/surfaceflinger/Tracing/TransactionTracing.cpp
+++ b/services/surfaceflinger/Tracing/TransactionTracing.cpp
@@ -23,35 +23,22 @@
 #include <utils/SystemClock.h>
 #include <utils/Trace.h>
 
-#include "RingBuffer.h"
 #include "TransactionTracing.h"
 
 namespace android {
 
 TransactionTracing::TransactionTracing() {
-    mBuffer = std::make_unique<
-            RingBuffer<proto::TransactionTraceFile, proto::TransactionTraceEntry>>();
-}
-
-TransactionTracing::~TransactionTracing() = default;
-
-bool TransactionTracing::enable() {
     std::scoped_lock lock(mTraceLock);
-    if (mEnabled) {
-        return false;
-    }
-    mBuffer->setSize(mBufferSizeInBytes);
+
+    mBuffer.setSize(mBufferSizeInBytes);
     mStartingTimestamp = systemTime();
-    mEnabled = true;
     {
         std::scoped_lock lock(mMainThreadLock);
-        mDone = false;
         mThread = std::thread(&TransactionTracing::loop, this);
     }
-    return true;
 }
 
-bool TransactionTracing::disable() {
+TransactionTracing::~TransactionTracing() {
     std::thread thread;
     {
         std::scoped_lock lock(mMainThreadLock);
@@ -63,43 +50,20 @@
         thread.join();
     }
 
-    std::scoped_lock lock(mTraceLock);
-    if (!mEnabled) {
-        return false;
-    }
-    mEnabled = false;
-
-    writeToFileLocked();
-    mBuffer->reset();
-    mQueuedTransactions.clear();
-    mStartingStates.clear();
-    mLayerHandles.clear();
-    return true;
-}
-
-bool TransactionTracing::isEnabled() const {
-    std::scoped_lock lock(mTraceLock);
-    return mEnabled;
+    writeToFile();
 }
 
 status_t TransactionTracing::writeToFile() {
     std::scoped_lock lock(mTraceLock);
-    if (!mEnabled) {
-        return STATUS_OK;
-    }
-    return writeToFileLocked();
-}
-
-status_t TransactionTracing::writeToFileLocked() {
     proto::TransactionTraceFile fileProto = createTraceFileProto();
     addStartingStateToProtoLocked(fileProto);
-    return mBuffer->writeToFile(fileProto, FILE_NAME);
+    return mBuffer.writeToFile(fileProto, FILE_NAME);
 }
 
 void TransactionTracing::setBufferSize(size_t bufferSizeInBytes) {
     std::scoped_lock lock(mTraceLock);
     mBufferSizeInBytes = bufferSizeInBytes;
-    mBuffer->setSize(mBufferSizeInBytes);
+    mBuffer.setSize(mBufferSizeInBytes);
 }
 
 proto::TransactionTraceFile TransactionTracing::createTraceFileProto() const {
@@ -111,21 +75,16 @@
 
 void TransactionTracing::dump(std::string& result) const {
     std::scoped_lock lock(mTraceLock);
-    base::StringAppendF(&result, "Transaction tracing state: %s\n",
-                        mEnabled ? "enabled" : "disabled");
     base::StringAppendF(&result,
                         "  queued transactions=%zu created layers=%zu handles=%zu states=%zu\n",
                         mQueuedTransactions.size(), mCreatedLayers.size(), mLayerHandles.size(),
                         mStartingStates.size());
-    mBuffer->dump(result);
+    mBuffer.dump(result);
 }
 
 void TransactionTracing::addQueuedTransaction(const TransactionState& transaction) {
     std::scoped_lock lock(mTraceLock);
     ATRACE_CALL();
-    if (!mEnabled) {
-        return;
-    }
     mQueuedTransactions[transaction.id] =
             TransactionProtoParser::toProto(transaction,
                                             std::bind(&TransactionTracing::getLayerIdLocked, this,
@@ -206,7 +165,7 @@
         std::string serializedProto;
         entryProto.SerializeToString(&serializedProto);
         entryProto.Clear();
-        std::vector<std::string> entries = mBuffer->emplace(std::move(serializedProto));
+        std::vector<std::string> entries = mBuffer.emplace(std::move(serializedProto));
         removedEntries.reserve(removedEntries.size() + entries.size());
         removedEntries.insert(removedEntries.end(), std::make_move_iterator(entries.begin()),
                               std::make_move_iterator(entries.end()));
@@ -229,10 +188,10 @@
     base::ScopedLockAssertion assumeLocked(mTraceLock);
     mTransactionsAddedToBufferCv.wait(lock, [&]() REQUIRES(mTraceLock) {
         proto::TransactionTraceEntry entry;
-        if (mBuffer->used() > 0) {
-            entry.ParseFromString(mBuffer->back());
+        if (mBuffer.used() > 0) {
+            entry.ParseFromString(mBuffer.back());
         }
-        return mBuffer->used() > 0 && entry.vsync_id() >= vsyncId;
+        return mBuffer.used() > 0 && entry.vsync_id() >= vsyncId;
     });
 }
 
@@ -352,7 +311,7 @@
     std::scoped_lock<std::mutex> lock(mTraceLock);
     proto::TransactionTraceFile proto = createTraceFileProto();
     addStartingStateToProtoLocked(proto);
-    mBuffer->writeToProto(proto);
+    mBuffer.writeToProto(proto);
     return proto;
 }