Use unique_ptr for proto parser to handle destruction.
There's a memory leak where Layers created for the proto tree are not
removed when no longer used. This ensures that there's a unique_ptr for
each parser layer created so it will be automatically destroyed when
out of scope.
Change-Id: I56731d28a39e7d9d157c59065102d97f316f3b7d
Fixes: 74071380
Test: adb shell dumpsys SurfaceFlinger
Test: LayerProtoStress.mem_info no longer shows increased memory
diff --git a/services/surfaceflinger/LayerStats.cpp b/services/surfaceflinger/LayerStats.cpp
index ea2542c..ad1043f 100644
--- a/services/surfaceflinger/LayerStats.cpp
+++ b/services/surfaceflinger/LayerStats.cpp
@@ -56,11 +56,11 @@
}
void LayerStats::traverseLayerTreeStatsLocked(
- const std::vector<const LayerProtoParser::Layer*>& layerTree,
+ std::vector<std::unique_ptr<LayerProtoParser::Layer>> layerTree,
const LayerProtoParser::LayerGlobal* layerGlobal) {
- for (auto layer : layerTree) {
+ for (std::unique_ptr<LayerProtoParser::Layer>& layer : layerTree) {
if (!layer) continue;
- traverseLayerTreeStatsLocked(layer->children, layerGlobal);
+ traverseLayerTreeStatsLocked(std::move(layer->children), layerGlobal);
std::string key =
base::StringPrintf("%s,%s,%s,%s,%s,%s,%s,%s,%s",
destinationLocation(layer->hwcFrame.left,
@@ -71,7 +71,7 @@
layerGlobal->resolution[0], true),
destinationSize(layer->hwcFrame.bottom - layer->hwcFrame.top,
layerGlobal->resolution[1], false),
- layer->type.c_str(), scaleRatioWH(layer).c_str(),
+ layer->type.c_str(), scaleRatioWH(layer.get()).c_str(),
layerTransform(layer->hwcTransform), layer->pixelFormat.c_str(),
layer->dataspace.c_str());
mLayerStatsMap[key]++;
@@ -83,8 +83,7 @@
auto layerGlobal = LayerProtoParser::generateLayerGlobalInfo(layersProto);
auto layerTree = LayerProtoParser::generateLayerTree(layersProto);
std::lock_guard<std::mutex> lock(mMutex);
- traverseLayerTreeStatsLocked(layerTree, &layerGlobal);
- LayerProtoParser::destroyLayerTree(layerTree);
+ traverseLayerTreeStatsLocked(std::move(layerTree), &layerGlobal);
}
void LayerStats::dump(String8& result) {
diff --git a/services/surfaceflinger/LayerStats.h b/services/surfaceflinger/LayerStats.h
index 1924110..9522725 100644
--- a/services/surfaceflinger/LayerStats.h
+++ b/services/surfaceflinger/LayerStats.h
@@ -37,8 +37,9 @@
private:
// Traverse layer tree to get all visible layers' stats
- void traverseLayerTreeStatsLocked(const std::vector<const LayerProtoParser::Layer*>& layerTree,
- const LayerProtoParser::LayerGlobal* layerGlobal);
+ void traverseLayerTreeStatsLocked(
+ std::vector<std::unique_ptr<LayerProtoParser::Layer>> layerTree,
+ const LayerProtoParser::LayerGlobal* layerGlobal);
// Convert layer's top-left position into 8x8 percentage of the display
static const char* destinationLocation(int32_t location, int32_t range, bool isHorizontal);
// Convert layer's size into 8x8 percentage of the display
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 8d5c496..c07c76f 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -4062,7 +4062,7 @@
LayersProto layersProto = dumpProtoInfo(LayerVector::StateSet::Current);
auto layerTree = LayerProtoParser::generateLayerTree(layersProto);
- result.append(LayerProtoParser::layersToString(layerTree).c_str());
+ result.append(LayerProtoParser::layersToString(std::move(layerTree)).c_str());
/*
* Dump Display state
diff --git a/services/surfaceflinger/layerproto/LayerProtoParser.cpp b/services/surfaceflinger/layerproto/LayerProtoParser.cpp
index 7483abd..909f1f3 100644
--- a/services/surfaceflinger/layerproto/LayerProtoParser.cpp
+++ b/services/surfaceflinger/layerproto/LayerProtoParser.cpp
@@ -13,7 +13,6 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-
#include <android-base/stringprintf.h>
#include <layerproto/LayerProtoParser.h>
#include <ui/DebugUtils.h>
@@ -24,7 +23,7 @@
namespace android {
namespace surfaceflinger {
-bool sortLayers(const LayerProtoParser::Layer* lhs, const LayerProtoParser::Layer* rhs) {
+bool sortLayers(LayerProtoParser::Layer* lhs, const LayerProtoParser::Layer* rhs) {
uint32_t ls = lhs->layerStack;
uint32_t rs = rhs->layerStack;
if (ls != rs) return ls < rs;
@@ -38,33 +37,30 @@
return lhs->id < rhs->id;
}
+bool sortLayerUniquePtrs(const std::unique_ptr<LayerProtoParser::Layer>& lhs,
+ const std::unique_ptr<LayerProtoParser::Layer>& rhs) {
+ return sortLayers(lhs.get(), rhs.get());
+}
+
const LayerProtoParser::LayerGlobal LayerProtoParser::generateLayerGlobalInfo(
const LayersProto& layersProto) {
return {{layersProto.resolution().w(), layersProto.resolution().h()}};
}
-void LayerProtoParser::destroyLayerTree(
- const std::vector<const LayerProtoParser::Layer*>& layerTree) {
- for (auto layer : layerTree) {
- destroyLayerTree(layer->children);
- delete layer;
- }
-}
-
-std::vector<const LayerProtoParser::Layer*> LayerProtoParser::generateLayerTree(
+std::vector<std::unique_ptr<LayerProtoParser::Layer>> LayerProtoParser::generateLayerTree(
const LayersProto& layersProto) {
- auto layerMap = generateMap(layersProto);
+ std::unordered_map<int32_t, LayerProtoParser::Layer*> layerMap = generateMap(layersProto);
+ std::vector<std::unique_ptr<LayerProtoParser::Layer>> layers;
- std::vector<const Layer*> layers;
- std::for_each(layerMap.begin(), layerMap.end(),
- [&](const std::pair<const int32_t, Layer*>& ref) {
- if (ref.second->parent == nullptr) {
- // only save top level layers
- layers.push_back(ref.second);
- }
- });
+ for (std::pair<int32_t, Layer*> kv : layerMap) {
+ if (kv.second->parent == nullptr) {
+ // Make unique_ptr for top level layers since they are not children. This ensures there
+ // will only be one unique_ptr made for each layer.
+ layers.push_back(std::unique_ptr<Layer>(kv.second));
+ }
+ }
- std::sort(layers.begin(), layers.end(), sortLayers);
+ std::sort(layers.begin(), layers.end(), sortLayerUniquePtrs);
return layers;
}
@@ -181,63 +177,61 @@
for (int i = 0; i < layerProto.children_size(); i++) {
if (layerMap.count(layerProto.children(i)) > 0) {
- auto childLayer = layerMap[layerProto.children(i)];
- currLayer->children.push_back(childLayer);
+ // Only make unique_ptrs for children since they are guaranteed to be unique, only one
+ // parent per child. This ensures there will only be one unique_ptr made for each layer.
+ currLayer->children.push_back(std::unique_ptr<Layer>(layerMap[layerProto.children(i)]));
}
}
for (int i = 0; i < layerProto.relatives_size(); i++) {
if (layerMap.count(layerProto.relatives(i)) > 0) {
- auto relativeLayer = layerMap[layerProto.relatives(i)];
- currLayer->relatives.push_back(relativeLayer);
+ currLayer->relatives.push_back(layerMap[layerProto.relatives(i)]);
}
}
if (layerProto.has_parent()) {
if (layerMap.count(layerProto.parent()) > 0) {
- auto parentLayer = layerMap[layerProto.parent()];
- currLayer->parent = parentLayer;
+ currLayer->parent = layerMap[layerProto.parent()];
}
}
if (layerProto.has_z_order_relative_of()) {
if (layerMap.count(layerProto.z_order_relative_of()) > 0) {
- auto relativeLayer = layerMap[layerProto.z_order_relative_of()];
- currLayer->zOrderRelativeOf = relativeLayer;
+ currLayer->zOrderRelativeOf = layerMap[layerProto.z_order_relative_of()];
}
}
}
std::string LayerProtoParser::layersToString(
- const std::vector<const LayerProtoParser::Layer*> layers) {
+ std::vector<std::unique_ptr<LayerProtoParser::Layer>> layers) {
std::string result;
- for (const LayerProtoParser::Layer* layer : layers) {
+ for (std::unique_ptr<LayerProtoParser::Layer>& layer : layers) {
if (layer->zOrderRelativeOf != nullptr) {
continue;
}
- result.append(layerToString(layer).c_str());
+ result.append(layerToString(layer.get()).c_str());
}
return result;
}
-std::string LayerProtoParser::layerToString(const LayerProtoParser::Layer* layer) {
+std::string LayerProtoParser::layerToString(LayerProtoParser::Layer* layer) {
std::string result;
- std::vector<const Layer*> traverse(layer->relatives);
- for (const LayerProtoParser::Layer* child : layer->children) {
+ std::vector<Layer*> traverse(layer->relatives);
+ for (std::unique_ptr<LayerProtoParser::Layer>& child : layer->children) {
if (child->zOrderRelativeOf != nullptr) {
continue;
}
- traverse.push_back(child);
+ traverse.push_back(child.get());
}
std::sort(traverse.begin(), traverse.end(), sortLayers);
size_t i = 0;
for (; i < traverse.size(); i++) {
- const auto& relative = traverse[i];
+ auto& relative = traverse[i];
if (relative->z >= 0) {
break;
}
@@ -246,7 +240,7 @@
result.append(layer->to_string().c_str());
result.append("\n");
for (; i < traverse.size(); i++) {
- const auto& relative = traverse[i];
+ auto& relative = traverse[i];
result.append(layerToString(relative).c_str());
}
diff --git a/services/surfaceflinger/layerproto/include/layerproto/LayerProtoParser.h b/services/surfaceflinger/layerproto/include/layerproto/LayerProtoParser.h
index 4747275..948d5ab 100644
--- a/services/surfaceflinger/layerproto/include/layerproto/LayerProtoParser.h
+++ b/services/surfaceflinger/layerproto/include/layerproto/LayerProtoParser.h
@@ -19,6 +19,7 @@
#include <math/vec4.h>
+#include <memory>
#include <unordered_map>
#include <vector>
@@ -79,8 +80,8 @@
public:
int32_t id;
std::string name;
- std::vector<const Layer*> children;
- std::vector<const Layer*> relatives;
+ std::vector<std::unique_ptr<Layer>> children;
+ std::vector<Layer*> relatives;
std::string type;
LayerProtoParser::Region transparentRegion;
LayerProtoParser::Region visibleRegion;
@@ -119,9 +120,8 @@
};
static const LayerGlobal generateLayerGlobalInfo(const LayersProto& layersProto);
- static std::vector<const Layer*> generateLayerTree(const LayersProto& layersProto);
- static void destroyLayerTree(const std::vector<const LayerProtoParser::Layer*>& layerTree);
- static std::string layersToString(const std::vector<const LayerProtoParser::Layer*> layers);
+ static std::vector<std::unique_ptr<Layer>> generateLayerTree(const LayersProto& layersProto);
+ static std::string layersToString(std::vector<std::unique_ptr<LayerProtoParser::Layer>> layers);
private:
static std::unordered_map<int32_t, Layer*> generateMap(const LayersProto& layersProto);
@@ -135,7 +135,7 @@
static void updateChildrenAndRelative(const LayerProto& layerProto,
std::unordered_map<int32_t, Layer*>& layerMap);
- static std::string layerToString(const LayerProtoParser::Layer* layer);
+ static std::string layerToString(LayerProtoParser::Layer* layer);
};
} // namespace surfaceflinger
diff --git a/services/surfaceflinger/tests/Android.bp b/services/surfaceflinger/tests/Android.bp
index 7d3da32..7523399 100644
--- a/services/surfaceflinger/tests/Android.bp
+++ b/services/surfaceflinger/tests/Android.bp
@@ -33,6 +33,7 @@
"libEGL",
"libGLESv2",
"libgui",
+ "liblayers_proto",
"liblog",
"libprotobuf-cpp-full",
"libui",
diff --git a/services/surfaceflinger/tests/Stress_test.cpp b/services/surfaceflinger/tests/Stress_test.cpp
index 33dd2f5..4577153 100644
--- a/services/surfaceflinger/tests/Stress_test.cpp
+++ b/services/surfaceflinger/tests/Stress_test.cpp
@@ -22,7 +22,7 @@
#include <thread>
#include <functional>
-
+#include <layerproto/LayerProtoParser.h>
namespace android {
@@ -47,4 +47,66 @@
}
}
+surfaceflinger::LayersProto generateLayerProto() {
+ surfaceflinger::LayersProto layersProto;
+ std::array<surfaceflinger::LayerProto*, 10> layers = {};
+ for (size_t i = 0; i < layers.size(); ++i) {
+ layers[i] = layersProto.add_layers();
+ layers[i]->set_id(i);
+ }
+
+ layers[0]->add_children(1);
+ layers[1]->set_parent(0);
+ layers[0]->add_children(2);
+ layers[2]->set_parent(0);
+ layers[0]->add_children(3);
+ layers[3]->set_parent(0);
+ layers[2]->add_children(4);
+ layers[4]->set_parent(2);
+ layers[3]->add_children(5);
+ layers[5]->set_parent(3);
+ layers[5]->add_children(6);
+ layers[6]->set_parent(5);
+ layers[5]->add_children(7);
+ layers[7]->set_parent(5);
+ layers[6]->add_children(8);
+ layers[8]->set_parent(6);
+
+ layers[4]->set_z_order_relative_of(3);
+ layers[3]->add_relatives(4);
+ layers[8]->set_z_order_relative_of(9);
+ layers[9]->add_relatives(8);
+ layers[3]->set_z_order_relative_of(1);
+ layers[1]->add_relatives(3);
+
+/* ----------------------------
+ * - 0 - - 9 -
+ * / | \
+ * 1 2 3(1)
+ * | |
+ * 4(3) 5
+ * / \
+ * 6 7
+ * |
+ * 8(9)
+ * -------------------------- */
+
+ return layersProto;
+}
+
+TEST(LayerProtoStress, mem_info) {
+ std::string cmd = "dumpsys meminfo ";
+ cmd += std::to_string(getpid());
+ system(cmd.c_str());
+ for (int i = 0; i < 100000; i++) {
+ surfaceflinger::LayersProto layersProto = generateLayerProto();
+ auto layerTree = surfaceflinger::LayerProtoParser::generateLayerTree(layersProto);
+ // Allow some layerTrees to just fall out of scope (instead of std::move)
+ if (i % 2) {
+ surfaceflinger::LayerProtoParser::layersToString(std::move(layerTree));
+ }
+ }
+ system(cmd.c_str());
+}
+
}