Merge "SurfaceFlinger: Use a lockless stack for binder->tracing thread" into tm-dev
diff --git a/services/surfaceflinger/Tracing/LocklessStack.h b/services/surfaceflinger/Tracing/LocklessStack.h
new file mode 100644
index 0000000..20f2aa8
--- /dev/null
+++ b/services/surfaceflinger/Tracing/LocklessStack.h
@@ -0,0 +1,72 @@
+/*
+ * Copyright 2021 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 <atomic>
+
+template <typename T>
+// Single consumer multi producer stack. We can understand the two operations independently to see
+// why they are without race condition.
+//
+// push is responsible for maintaining a linked list stored in mPush, and called from multiple
+// threads without lock. We can see that if two threads never observe the same value from
+// mPush.load, it just functions as a normal linked list. In the case where two threads observe the
+// same value, one of them has to execute the compare_exchange first. The one that doesn't execute
+// the compare exchange first, will receive false from compare_exchange. previousHead is updated (by
+// compare_exchange) to the most recent value of mPush, and we try again. It's relatively clear to
+// see that the process can repeat with an arbitrary number of threads.
+//
+// Pop is much simpler. If mPop is empty (as it begins) it atomically exchanges
+// the entire push list with null. This is safe, since the only other reader (push)
+// of mPush will retry if it changes in between it's read and atomic compare. We
+// then store the list and pop one element.
+//
+// If we already had something in the pop list we just pop directly.
+class LocklessStack {
+public:
+ class Entry {
+ public:
+ T* mValue;
+ std::atomic<Entry*> mNext;
+ Entry(T* value): mValue(value) {}
+ };
+ std::atomic<Entry*> mPush = nullptr;
+ std::atomic<Entry*> mPop = nullptr;
+ void push(T* value) {
+ Entry* entry = new Entry(value);
+ Entry* previousHead = mPush.load(/*std::memory_order_relaxed*/);
+ do {
+ entry->mNext = previousHead;
+ } while (!mPush.compare_exchange_weak(previousHead, entry)); /*std::memory_order_release*/
+ }
+ T* pop() {
+ Entry* popped = mPop.load(/*std::memory_order_acquire*/);
+ if (popped) {
+ // Single consumer so this is fine
+ mPop.store(popped->mNext/* , std::memory_order_release */);
+ auto value = popped->mValue;
+ delete popped;
+ return value;
+ } else {
+ Entry *grabbedList = mPush.exchange(nullptr/* , std::memory_order_acquire */);
+ if (!grabbedList) return nullptr;
+ mPop.store(grabbedList->mNext/* , std::memory_order_release */);
+ auto value = grabbedList->mValue;
+ delete grabbedList;
+ return value;
+ }
+ }
+};
diff --git a/services/surfaceflinger/Tracing/TransactionProtoParser.cpp b/services/surfaceflinger/Tracing/TransactionProtoParser.cpp
index 1ec9577..d249b60 100644
--- a/services/surfaceflinger/Tracing/TransactionProtoParser.cpp
+++ b/services/surfaceflinger/Tracing/TransactionProtoParser.cpp
@@ -180,13 +180,13 @@
}
if (layer.what & layer_state_t::eReparent) {
- int32_t layerId = layer.parentSurfaceControlForChild
+ int64_t layerId = layer.parentSurfaceControlForChild
? mMapper->getLayerId(layer.parentSurfaceControlForChild->getHandle())
: -1;
proto.set_parent_id(layerId);
}
if (layer.what & layer_state_t::eRelativeLayerChanged) {
- int32_t layerId = layer.relativeLayerSurfaceControl
+ int64_t layerId = layer.relativeLayerSurfaceControl
? mMapper->getLayerId(layer.relativeLayerSurfaceControl->getHandle())
: -1;
proto.set_relative_parent_id(layerId);
@@ -342,13 +342,13 @@
outState.merge(state);
if (state.what & layer_state_t::eReparent) {
- outState.parentId = proto.parent_id();
+ outState.parentId = static_cast<int32_t>(proto.parent_id());
}
if (state.what & layer_state_t::eRelativeLayerChanged) {
- outState.relativeParentId = proto.relative_parent_id();
+ outState.relativeParentId = static_cast<int32_t>(proto.relative_parent_id());
}
if (state.what & layer_state_t::eInputInfoChanged) {
- outState.inputCropId = proto.window_info_handle().crop_layer_id();
+ outState.inputCropId = static_cast<int32_t>(proto.window_info_handle().crop_layer_id());
}
if (state.what & layer_state_t::eBufferChanged) {
const proto::LayerState_BufferData& bufferProto = proto.buffer_data();
@@ -364,7 +364,7 @@
}
void TransactionProtoParser::fromProto(const proto::LayerState& proto, layer_state_t& layer) {
- layer.layerId = proto.layer_id();
+ layer.layerId = (int32_t)proto.layer_id();
layer.what |= proto.what();
layer.surface = mMapper->getLayerHandle(layer.layerId);
@@ -451,23 +451,25 @@
}
if (proto.what() & layer_state_t::eReparent) {
- int32_t layerId = proto.parent_id();
+ int64_t layerId = proto.parent_id();
if (layerId == -1) {
layer.parentSurfaceControlForChild = nullptr;
} else {
layer.parentSurfaceControlForChild =
new SurfaceControl(SurfaceComposerClient::getDefault(),
- mMapper->getLayerHandle(layerId), nullptr, layerId);
+ mMapper->getLayerHandle(static_cast<int32_t>(layerId)),
+ nullptr, static_cast<int32_t>(layerId));
}
}
if (proto.what() & layer_state_t::eRelativeLayerChanged) {
- int32_t layerId = proto.relative_parent_id();
+ int64_t layerId = proto.relative_parent_id();
if (layerId == -1) {
layer.relativeLayerSurfaceControl = nullptr;
} else {
layer.relativeLayerSurfaceControl =
new SurfaceControl(SurfaceComposerClient::getDefault(),
- mMapper->getLayerHandle(layerId), nullptr, layerId);
+ mMapper->getLayerHandle(static_cast<int32_t>(layerId)),
+ nullptr, static_cast<int32_t>(layerId));
}
layer.z = proto.z();
}
@@ -494,8 +496,9 @@
inputInfo.transform.set(transformProto.tx(), transformProto.ty());
inputInfo.replaceTouchableRegionWithCrop =
windowInfoProto.replace_touchable_region_with_crop();
- int32_t layerId = windowInfoProto.crop_layer_id();
- inputInfo.touchableRegionCropHandle = mMapper->getLayerHandle(layerId);
+ int64_t layerId = windowInfoProto.crop_layer_id();
+ inputInfo.touchableRegionCropHandle =
+ mMapper->getLayerHandle(static_cast<int32_t>(layerId));
layer.windowInfoHandle = sp<gui::WindowInfoHandle>::make(inputInfo);
}
if (proto.what() & layer_state_t::eBackgroundColorChanged) {
diff --git a/services/surfaceflinger/Tracing/TransactionProtoParser.h b/services/surfaceflinger/Tracing/TransactionProtoParser.h
index 2f70b27..872a901 100644
--- a/services/surfaceflinger/Tracing/TransactionProtoParser.h
+++ b/services/surfaceflinger/Tracing/TransactionProtoParser.h
@@ -80,7 +80,8 @@
public:
virtual ~FlingerDataMapper() = default;
virtual sp<IBinder> getLayerHandle(int32_t /* layerId */) const { return nullptr; }
- virtual int32_t getLayerId(const sp<IBinder>& /* layerHandle */) const { return -1; }
+ virtual int64_t getLayerId(const sp<IBinder>& /* layerHandle */) const { return -1; }
+ virtual int64_t getLayerId(BBinder* /* layerHandle */) const { return -1; }
virtual sp<IBinder> getDisplayHandle(int32_t /* displayId */) const { return nullptr; }
virtual int32_t getDisplayId(const sp<IBinder>& /* displayHandle */) const { return -1; }
virtual std::shared_ptr<BufferData> getGraphicData(uint64_t bufferId, uint32_t width,
@@ -106,6 +107,7 @@
TransactionState fromProto(const proto::TransactionState&);
void mergeFromProto(const proto::LayerState&, TracingLayerState& outState);
void fromProto(const proto::LayerCreationArgs&, TracingLayerCreationArgs& outArgs);
+ std::unique_ptr<FlingerDataMapper> mMapper;
private:
proto::LayerState toProto(const layer_state_t&);
@@ -113,7 +115,6 @@
void fromProto(const proto::LayerState&, layer_state_t& out);
DisplayState fromProto(const proto::DisplayState&);
- std::unique_ptr<FlingerDataMapper> mMapper;
};
} // namespace android::surfaceflinger
diff --git a/services/surfaceflinger/Tracing/TransactionTracing.cpp b/services/surfaceflinger/Tracing/TransactionTracing.cpp
index d5e837f..6381758 100644
--- a/services/surfaceflinger/Tracing/TransactionTracing.cpp
+++ b/services/surfaceflinger/Tracing/TransactionTracing.cpp
@@ -29,23 +29,16 @@
namespace android {
-class FlingerDataMapper : public TransactionProtoParser::FlingerDataMapper {
- std::unordered_map<BBinder* /* layerHandle */, int32_t /* layerId */>& mLayerHandles;
-
+// Keeps the binder address as the layer id so we can avoid holding the tracing lock in the
+// binder thread.
+class FlatDataMapper : public TransactionProtoParser::FlingerDataMapper {
public:
- FlingerDataMapper(std::unordered_map<BBinder* /* handle */, int32_t /* id */>& layerHandles)
- : mLayerHandles(layerHandles) {}
-
- int32_t getLayerId(const sp<IBinder>& layerHandle) const override {
+ virtual int64_t getLayerId(const sp<IBinder>& layerHandle) const {
if (layerHandle == nullptr) {
return -1;
}
- auto it = mLayerHandles.find(layerHandle->localBinder());
- if (it == mLayerHandles.end()) {
- ALOGW("Could not find layer handle %p", layerHandle->localBinder());
- return -1;
- }
- return it->second;
+
+ return reinterpret_cast<int64_t>(layerHandle->localBinder());
}
void getGraphicBufferPropertiesFromCache(client_cache_t cachedBuffer, uint64_t* outBufferId,
@@ -72,8 +65,33 @@
}
};
+class FlingerDataMapper : public FlatDataMapper {
+ std::unordered_map<BBinder* /* layerHandle */, int32_t /* layerId */>& mLayerHandles;
+
+public:
+ FlingerDataMapper(std::unordered_map<BBinder* /* handle */, int32_t /* id */>& layerHandles)
+ : mLayerHandles(layerHandles) {}
+
+ int64_t getLayerId(const sp<IBinder>& layerHandle) const override {
+ if (layerHandle == nullptr) {
+ return -1;
+ }
+ return getLayerId(layerHandle->localBinder());
+ }
+
+ int64_t getLayerId(BBinder* localBinder) const {
+ auto it = mLayerHandles.find(localBinder);
+ if (it == mLayerHandles.end()) {
+ ALOGW("Could not find layer handle %p", localBinder);
+ return -1;
+ }
+ return it->second;
+ }
+};
+
TransactionTracing::TransactionTracing()
- : mProtoParser(std::make_unique<FlingerDataMapper>(mLayerHandles)) {
+ : mProtoParser(std::make_unique<FlingerDataMapper>(mLayerHandles)),
+ mLockfreeProtoParser(std::make_unique<FlatDataMapper>()) {
std::scoped_lock lock(mTraceLock);
mBuffer.setSize(mBufferSizeInBytes);
@@ -129,9 +147,9 @@
}
void TransactionTracing::addQueuedTransaction(const TransactionState& transaction) {
- std::scoped_lock lock(mTraceLock);
- ATRACE_CALL();
- mQueuedTransactions[transaction.id] = mProtoParser.toProto(transaction);
+ proto::TransactionState* state =
+ new proto::TransactionState(mLockfreeProtoParser.toProto(transaction));
+ mTransactionQueue.push(state);
}
void TransactionTracing::addCommittedTransactions(std::vector<TransactionState>& transactions,
@@ -182,6 +200,38 @@
std::scoped_lock lock(mTraceLock);
std::vector<std::string> removedEntries;
proto::TransactionTraceEntry entryProto;
+
+ while (auto incomingTransaction = mTransactionQueue.pop()) {
+ auto transaction = *incomingTransaction;
+ int32_t layerCount = transaction.layer_changes_size();
+ for (int i = 0; i < layerCount; i++) {
+ auto layer = transaction.mutable_layer_changes(i);
+ layer->set_layer_id(
+ mProtoParser.mMapper->getLayerId(reinterpret_cast<BBinder*>(layer->layer_id())));
+ if ((layer->what() & layer_state_t::eReparent) && layer->parent_id() != -1) {
+ layer->set_parent_id(
+ mProtoParser.mMapper->getLayerId(reinterpret_cast<BBinder*>(
+ layer->parent_id())));
+ }
+
+ if ((layer->what() & layer_state_t::eRelativeLayerChanged) &&
+ layer->relative_parent_id() != -1) {
+ layer->set_relative_parent_id(
+ mProtoParser.mMapper->getLayerId(reinterpret_cast<BBinder*>(
+ layer->relative_parent_id())));
+ }
+
+ if (layer->has_window_info_handle() &&
+ layer->window_info_handle().crop_layer_id() != -1) {
+ auto input = layer->mutable_window_info_handle();
+ input->set_crop_layer_id(
+ mProtoParser.mMapper->getLayerId(reinterpret_cast<BBinder*>(
+ input->crop_layer_id())));
+ }
+ }
+ mQueuedTransactions[incomingTransaction->transaction_id()] = transaction;
+ delete incomingTransaction;
+ }
for (const CommittedTransactions& entry : committedTransactions) {
entryProto.set_elapsed_realtime_nanos(entry.timestamp);
entryProto.set_vsync_id(entry.vsyncId);
@@ -317,9 +367,9 @@
// Merge layer states to starting transaction state.
for (const proto::TransactionState& transaction : removedEntry.transactions()) {
for (const proto::LayerState& layerState : transaction.layer_changes()) {
- auto it = mStartingStates.find(layerState.layer_id());
+ auto it = mStartingStates.find((int32_t)layerState.layer_id());
if (it == mStartingStates.end()) {
- ALOGW("Could not find layer id %d", layerState.layer_id());
+ ALOGW("Could not find layer id %d", (int32_t)layerState.layer_id());
continue;
}
mProtoParser.mergeFromProto(layerState, it->second);
diff --git a/services/surfaceflinger/Tracing/TransactionTracing.h b/services/surfaceflinger/Tracing/TransactionTracing.h
index 95256c4..4c291f9 100644
--- a/services/surfaceflinger/Tracing/TransactionTracing.h
+++ b/services/surfaceflinger/Tracing/TransactionTracing.h
@@ -26,6 +26,7 @@
#include <thread>
#include "RingBuffer.h"
+#include "LocklessStack.h"
#include "TransactionProtoParser.h"
using namespace android::surfaceflinger;
@@ -78,6 +79,7 @@
size_t mBufferSizeInBytes GUARDED_BY(mTraceLock) = CONTINUOUS_TRACING_BUFFER_SIZE;
std::unordered_map<uint64_t, proto::TransactionState> mQueuedTransactions
GUARDED_BY(mTraceLock);
+ LocklessStack<proto::TransactionState> mTransactionQueue;
nsecs_t mStartingTimestamp GUARDED_BY(mTraceLock);
std::vector<proto::LayerCreationArgs> mCreatedLayers GUARDED_BY(mTraceLock);
std::unordered_map<BBinder* /* layerHandle */, int32_t /* layerId */> mLayerHandles
@@ -85,6 +87,9 @@
std::vector<int32_t /* layerId */> mRemovedLayerHandles GUARDED_BY(mTraceLock);
std::map<int32_t /* layerId */, TracingLayerState> mStartingStates GUARDED_BY(mTraceLock);
TransactionProtoParser mProtoParser GUARDED_BY(mTraceLock);
+ // Parses the transaction to proto without holding any tracing locks so we can generate proto
+ // in the binder thread without any contention.
+ TransactionProtoParser mLockfreeProtoParser;
// We do not want main thread to block so main thread will try to acquire mMainThreadLock,
// otherwise will push data to temporary container.
diff --git a/services/surfaceflinger/layerproto/transactions.proto b/services/surfaceflinger/layerproto/transactions.proto
index fcf4499..4f99b19 100644
--- a/services/surfaceflinger/layerproto/transactions.proto
+++ b/services/surfaceflinger/layerproto/transactions.proto
@@ -70,7 +70,7 @@
// Keep insync with layer_state_t
message LayerState {
- int32 layer_id = 1;
+ int64 layer_id = 1;
// Changes are split into ChangesLsb and ChangesMsb. First 32 bits are in ChangesLsb
// and the next 32 bits are in ChangesMsb. This is needed because enums have to be
// 32 bits and there's no nice way to put 64bit constants into .proto files.
@@ -161,8 +161,8 @@
Matrix22 matrix = 11;
float corner_radius = 12;
uint32 background_blur_radius = 13;
- int32 parent_id = 14;
- int32 relative_parent_id = 15;
+ int64 parent_id = 14;
+ int64 relative_parent_id = 15;
float alpha = 16;
message Color3 {
@@ -233,7 +233,7 @@
bool focusable = 5;
bool has_wallpaper = 6;
float global_scale_factor = 7;
- int32 crop_layer_id = 8;
+ int64 crop_layer_id = 8;
bool replace_touchable_region_with_crop = 9;
RectProto touchable_region_crop = 10;
Transform transform = 11;
diff --git a/services/surfaceflinger/tests/unittests/TransactionProtoParserTest.cpp b/services/surfaceflinger/tests/unittests/TransactionProtoParserTest.cpp
index ab893a3..f5e3b77 100644
--- a/services/surfaceflinger/tests/unittests/TransactionProtoParserTest.cpp
+++ b/services/surfaceflinger/tests/unittests/TransactionProtoParserTest.cpp
@@ -81,7 +81,7 @@
sp<IBinder> getLayerHandle(int32_t id) const override {
return (id == 42) ? layerHandle : nullptr;
}
- int32_t getLayerId(const sp<IBinder>& handle) const override {
+ int64_t getLayerId(const sp<IBinder>& handle) const override {
return (handle == layerHandle) ? 42 : -1;
}
sp<IBinder> getDisplayHandle(int32_t id) const {