Avoid copying Transaction objects unneccessarily.
Flag: EXEMPT refactor
Bug: 385156191
Test: presubmit
Change-Id: Ibd9d64bd7d41adbf5af0dacd660b6aaed6bc8741
diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp
index 1aae13c..5b0f21d 100644
--- a/libs/gui/BLASTBufferQueue.cpp
+++ b/libs/gui/BLASTBufferQueue.cpp
@@ -1032,7 +1032,7 @@
// 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::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 65313c0..359a528 100644
--- a/libs/gui/SurfaceComposerClient.cpp
+++ b/libs/gui/SurfaceComposerClient.cpp
@@ -828,11 +828,10 @@
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 db1b9fb..c69b0a7 100644
--- a/libs/gui/include/gui/BLASTBufferQueue.h
+++ b/libs/gui/include/gui/BLASTBufferQueue.h
@@ -284,7 +284,7 @@
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 15e3341..668bd6f 100644
--- a/libs/gui/include/gui/SurfaceComposerClient.h
+++ b/libs/gui/include/gui/SurfaceComposerClient.h
@@ -443,7 +443,7 @@
virtual ~PresentationCallbackRAII();
};
- class Transaction : public Parcelable {
+ class Transaction {
private:
static sp<IBinder> sApplyToken;
static std::mutex sApplyTokenMutex;
@@ -464,19 +464,20 @@
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 4358227..79124f3 100644
--- a/libs/gui/include/gui/TransactionState.h
+++ b/libs/gui/include/gui/TransactionState.h
@@ -26,7 +26,8 @@
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 @@
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 46b98f9..192602d 100644
--- a/services/surfaceflinger/tests/DereferenceSurfaceControl_test.cpp
+++ b/services/surfaceflinger/tests/DereferenceSurfaceControl_test.cpp
@@ -52,6 +52,8 @@
};
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, 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 18bd3b9..94cb878 100644
--- a/services/surfaceflinger/tests/IPC_test.cpp
+++ b/services/surfaceflinger/tests/IPC_test.cpp
@@ -42,14 +42,22 @@
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 6a5ac2a..1395fb6 100644
--- a/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp
+++ b/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp
@@ -78,7 +78,7 @@
}
};
- 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 @@
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()) {