commit 162305aaceca5f6cbaa03db1aa124f67e313612e
author David James <davidjames@google.com> Sun Apr 07 04:27:56 2019 +0000
committer David James <davidjames@google.com> Sun Apr 07 04:28:50 2019 +0000
tree 5a3490b906bf2ab6bf2fb8b0eb7f8c505b178d5d
parent 5ff61f32a78dca0c722c8ac2685874a26190ceed

Revert "Fix lifecycle issue in CommonPool"

This reverts commit 5ff61f32a78dca0c722c8ac2685874a26190ceed.

Reason for revert: causes global presubmit to be very flaky, see b/130081457
Fixes: b/130081457

Change-Id: I93aa5d6686cd6b5bf831766c9e47c291749526a9

libs/hwui/tests/unit/CommonPoolTests.cpp

diff --git a/libs/hwui/tests/unit/CommonPoolTests.cpp b/libs/hwui/tests/unit/CommonPoolTests.cpp
index 9c885ef..c564ed6 100644
--- a/libs/hwui/tests/unit/CommonPoolTests.cpp
+++ b/libs/hwui/tests/unit/CommonPoolTests.cpp
@@ -135,127 +135,4 @@
     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

libs/hwui/thread/CommonPool.h

diff --git a/libs/hwui/thread/CommonPool.h b/libs/hwui/thread/CommonPool.h
index 5162825..aef2990 100644
--- a/libs/hwui/thread/CommonPool.h
+++ b/libs/hwui/thread/CommonPool.h
@@ -57,13 +57,11 @@
         mHead = newHead;
     }
 
-    constexpr T pop() {
+    constexpr T&& pop() {
         LOG_ALWAYS_FATAL_IF(mTail == mHead, "empty");
         int index = mTail;
         mTail = (mTail + 1) % SIZE;
-        T ret = std::move(mBuffer[index]);
-        mBuffer[index] = nullptr;
-        return ret;
+        return std::move(mBuffer[index]);
     }
 
 private: