From 5ff61f32a78dca0c722c8ac2685874a26190ceed Mon Sep 17 00:00:00 2001
From: John Reck <jreck@google.com>
Date: Thu, 4 Apr 2019 15:49:40 -0700
Subject: [PATCH] Fix lifecycle issue in CommonPool

Destroy things more faster

Fixes: 129250875
Test: hwui_unit_tests
Change-Id: I7e060fcb61f5321dd9e68a3ee4a01868033b3fc3
---
 libs/hwui/tests/unit/CommonPoolTests.cpp | 123 +++++++++++++++++++++++
 libs/hwui/thread/CommonPool.h            |   6 +-
 2 files changed, 127 insertions(+), 2 deletions(-)

diff --git a/libs/hwui/tests/unit/CommonPoolTests.cpp b/libs/hwui/tests/unit/CommonPoolTests.cpp
index c564ed6327869..9c885ef0e29f7 100644
--- a/libs/hwui/tests/unit/CommonPoolTests.cpp
+++ b/libs/hwui/tests/unit/CommonPoolTests.cpp
@@ -135,4 +135,127 @@ TEST(CommonPool, fullQueue) {
     for (auto& f : futures) {
         f.get();
     }
+}
+
+struct DestructorObserver {
+    DestructorObserver(int* destroyCount) : mDestroyCount(destroyCount) {}
+    DestructorObserver(const DestructorObserver& other) : mDestroyCount(other.mDestroyCount) {}
+    DestructorObserver(DestructorObserver&& other) {
+        mDestroyCount = other.mDestroyCount;
+        other.mDestroyCount = nullptr;
+    }
+    ~DestructorObserver() {
+        if (mDestroyCount) {
+            (*mDestroyCount)++;
+        }
+    }
+    DestructorObserver& operator=(DestructorObserver&& other) {
+        mDestroyCount = other.mDestroyCount;
+        other.mDestroyCount = nullptr;
+        return *this;
+    }
+    int* mDestroyCount;
+};
+
+struct CopyObserver {
+    CopyObserver(int* copyCount) : mCopyCount(copyCount) {}
+    CopyObserver(CopyObserver&& other) = default;
+    CopyObserver& operator=(CopyObserver&& other) = default;
+
+    CopyObserver(const CopyObserver& other) {
+        mCopyCount = other.mCopyCount;
+        if (mCopyCount) {
+            (*mCopyCount)++;
+        }
+    }
+
+    CopyObserver& operator=(const CopyObserver& other) {
+        mCopyCount = other.mCopyCount;
+        if (mCopyCount) {
+            (*mCopyCount)++;
+        }
+        return *this;
+    }
+
+    int* mCopyCount;
+};
+
+TEST(CommonPool, asyncLifecycleCheck) {
+    std::vector<std::future<void>> mFrameFences;
+    int destroyCount = 0;
+    int runCount = 0;
+    {
+        DestructorObserver observer{&destroyCount};
+        auto func = [observer = std::move(observer), count = &runCount]() {
+            if (observer.mDestroyCount) {
+                (*count)++;
+            }
+        };
+        mFrameFences.push_back(CommonPool::async(std::move(func)));
+    }
+    for (auto& fence : mFrameFences) {
+        EXPECT_TRUE(fence.valid());
+        fence.get();
+        EXPECT_FALSE(fence.valid());
+    }
+    mFrameFences.clear();
+    EXPECT_EQ(1, runCount);
+    EXPECT_EQ(1, destroyCount);
+}
+
+TEST(CommonPool, asyncCopyCheck) {
+    std::vector<std::future<void>> mFrameFences;
+    int copyCount = 0;
+    int runCount = 0;
+    {
+        CopyObserver observer{&copyCount};
+        auto func = [observer = std::move(observer), count = &runCount]() {
+            if (observer.mCopyCount) {
+                (*count)++;
+            }
+        };
+        mFrameFences.push_back(CommonPool::async(std::move(func)));
+    }
+    for (auto& fence : mFrameFences) {
+        EXPECT_TRUE(fence.valid());
+        fence.get();
+        EXPECT_FALSE(fence.valid());
+    }
+    mFrameFences.clear();
+    EXPECT_EQ(1, runCount);
+    // We expect std::move all the way
+    EXPECT_EQ(0, copyCount);
+}
+
+TEST(CommonPool, syncLifecycleCheck) {
+    int destroyCount = 0;
+    int runCount = 0;
+    {
+        DestructorObserver observer{&destroyCount};
+        auto func = [observer = std::move(observer), count = &runCount]() {
+            if (observer.mDestroyCount) {
+                (*count)++;
+            }
+        };
+        CommonPool::runSync(std::move(func));
+    }
+    EXPECT_EQ(1, runCount);
+    EXPECT_EQ(1, destroyCount);
+}
+
+TEST(CommonPool, syncCopyCheck) {
+    int copyCount = 0;
+    int runCount = 0;
+    {
+        CopyObserver observer{&copyCount};
+        auto func = [observer = std::move(observer), count = &runCount]() {
+            if (observer.mCopyCount) {
+                (*count)++;
+            }
+        };
+        CommonPool::runSync(std::move(func));
+    }
+    EXPECT_EQ(1, runCount);
+    // We expect std::move all the way
+    EXPECT_EQ(0, copyCount);
 }
\ No newline at end of file
diff --git a/libs/hwui/thread/CommonPool.h b/libs/hwui/thread/CommonPool.h
index aef2990d63434..51628259d2a8c 100644
--- a/libs/hwui/thread/CommonPool.h
+++ b/libs/hwui/thread/CommonPool.h
@@ -57,11 +57,13 @@ public:
         mHead = newHead;
     }
 
-    constexpr T&& pop() {
+    constexpr T pop() {
         LOG_ALWAYS_FATAL_IF(mTail == mHead, "empty");
         int index = mTail;
         mTail = (mTail + 1) % SIZE;
-        return std::move(mBuffer[index]);
+        T ret = std::move(mBuffer[index]);
+        mBuffer[index] = nullptr;
+        return ret;
     }
 
 private: