allow for slow FrameMetricsListeners
A slow listener could cause a race in the NotifyHandler
where the single reference to the buffer to send would get
updated when it shouldn't have been.
Switch to a queue of available buffers to prevent this race.
Also, stop setting and clearing the observer reference and instead
incStrong/decStrong to mark temporary strong ownership without
colliding with other owners in flight.
Bug: 27097094
Change-Id: Iee647bfae8b80019b6d8290179eed3973230901f
diff --git a/libs/hwui/Android.mk b/libs/hwui/Android.mk
index 6a565033..6257122 100644
--- a/libs/hwui/Android.mk
+++ b/libs/hwui/Android.mk
@@ -232,7 +232,6 @@
LOCAL_SRC_FILES += \
$(hwui_test_common_src_files) \
- tests/unit/BufferPoolTests.cpp \
tests/unit/CanvasStateTests.cpp \
tests/unit/ClipAreaTests.cpp \
tests/unit/CrashHandlerInjector.cpp \
diff --git a/libs/hwui/BufferPool.h b/libs/hwui/BufferPool.h
deleted file mode 100644
index 005b399..0000000
--- a/libs/hwui/BufferPool.h
+++ /dev/null
@@ -1,181 +0,0 @@
-/*
- * Copyright (C) 2016 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-#pragma once
-
-#include "utils/RefBase.h"
-#include "utils/Log.h"
-#include "utils/Macros.h"
-
-#include <atomic>
-#include <stdint.h>
-#include <memory>
-#include <mutex>
-
-namespace android {
-namespace uirenderer {
-
-/*
- * Simple thread-safe pool of int64_t arrays of a provided size.
- *
- * Permits allocating a client-provided max number of buffers.
- * If all buffers are in use, refuses to service any more
- * acquire requests until buffers are re-released to the pool.
- */
-class BufferPool : public VirtualLightRefBase {
-public:
- class Buffer {
- PREVENT_COPY_AND_ASSIGN(Buffer);
- public:
- int64_t* getBuffer() { return mBuffer.get(); }
- size_t getSize() { return mSize; }
-
- void release() {
- LOG_ALWAYS_FATAL_IF(mPool.get() == nullptr, "attempt to release unacquired buffer");
- mPool->release(this);
- }
-
- Buffer* incRef() {
- mRefs++;
- return this;
- }
-
- int decRef() {
- int refs = mRefs.fetch_sub(1);
- LOG_ALWAYS_FATAL_IF(refs == 0, "buffer reference decremented below 0");
- return refs - 1;
- }
-
- bool isUniqueRef() {
- return mRefs.load() == 1;
- }
-
- private:
- friend class BufferPool;
-
- Buffer(BufferPool* pool, size_t size) : mRefs(1) {
- mSize = size;
- mBuffer.reset(new int64_t[size]);
- mPool = pool;
- }
-
- void setPool(BufferPool* pool) {
- mPool = pool;
- }
-
- std::unique_ptr<Buffer> mNext;
- std::unique_ptr<int64_t[]> mBuffer;
- sp<BufferPool> mPool;
- size_t mSize;
-
- std::atomic_int mRefs;
- };
-
- BufferPool(size_t bufferSize, size_t count)
- : mBufferSize(bufferSize), mCount(count) {}
-
- /**
- * Acquires a buffer from the buffer pool if available.
- *
- * Only `mCount` buffers are allowed to be in use at a single
- * instance.
- *
- * If no buffer is available, i.e. `mCount` buffers are in use,
- * returns nullptr.
- *
- * The pointer returned from this method *MUST NOT* be freed, instead
- * BufferPool::release() must be called upon it when the client
- * is done with it. Failing to release buffers will eventually make the
- * BufferPool refuse to service any more BufferPool::acquire() requests.
- */
- BufferPool::Buffer* acquire() {
- std::lock_guard<std::mutex> lock(mLock);
-
- if (mHead.get() != nullptr) {
- BufferPool::Buffer* res = mHead.release();
- mHead = std::move(res->mNext);
- res->mNext.reset(nullptr);
- res->setPool(this);
- res->incRef();
- return res;
- }
-
- if (mAllocatedCount < mCount) {
- ++mAllocatedCount;
- return new BufferPool::Buffer(this, mBufferSize);
- }
-
- return nullptr;
- }
-
- /**
- * Releases a buffer previously acquired by BufferPool::acquire().
- *
- * The released buffer is not valid after calling this method and
- * attempting to use will result in undefined behavior.
- */
- void release(BufferPool::Buffer* buffer) {
- std::lock_guard<std::mutex> lock(mLock);
-
- if (buffer->decRef() != 0) {
- return;
- }
-
- buffer->setPool(nullptr);
-
- BufferPool::Buffer* list = mHead.get();
- if (list == nullptr) {
- mHead.reset(buffer);
- mHead->mNext.reset(nullptr);
- return;
- }
-
- while (list->mNext.get() != nullptr) {
- list = list->mNext.get();
- }
-
- list->mNext.reset(buffer);
- }
-
- /*
- * Used for testing.
- */
- size_t getAvailableBufferCount() {
- size_t remainingToAllocateCount = mCount - mAllocatedCount;
-
- BufferPool::Buffer* list = mHead.get();
- if (list == nullptr) return remainingToAllocateCount;
-
- int count = 1;
- while (list->mNext.get() != nullptr) {
- count++;
- list = list->mNext.get();
- }
-
- return count + remainingToAllocateCount;
- }
-
-private:
- mutable std::mutex mLock;
-
- size_t mBufferSize;
- size_t mCount;
- size_t mAllocatedCount = 0;
- std::unique_ptr<BufferPool::Buffer> mHead;
-};
-
-}; // namespace uirenderer
-}; // namespace android
diff --git a/libs/hwui/FrameMetricsObserver.h b/libs/hwui/FrameMetricsObserver.h
index 2b42a80..4f81c86 100644
--- a/libs/hwui/FrameMetricsObserver.h
+++ b/libs/hwui/FrameMetricsObserver.h
@@ -18,14 +18,12 @@
#include <utils/RefBase.h>
-#include "BufferPool.h"
-
namespace android {
namespace uirenderer {
class FrameMetricsObserver : public VirtualLightRefBase {
public:
- virtual void notify(BufferPool::Buffer* buffer, int dropCount);
+ virtual void notify(const int64_t* buffer);
};
}; // namespace uirenderer
diff --git a/libs/hwui/FrameMetricsReporter.h b/libs/hwui/FrameMetricsReporter.h
index 0831d24..c1cd0a92 100644
--- a/libs/hwui/FrameMetricsReporter.h
+++ b/libs/hwui/FrameMetricsReporter.h
@@ -19,7 +19,6 @@
#include <utils/RefBase.h>
#include <utils/Log.h>
-#include "BufferPool.h"
#include "FrameInfo.h"
#include "FrameMetricsObserver.h"
@@ -31,10 +30,7 @@
class FrameMetricsReporter {
public:
- FrameMetricsReporter() {
- mBufferPool = new BufferPool(kBufferSize, kBufferCount);
- LOG_ALWAYS_FATAL_IF(mBufferPool.get() == nullptr, "OOM: unable to allocate buffer pool");
- }
+ FrameMetricsReporter() {}
void addObserver(FrameMetricsObserver* observer) {
mObservers.push_back(observer);
@@ -55,36 +51,13 @@
}
void reportFrameMetrics(const int64_t* stats) {
- BufferPool::Buffer* statsBuffer = mBufferPool->acquire();
-
- if (statsBuffer != nullptr) {
- // copy in frame stats
- memcpy(statsBuffer->getBuffer(), stats, kBufferSize * sizeof(*stats));
-
- // notify on requested threads
- for (size_t i = 0; i < mObservers.size(); i++) {
- mObservers[i]->notify(statsBuffer, mDroppedReports);
- }
-
- // drop our reference
- statsBuffer->release();
- mDroppedReports = 0;
- } else {
- mDroppedReports++;
+ for (size_t i = 0; i < mObservers.size(); i++) {
+ mObservers[i]->notify(stats);
}
}
- int getDroppedReports() { return mDroppedReports; }
-
private:
- static const size_t kBufferCount = 3;
- static const size_t kBufferSize = static_cast<size_t>(FrameInfoIndex::NumIndexes);
-
std::vector< sp<FrameMetricsObserver> > mObservers;
-
- sp<BufferPool> mBufferPool;
-
- int mDroppedReports = 0;
};
}; // namespace uirenderer
diff --git a/libs/hwui/renderthread/CanvasContext.h b/libs/hwui/renderthread/CanvasContext.h
index 1f81970..cb61e51 100644
--- a/libs/hwui/renderthread/CanvasContext.h
+++ b/libs/hwui/renderthread/CanvasContext.h
@@ -159,14 +159,6 @@
}
}
- long getDroppedFrameReportCount() {
- if (mFrameMetricsReporter.get() != nullptr) {
- return mFrameMetricsReporter->getDroppedReports();
- }
-
- return 0;
- }
-
private:
friend class RegisterFrameCallbackTask;
// TODO: Replace with something better for layer & other GL object
diff --git a/libs/hwui/tests/unit/BufferPoolTests.cpp b/libs/hwui/tests/unit/BufferPoolTests.cpp
deleted file mode 100644
index 44e6d3a..0000000
--- a/libs/hwui/tests/unit/BufferPoolTests.cpp
+++ /dev/null
@@ -1,102 +0,0 @@
-/*
- * Copyright (C) 2016 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-#include <gtest/gtest.h>
-
-#include <BufferPool.h>
-#include <utils/StrongPointer.h>
-
-namespace android {
-namespace uirenderer {
-
-TEST(BufferPool, acquireThenRelease) {
- static const int numRuns = 5;
-
- // 10 buffers of size 1
- static const size_t bufferSize = 1;
- static const size_t bufferCount = 10;
- sp<BufferPool> pool = new BufferPool(bufferSize, bufferCount);
-
- for (int run = 0; run < numRuns; run++) {
- BufferPool::Buffer* acquiredBuffers[bufferCount];
- for (size_t i = 0; i < bufferCount; i++) {
- ASSERT_EQ(bufferCount - i, pool->getAvailableBufferCount());
- acquiredBuffers[i] = pool->acquire();
- ASSERT_NE(nullptr, acquiredBuffers[i]);
- ASSERT_TRUE(acquiredBuffers[i]->isUniqueRef());
- }
-
- for (size_t i = 0; i < bufferCount; i++) {
- ASSERT_EQ(i, pool->getAvailableBufferCount());
- acquiredBuffers[i]->release();
- acquiredBuffers[i] = nullptr;
- }
-
- ASSERT_EQ(bufferCount, pool->getAvailableBufferCount());
- }
-}
-
-TEST(BufferPool, acquireReleaseInterleaved) {
- static const int numRuns = 5;
-
- // 10 buffers of size 1
- static const size_t bufferSize = 1;
- static const size_t bufferCount = 10;
-
- sp<BufferPool> pool = new BufferPool(bufferSize, bufferCount);
-
- for (int run = 0; run < numRuns; run++) {
- BufferPool::Buffer* acquiredBuffers[bufferCount];
-
- // acquire all
- for (size_t i = 0; i < bufferCount; i++) {
- ASSERT_EQ(bufferCount - i, pool->getAvailableBufferCount());
- acquiredBuffers[i] = pool->acquire();
- ASSERT_NE(nullptr, acquiredBuffers[i]);
- }
-
- // release half
- for (size_t i = 0; i < bufferCount / 2; i++) {
- ASSERT_EQ(i, pool->getAvailableBufferCount());
- acquiredBuffers[i]->release();
- acquiredBuffers[i] = nullptr;
- }
-
- const size_t expectedRemaining = bufferCount / 2;
- ASSERT_EQ(expectedRemaining, pool->getAvailableBufferCount());
-
- // acquire half
- for (size_t i = 0; i < bufferCount / 2; i++) {
- ASSERT_EQ(expectedRemaining - i, pool->getAvailableBufferCount());
- acquiredBuffers[i] = pool->acquire();
- }
-
- // acquire one more, should fail
- ASSERT_EQ(nullptr, pool->acquire());
-
- // release all
- for (size_t i = 0; i < bufferCount; i++) {
- ASSERT_EQ(i, pool->getAvailableBufferCount());
- acquiredBuffers[i]->release();
- acquiredBuffers[i] = nullptr;
- }
-
- ASSERT_EQ(bufferCount, pool->getAvailableBufferCount());
- }
-}
-
-};
-};