SF: Simplify dumpsys flag parsing
This CL removes sequential parsing for dumpsys flags, which was order-
dependent and boilerplate-heavy.
Note that flags cannot be combined for simplicity.
Bug: None
Test: dumpsys SurfaceFlinger [--FLAG [ARGS...]]
Change-Id: Id305ffee464c6cad4f228c7a7f603c33a2978d66
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 5df2876..1cb38c8 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -28,6 +28,7 @@
#include <functional>
#include <mutex>
#include <optional>
+#include <unordered_map>
#include <cutils/properties.h>
#include <log/log.h>
@@ -4344,8 +4345,8 @@
// ---------------------------------------------------------------------------
-status_t SurfaceFlinger::doDump(int fd, const Vector<String16>& args, bool asProto)
- NO_THREAD_SAFETY_ANALYSIS {
+status_t SurfaceFlinger::doDump(int fd, const DumpArgs& args,
+ bool asProto) NO_THREAD_SAFETY_ANALYSIS {
std::string result;
IPCThreadState* ipc = IPCThreadState::self();
@@ -4369,114 +4370,35 @@
strerror(-err), err);
}
- bool dumpAll = true;
- size_t index = 0;
- size_t numArgs = args.size();
+ using namespace std::string_literals;
- if (numArgs) {
- if ((index < numArgs) &&
- (args[index] == String16("--list"))) {
- index++;
- listLayersLocked(args, index, result);
- dumpAll = false;
- }
+ static const std::unordered_map<std::string, Dumper> dumpers = {
+ {"--clear-layer-stats"s, dumper([this](std::string&) { mLayerStats.clear(); })},
+ {"--disable-layer-stats"s, dumper([this](std::string&) { mLayerStats.disable(); })},
+ {"--display-id"s, dumper(&SurfaceFlinger::dumpDisplayIdentificationData)},
+ {"--dispsync"s, dumper([this](std::string& s) { mPrimaryDispSync->dump(s); })},
+ {"--dump-layer-stats"s, dumper([this](std::string& s) { mLayerStats.dump(s); })},
+ {"--enable-layer-stats"s, dumper([this](std::string&) { mLayerStats.enable(); })},
+ {"--frame-composition"s, dumper(&SurfaceFlinger::dumpFrameCompositionInfo)},
+ {"--frame-events"s, dumper(&SurfaceFlinger::dumpFrameEventsLocked)},
+ {"--latency"s, argsDumper(&SurfaceFlinger::dumpStatsLocked)},
+ {"--latency-clear"s, argsDumper(&SurfaceFlinger::clearStatsLocked)},
+ {"--list"s, dumper(&SurfaceFlinger::listLayersLocked)},
+ {"--static-screen"s, dumper(&SurfaceFlinger::dumpStaticScreenStats)},
+ {"--timestats"s, protoDumper(&SurfaceFlinger::dumpTimeStats)},
+ {"--wide-color"s, dumper(&SurfaceFlinger::dumpWideColorInfo)},
+ };
- if ((index < numArgs) &&
- (args[index] == String16("--latency"))) {
- index++;
- dumpStatsLocked(args, index, result);
- dumpAll = false;
- }
+ const auto flag = args.empty() ? ""s : std::string(String8(args[0]));
- if ((index < numArgs) &&
- (args[index] == String16("--latency-clear"))) {
- index++;
- clearStatsLocked(args, index, result);
- dumpAll = false;
- }
-
- if ((index < numArgs) &&
- (args[index] == String16("--dispsync"))) {
- index++;
- mPrimaryDispSync->dump(result);
- dumpAll = false;
- }
-
- if ((index < numArgs) &&
- (args[index] == String16("--static-screen"))) {
- index++;
- dumpStaticScreenStats(result);
- dumpAll = false;
- }
-
- if ((index < numArgs) &&
- (args[index] == String16("--frame-events"))) {
- index++;
- dumpFrameEventsLocked(result);
- dumpAll = false;
- }
-
- if ((index < numArgs) && (args[index] == String16("--wide-color"))) {
- index++;
- dumpWideColorInfo(result);
- dumpAll = false;
- }
-
- if ((index < numArgs) &&
- (args[index] == String16("--enable-layer-stats"))) {
- index++;
- mLayerStats.enable();
- dumpAll = false;
- }
-
- if ((index < numArgs) &&
- (args[index] == String16("--disable-layer-stats"))) {
- index++;
- mLayerStats.disable();
- dumpAll = false;
- }
-
- if ((index < numArgs) &&
- (args[index] == String16("--clear-layer-stats"))) {
- index++;
- mLayerStats.clear();
- dumpAll = false;
- }
-
- if ((index < numArgs) &&
- (args[index] == String16("--dump-layer-stats"))) {
- index++;
- mLayerStats.dump(result);
- dumpAll = false;
- }
-
- if ((index < numArgs) &&
- (args[index] == String16("--frame-composition"))) {
- index++;
- dumpFrameCompositionInfo(result);
- dumpAll = false;
- }
-
- if ((index < numArgs) &&
- (args[index] == String16("--display-identification"))) {
- index++;
- dumpDisplayIdentificationData(result);
- dumpAll = false;
- }
-
- if ((index < numArgs) && (args[index] == String16("--timestats"))) {
- index++;
- mTimeStats->parseArgs(asProto, args, index, result);
- dumpAll = false;
- }
- }
-
- if (dumpAll) {
+ if (const auto it = dumpers.find(flag); it != dumpers.end()) {
+ (it->second)(args, asProto, result);
+ } else {
if (asProto) {
LayersProto layersProto = dumpProtoInfo(LayerVector::StateSet::Current);
result.append(layersProto.SerializeAsString().c_str(), layersProto.ByteSize());
} else {
- dumpAllLocked(args, index, result);
+ dumpAllLocked(args, result);
}
}
@@ -4488,43 +4410,29 @@
return NO_ERROR;
}
-void SurfaceFlinger::listLayersLocked(const Vector<String16>& /* args */, size_t& /* index */,
- std::string& result) const {
+void SurfaceFlinger::listLayersLocked(std::string& result) const {
mCurrentState.traverseInZOrder(
[&](Layer* layer) { StringAppendF(&result, "%s\n", layer->getName().string()); });
}
-void SurfaceFlinger::dumpStatsLocked(const Vector<String16>& args, size_t& index,
- std::string& result) const {
- String8 name;
- if (index < args.size()) {
- name = String8(args[index]);
- index++;
- }
-
+void SurfaceFlinger::dumpStatsLocked(const DumpArgs& args, std::string& result) const {
StringAppendF(&result, "%" PRId64 "\n", getVsyncPeriod());
- if (name.isEmpty()) {
- mAnimFrameTracker.dumpStats(result);
- } else {
+ if (args.size() > 1) {
+ const auto name = String8(args[1]);
mCurrentState.traverseInZOrder([&](Layer* layer) {
if (name == layer->getName()) {
layer->dumpFrameStats(result);
}
});
+ } else {
+ mAnimFrameTracker.dumpStats(result);
}
}
-void SurfaceFlinger::clearStatsLocked(const Vector<String16>& args, size_t& index,
- std::string& /* result */) {
- String8 name;
- if (index < args.size()) {
- name = String8(args[index]);
- index++;
- }
-
+void SurfaceFlinger::clearStatsLocked(const DumpArgs& args, std::string&) {
mCurrentState.traverseInZOrder([&](Layer* layer) {
- if (name.isEmpty() || (name == layer->getName())) {
+ if (args.size() < 2 || String8(args[1]) == layer->getName()) {
layer->clearFrameStats();
}
});
@@ -4532,6 +4440,10 @@
mAnimFrameTracker.clearStats();
}
+void SurfaceFlinger::dumpTimeStats(const DumpArgs& args, bool asProto, std::string& result) const {
+ mTimeStats->parseArgs(asProto, args, result);
+}
+
// This should only be called from the main thread. Otherwise it would need
// the lock and should use mCurrentState rather than mDrawingState.
void SurfaceFlinger::logFrameStats() {
@@ -4760,15 +4672,8 @@
return layersProto;
}
-void SurfaceFlinger::dumpAllLocked(const Vector<String16>& args, size_t& index,
- std::string& result) const {
- bool colorize = false;
- if (index < args.size()
- && (args[index] == String16("--color"))) {
- colorize = true;
- index++;
- }
-
+void SurfaceFlinger::dumpAllLocked(const DumpArgs& args, std::string& result) const {
+ const bool colorize = !args.empty() && args[0] == String16("--color");
Colorizer colorizer(colorize);
// figure out if we're stuck somewhere
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index 499ebcd..1a54f6c 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -68,6 +68,7 @@
#include <atomic>
#include <cstdint>
+#include <functional>
#include <map>
#include <memory>
#include <mutex>
@@ -75,6 +76,7 @@
#include <set>
#include <string>
#include <thread>
+#include <type_traits>
#include <unordered_map>
#include <utility>
@@ -786,19 +788,9 @@
};
}
- /* ------------------------------------------------------------------------
- * Debugging & dumpsys
+ /*
+ * Display identification
*/
-public:
- status_t dumpCritical(int fd, const Vector<String16>& /*args*/, bool asProto) {
- return doDump(fd, Vector<String16>(), asProto);
- }
-
- status_t dumpAll(int fd, const Vector<String16>& args, bool asProto) {
- return doDump(fd, args, asProto);
- }
-
-private:
sp<IBinder> getPhysicalDisplayToken(DisplayId displayId) const {
const auto it = mPhysicalDisplayTokens.find(displayId);
return it != mPhysicalDisplayTokens.end() ? it->second : nullptr;
@@ -830,15 +822,45 @@
return hwcDisplayId ? getHwComposer().toPhysicalDisplayId(*hwcDisplayId) : std::nullopt;
}
- void listLayersLocked(const Vector<String16>& args, size_t& index, std::string& result) const;
- void dumpStatsLocked(const Vector<String16>& args, size_t& index, std::string& result) const
- REQUIRES(mStateLock);
- void clearStatsLocked(const Vector<String16>& args, size_t& index, std::string& result);
- void dumpAllLocked(const Vector<String16>& args, size_t& index, std::string& result) const
- REQUIRES(mStateLock);
+ /*
+ * Debugging & dumpsys
+ */
bool startDdmConnection();
- void appendSfConfigString(std::string& result) const;
+ using DumpArgs = Vector<String16>;
+ using Dumper = std::function<void(const DumpArgs&, bool asProto, std::string&)>;
+
+ template <typename F, std::enable_if_t<!std::is_member_function_pointer_v<F>>* = nullptr>
+ static Dumper dumper(F&& dump) {
+ using namespace std::placeholders;
+ return std::bind(std::forward<F>(dump), _3);
+ }
+
+ template <typename F, std::enable_if_t<std::is_member_function_pointer_v<F>>* = nullptr>
+ Dumper dumper(F dump) {
+ using namespace std::placeholders;
+ return std::bind(dump, this, _3);
+ }
+
+ template <typename F>
+ Dumper argsDumper(F dump) {
+ using namespace std::placeholders;
+ return std::bind(dump, this, _1, _3);
+ }
+
+ template <typename F>
+ Dumper protoDumper(F dump) {
+ using namespace std::placeholders;
+ return std::bind(dump, this, _1, _2, _3);
+ }
+
+ void dumpAllLocked(const DumpArgs& args, std::string& result) const REQUIRES(mStateLock);
+
+ void appendSfConfigString(std::string& result) const;
+ void listLayersLocked(std::string& result) const;
+ void dumpStatsLocked(const DumpArgs& args, std::string& result) const REQUIRES(mStateLock);
+ void clearStatsLocked(const DumpArgs& args, std::string& result);
+ void dumpTimeStats(const DumpArgs& args, bool asProto, std::string& result) const;
void logFrameStats();
void dumpStaticScreenStats(std::string& result) const;
@@ -857,7 +879,16 @@
bool isLayerTripleBufferingDisabled() const {
return this->mLayerTripleBufferingDisabled;
}
- status_t doDump(int fd, const Vector<String16>& args, bool asProto);
+
+ status_t doDump(int fd, const DumpArgs& args, bool asProto);
+
+ status_t dumpCritical(int fd, const DumpArgs&, bool asProto) override {
+ return doDump(fd, DumpArgs(), asProto);
+ }
+
+ status_t dumpAll(int fd, const DumpArgs& args, bool asProto) override {
+ return doDump(fd, args, asProto);
+ }
/* ------------------------------------------------------------------------
* VrFlinger
diff --git a/services/surfaceflinger/TimeStats/TimeStats.cpp b/services/surfaceflinger/TimeStats/TimeStats.cpp
index 6a5488a..8d3776b 100644
--- a/services/surfaceflinger/TimeStats/TimeStats.cpp
+++ b/services/surfaceflinger/TimeStats/TimeStats.cpp
@@ -32,19 +32,12 @@
namespace android {
-void TimeStats::parseArgs(bool asProto, const Vector<String16>& args, size_t& index,
- std::string& result) {
+void TimeStats::parseArgs(bool asProto, const Vector<String16>& args, std::string& result) {
ATRACE_CALL();
- if (args.size() > index + 10) {
- ALOGD("Invalid args count");
- return;
- }
-
std::unordered_map<std::string, int32_t> argsMap;
- while (index < args.size()) {
+ for (size_t index = 0; index < args.size(); ++index) {
argsMap[std::string(String8(args[index]).c_str())] = index;
- ++index;
}
if (argsMap.count("-disable")) {
diff --git a/services/surfaceflinger/TimeStats/TimeStats.h b/services/surfaceflinger/TimeStats/TimeStats.h
index 71c3ed7..e8fbcab 100644
--- a/services/surfaceflinger/TimeStats/TimeStats.h
+++ b/services/surfaceflinger/TimeStats/TimeStats.h
@@ -77,7 +77,7 @@
TimeStats() = default;
~TimeStats() = default;
- void parseArgs(bool asProto, const Vector<String16>& args, size_t& index, std::string& result);
+ void parseArgs(bool asProto, const Vector<String16>& args, std::string& result);
bool isEnabled();
void incrementTotalFrames();
diff --git a/services/surfaceflinger/tests/unittests/TimeStatsTest.cpp b/services/surfaceflinger/tests/unittests/TimeStatsTest.cpp
index 86f1a39..0f95cf9 100644
--- a/services/surfaceflinger/tests/unittests/TimeStatsTest.cpp
+++ b/services/surfaceflinger/tests/unittests/TimeStatsTest.cpp
@@ -131,7 +131,6 @@
};
std::string TimeStatsTest::inputCommand(InputCommand cmd, bool useProto) {
- size_t index = 0;
std::string result;
Vector<String16> args;
@@ -162,7 +161,7 @@
ALOGD("Invalid control command");
}
- EXPECT_NO_FATAL_FAILURE(mTimeStats->parseArgs(useProto, args, index, result));
+ EXPECT_NO_FATAL_FAILURE(mTimeStats->parseArgs(useProto, args, result));
return result;
}