Avoid copying Transaction objects unneccessarily.

Flag: EXEMPT refactor
Bug: 385156191
Test: presubmit
Change-Id: Ibd9d64bd7d41adbf5af0dacd660b6aaed6bc8741

8 files changed, 35 insertions, 19 deletions

diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp
index 1aae13c1f4..5b0f21de91 100644
--- a/libs/gui/BLASTBufferQueue.cpp
+++ b/libs/gui/BLASTBufferQueue.cpp
@@ -1032,7 +1032,7 @@ void BLASTBufferQueue::mergeWithNextTransaction(SurfaceComposerClient::Transacti
         // Apply the transaction since we have already acquired the desired frame.
         t->setApplyToken(mApplyToken).apply();
     } else {
-        mPendingTransactions.emplace_back(frameNumber, *t);
+        mPendingTransactions.emplace_back(frameNumber, std::move(*t));
         // Clear the transaction so it can't be applied elsewhere.
         t->clear();
     }
@@ -1050,8 +1050,8 @@ void BLASTBufferQueue::applyPendingTransactions(uint64_t frameNumber) {
 void BLASTBufferQueue::mergePendingTransactions(SurfaceComposerClient::Transaction* t,
                                                 uint64_t frameNumber) {
     auto mergeTransaction =
-            [&t, currentFrameNumber = frameNumber](
-                    std::tuple<uint64_t, SurfaceComposerClient::Transaction> pendingTransaction) {
+            [t, currentFrameNumber = frameNumber](
+                    std::pair<uint64_t, SurfaceComposerClient::Transaction>& pendingTransaction) {
                 auto& [targetFrameNumber, transaction] = pendingTransaction;
                 if (currentFrameNumber < targetFrameNumber) {
                     return false;
diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp
index 65313c0fac..359a5288a6 100644
--- a/libs/gui/SurfaceComposerClient.cpp
+++ b/libs/gui/SurfaceComposerClient.cpp
@@ -828,11 +828,10 @@ SurfaceComposerClient::Transaction::Transaction() {
     mTransactionCompletedListener = TransactionCompletedListener::getInstance();
 }
 
-SurfaceComposerClient::Transaction::Transaction(const Transaction& other) {
-    mState = other.mState;
-    mListenerCallbacks = other.mListenerCallbacks;
-    mTransactionCompletedListener = TransactionCompletedListener::getInstance();
-}
+SurfaceComposerClient::Transaction::Transaction(Transaction&& other)
+      : mTransactionCompletedListener(TransactionCompletedListener::getInstance()),
+        mState(std::move(other.mState)),
+        mListenerCallbacks(std::move(other.mListenerCallbacks)) {}
 
 void SurfaceComposerClient::Transaction::sanitize(int pid, int uid) {
     uint32_t permissions = LayerStatePermissions::getTransactionPermissions(pid, uid);
diff --git a/libs/gui/include/gui/BLASTBufferQueue.h b/libs/gui/include/gui/BLASTBufferQueue.h
index db1b9fb8eb..c69b0a79ad 100644
--- a/libs/gui/include/gui/BLASTBufferQueue.h
+++ b/libs/gui/include/gui/BLASTBufferQueue.h
@@ -284,7 +284,7 @@ private:
     std::function<void(SurfaceComposerClient::Transaction*)> mTransactionReadyCallback
             GUARDED_BY(mMutex);
     SurfaceComposerClient::Transaction* mSyncTransaction GUARDED_BY(mMutex);
-    std::vector<std::tuple<uint64_t /* framenumber */, SurfaceComposerClient::Transaction>>
+    std::vector<std::pair<uint64_t /* framenumber */, SurfaceComposerClient::Transaction>>
             mPendingTransactions GUARDED_BY(mMutex);
 
     std::queue<std::pair<uint64_t, FrameTimelineInfo>> mPendingFrameTimelines GUARDED_BY(mMutex);
diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h
index 15e3341ca2..668bd6fbb8 100644
--- a/libs/gui/include/gui/SurfaceComposerClient.h
+++ b/libs/gui/include/gui/SurfaceComposerClient.h
@@ -443,7 +443,7 @@ public:
         virtual ~PresentationCallbackRAII();
     };
 
-    class Transaction : public Parcelable {
+    class Transaction {
     private:
         static sp<IBinder> sApplyToken;
         static std::mutex sApplyTokenMutex;
@@ -464,19 +464,20 @@ public:
 
     protected:
         // Accessed in tests.
+        explicit Transaction(Transaction const& other) = default;
         std::unordered_map<sp<ITransactionCompletedListener>, CallbackInfo, TCLHash>
                 mListenerCallbacks;
 
     public:
         Transaction();
-        virtual ~Transaction() = default;
-        Transaction(Transaction const& other);
+        Transaction(Transaction&& other);
+        Transaction& operator=(Transaction&& other) = default;
 
         // Factory method that creates a new Transaction instance from the parcel.
         static std::unique_ptr<Transaction> createFromParcel(const Parcel* parcel);
 
-        status_t writeToParcel(Parcel* parcel) const override;
-        status_t readFromParcel(const Parcel* parcel) override;
+        status_t writeToParcel(Parcel* parcel) const;
+        status_t readFromParcel(const Parcel* parcel);
 
         // Clears the contents of the transaction without applying it.
         void clear();
diff --git a/libs/gui/include/gui/TransactionState.h b/libs/gui/include/gui/TransactionState.h
index 4358227dae..79124f3ed7 100644
--- a/libs/gui/include/gui/TransactionState.h
+++ b/libs/gui/include/gui/TransactionState.h
@@ -26,7 +26,8 @@ namespace android {
 class TransactionState {
 public:
     explicit TransactionState() = default;
-    TransactionState(TransactionState const& other) = default;
+    TransactionState(TransactionState&& other) = default;
+    TransactionState& operator=(TransactionState&& other) = default;
     status_t writeToParcel(Parcel* parcel) const;
     status_t readFromParcel(const Parcel* parcel);
     layer_state_t* getLayerState(const sp<SurfaceControl>& sc);
@@ -86,6 +87,9 @@ public:
     std::vector<ListenerCallbacks> mListenerCallbacks;
 
 private:
+    explicit TransactionState(TransactionState const& other) = default;
+    friend class TransactionApplicationTest;
+    friend class SurfaceComposerClient;
     // We keep track of the last MAX_MERGE_HISTORY_LENGTH merged transaction ids.
     // Ordered most recently merged to least recently merged.
     static constexpr size_t MAX_MERGE_HISTORY_LENGTH = 10u;
diff --git a/services/surfaceflinger/tests/DereferenceSurfaceControl_test.cpp b/services/surfaceflinger/tests/DereferenceSurfaceControl_test.cpp
index 46b98f9193..192602d7d2 100644
--- a/services/surfaceflinger/tests/DereferenceSurfaceControl_test.cpp
+++ b/services/surfaceflinger/tests/DereferenceSurfaceControl_test.cpp
@@ -52,6 +52,8 @@ protected:
 };
 
 TEST_F(DereferenceSurfaceControlTest, LayerNotInTransaction) {
+    // Last strong pointer is removed, the layer is destroyed and is removed
+    // from compostion.
     fgLayer = nullptr;
     {
         SCOPED_TRACE("after setting null");
@@ -61,7 +63,9 @@ TEST_F(DereferenceSurfaceControlTest, LayerNotInTransaction) {
 }
 
 TEST_F(DereferenceSurfaceControlTest, LayerInTransaction) {
-    auto transaction = Transaction().show(fgLayer);
+    Transaction transaction;
+    transaction.show(fgLayer);
+    // |transaction| retains a strong pointer, so layer is retained.
     fgLayer = nullptr;
     {
         SCOPED_TRACE("after setting null");
diff --git a/services/surfaceflinger/tests/IPC_test.cpp b/services/surfaceflinger/tests/IPC_test.cpp
index 18bd3b92d2..94cb878311 100644
--- a/services/surfaceflinger/tests/IPC_test.cpp
+++ b/services/surfaceflinger/tests/IPC_test.cpp
@@ -42,14 +42,22 @@ using CallbackInfo = SurfaceComposerClient::CallbackInfo;
 using TCLHash = SurfaceComposerClient::TCLHash;
 using android::hardware::graphics::common::V1_1::BufferUsage;
 
-class TransactionHelper : public Transaction {
+class TransactionHelper : public Transaction, public Parcelable {
 public:
+    TransactionHelper() : Transaction() {}
     size_t getNumListeners() { return mListenerCallbacks.size(); }
 
     std::unordered_map<sp<ITransactionCompletedListener>, CallbackInfo, TCLHash>
     getListenerCallbacks() {
         return mListenerCallbacks;
     }
+    status_t writeToParcel(Parcel* parcel) const override {
+        return Transaction::writeToParcel(parcel);
+    }
+
+    status_t readFromParcel(const Parcel* parcel) override {
+        return Transaction::readFromParcel(parcel);
+    }
 };
 
 class IPCTestUtils {
diff --git a/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp b/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp
index 6a5ac2a70e..1395fb6af3 100644
--- a/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp
+++ b/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp
@@ -78,7 +78,7 @@ public:
         }
     };
 
-    void checkEqual(TransactionInfo info, QueuedTransactionState state) {
+    void checkEqual(const TransactionInfo& info, const QueuedTransactionState& state) {
         EXPECT_EQ(0u, info.mComposerStates.size());
         EXPECT_EQ(0u, state.states.size());
 
@@ -383,7 +383,7 @@ public:
         EXPECT_TRUE(mFlinger.getTransactionQueue().isEmpty());
         EXPECT_EQ(0u, mFlinger.getPendingTransactionQueue().size());
         std::unordered_set<uint32_t> createdLayers;
-        for (auto transaction : transactions) {
+        for (auto& transaction : transactions) {
             for (auto& state : transaction.mComposerStates) {
                 auto layerId = static_cast<uint32_t>(state.state.layerId);
                 if (createdLayers.find(layerId) == createdLayers.end()) {