Revert "[TimeStats] Add callback for global stats"
This reverts commit d5065451af373b954ee660d26d0fbf7b68f4c539.
Reason for revert: submission of I6d395237082337a22f37190343a1fc59703d39bf raced with this change's presubmit, causing a build breakage.
Change-Id: I5fbb1b0e93340b6a4cd99e4731d69e0fb17e5c14
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 97bed34..b7a2c76 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -490,7 +490,6 @@
ALOGI("Boot is finished (%ld ms)", long(ns2ms(duration)) );
mFrameTracer->initialize();
- mTimeStats->onBootFinished();
// wait patiently for the window manager death
const String16 name("window");
diff --git a/services/surfaceflinger/TimeStats/Android.bp b/services/surfaceflinger/TimeStats/Android.bp
index ed0fe9e..20c2218 100644
--- a/services/surfaceflinger/TimeStats/Android.bp
+++ b/services/surfaceflinger/TimeStats/Android.bp
@@ -5,23 +5,15 @@
],
shared_libs: [
"libbase",
- "libbinder",
"libcutils",
"liblog",
"libprotobuf-cpp-lite",
- "libstatslog",
- "libstatspull",
- "libstatssocket",
"libtimestats_proto",
"libui",
"libutils",
],
export_include_dirs: ["."],
export_shared_lib_headers: [
- "libbinder",
- "libstatslog",
- "libstatspull",
- "libstatssocket",
"libtimestats_proto",
],
cppflags: [
diff --git a/services/surfaceflinger/TimeStats/TimeStats.cpp b/services/surfaceflinger/TimeStats/TimeStats.cpp
index 6520d01..a5fabf2 100644
--- a/services/surfaceflinger/TimeStats/TimeStats.cpp
+++ b/services/surfaceflinger/TimeStats/TimeStats.cpp
@@ -32,54 +32,14 @@
namespace impl {
-bool TimeStats::pullGlobalAtomCallback(int32_t atom_tag, pulled_stats_event_list* data,
- const void* cookie) {
- impl::TimeStats* timeStats =
- const_cast<impl::TimeStats*>(reinterpret_cast<const impl::TimeStats*>(cookie));
- if (atom_tag != android::util::SURFACEFLINGER_STATS_GLOBAL_INFO) {
- return false;
- }
-
- std::lock_guard<std::mutex> lock(timeStats->mMutex);
-
- const auto& stats = timeStats->mTimeStats;
- if (stats.statsStart == 0) {
- return false;
- }
- timeStats->flushPowerTimeLocked();
-
- struct stats_event* event = timeStats->mStatsDelegate->addStatsEventToPullData(data);
- timeStats->mStatsDelegate->statsEventSetAtomId(event,
- android::util::SURFACEFLINGER_STATS_GLOBAL_INFO);
- timeStats->mStatsDelegate->statsEventWriteInt64(event, stats.totalFrames);
- timeStats->mStatsDelegate->statsEventWriteInt64(event, stats.missedFrames);
- timeStats->mStatsDelegate->statsEventWriteInt64(event, stats.clientCompositionFrames);
- timeStats->mStatsDelegate->statsEventWriteInt64(event, stats.displayOnTime);
- timeStats->mStatsDelegate->statsEventWriteInt64(event, stats.presentToPresent.totalTime());
- timeStats->mStatsDelegate->statsEventBuild(event);
- timeStats->clearGlobalLocked();
-
- return true;
-}
-
TimeStats::TimeStats() {
// Temporarily enable TimeStats by default. Telemetry is disabled while
// we move onto statsd, so TimeStats is currently not exercised at all
- // during testing without enabling by default.
- // TODO: remove this, as we should only be paying this overhead on devices
- // where statsd exists.
+ // during testing.
+ // TODO: remove this.
enable();
}
-TimeStats::TimeStats(std::unique_ptr<StatsEventDelegate> statsDelegate) : TimeStats() {
- mStatsDelegate = std::move(statsDelegate);
-}
-
-void TimeStats::onBootFinished() {
- std::lock_guard<std::mutex> lock(mMutex);
- mRegisteredCallback = false;
-}
-
void TimeStats::parseArgs(bool asProto, const Vector<String16>& args, std::string& result) {
ATRACE_CALL();
@@ -105,7 +65,7 @@
}
if (argsMap.count("-clear")) {
- clearAll();
+ clear();
}
if (argsMap.count("-enable")) {
@@ -589,30 +549,6 @@
mGlobalRecord.renderEngineDurations.pop_front();
}
- // Try to register to statsd at the end of every global flush, if we haven't
- // yet.
- registerToStatsdIfNeededLocked();
-}
-
-bool TimeStats::StatsEventDelegate::checkStatsService() {
- ATRACE_CALL();
- bool ret =
- android::defaultServiceManager()->checkService(android::String16("stats")) != nullptr;
- return ret;
-}
-
-void TimeStats::registerToStatsdIfNeededLocked() {
- if (!mRegisteredCallback && mStatsDelegate->checkStatsService()) {
- // Note that this assumes that statsd will never crash. To be absolutely
- // correct we would need to register a DeathRecipient ourselves, but to
- // minimize the cost on the rendering path let's only register once as
- // soon as we know that statd has booted up.
- ALOGD("Registering statsd callback");
- mStatsDelegate
- ->registerStatsPullAtomCallback(android::util::SURFACEFLINGER_STATS_GLOBAL_INFO,
- TimeStats::pullGlobalAtomCallback, nullptr, this);
- mRegisteredCallback = true;
- }
}
void TimeStats::setPresentFenceGlobal(const std::shared_ptr<FenceTime>& presentFence) {
@@ -669,15 +605,12 @@
ALOGD("Disabled");
}
-void TimeStats::clearAll() {
- std::lock_guard<std::mutex> lock(mMutex);
- clearGlobalLocked();
- clearLayersLocked();
-}
-
-void TimeStats::clearGlobalLocked() {
+void TimeStats::clear() {
ATRACE_CALL();
+ std::lock_guard<std::mutex> lock(mMutex);
+ mTimeStatsTracker.clear();
+ mTimeStats.stats.clear();
mTimeStats.statsStart = (mEnabled.load() ? static_cast<int64_t>(std::time(0)) : 0);
mTimeStats.statsEnd = 0;
mTimeStats.totalFrames = 0;
@@ -691,15 +624,7 @@
mPowerTime.prevTime = systemTime();
mGlobalRecord.prevPresentTime = 0;
mGlobalRecord.presentFences.clear();
- ALOGD("Cleared global stats");
-}
-
-void TimeStats::clearLayersLocked() {
- ATRACE_CALL();
-
- mTimeStatsTracker.clear();
- mTimeStats.stats.clear();
- ALOGD("Cleared layer stats");
+ ALOGD("Cleared");
}
bool TimeStats::isEnabled() {
diff --git a/services/surfaceflinger/TimeStats/TimeStats.h b/services/surfaceflinger/TimeStats/TimeStats.h
index 94c24ea..65e5cf4 100644
--- a/services/surfaceflinger/TimeStats/TimeStats.h
+++ b/services/surfaceflinger/TimeStats/TimeStats.h
@@ -16,11 +16,7 @@
#pragma once
-#include <binder/IServiceManager.h>
#include <hardware/hwcomposer_defs.h>
-#include <stats_event.h>
-#include <stats_pull_atom_callback.h>
-#include <statslog.h>
#include <timestatsproto/TimeStatsHelper.h>
#include <timestatsproto/TimeStatsProtoHeader.h>
#include <ui/FenceTime.h>
@@ -41,10 +37,6 @@
public:
virtual ~TimeStats() = default;
- // Called once boot has been finished to perform additional capabilities,
- // e.g. registration to statsd.
- virtual void onBootFinished() = 0;
-
virtual void parseArgs(bool asProto, const Vector<String16>& args, std::string& result) = 0;
virtual bool isEnabled() = 0;
virtual std::string miniDump() = 0;
@@ -139,40 +131,6 @@
public:
TimeStats();
- // Delegate to the statsd service and associated APIs.
- // Production code may use this class directly, whereas unit test may define
- // a subclass for ease of testing.
- class StatsEventDelegate {
- public:
- virtual ~StatsEventDelegate() = default;
- virtual struct stats_event* addStatsEventToPullData(pulled_stats_event_list* data) {
- return add_stats_event_to_pull_data(data);
- }
- virtual void registerStatsPullAtomCallback(int32_t atom_tag,
- stats_pull_atom_callback_t callback,
- pull_atom_metadata* metadata, void* cookie) {
- return register_stats_pull_atom_callback(atom_tag, callback, metadata, cookie);
- }
-
- // Check if the statsd daemon exists, as otherwise callback registration
- // will silently fail.
- virtual bool checkStatsService();
-
- virtual void statsEventSetAtomId(struct stats_event* event, int32_t atom_id) {
- return stats_event_set_atom_id(event, atom_id);
- }
-
- virtual void statsEventWriteInt64(struct stats_event* event, int64_t field) {
- return stats_event_write_int64(event, field);
- }
-
- virtual void statsEventBuild(struct stats_event* event) { return stats_event_build(event); }
- };
- // For testing only for injecting custom dependencies.
- TimeStats(std::unique_ptr<StatsEventDelegate> statsDelegate);
-
- void onBootFinished() override;
-
void parseArgs(bool asProto, const Vector<String16>& args, std::string& result) override;
bool isEnabled() override;
std::string miniDump() override;
@@ -209,19 +167,14 @@
static const size_t MAX_NUM_TIME_RECORDS = 64;
private:
- static bool pullGlobalAtomCallback(int32_t atom_tag, pulled_stats_event_list* data,
- const void* cookie);
bool recordReadyLocked(int32_t layerId, TimeRecord* timeRecord);
void flushAvailableRecordsToStatsLocked(int32_t layerId);
void flushPowerTimeLocked();
void flushAvailableGlobalRecordsToStatsLocked();
- void registerToStatsdIfNeededLocked();
void enable();
void disable();
- void clearAll();
- void clearGlobalLocked();
- void clearLayersLocked();
+ void clear();
void dump(bool asProto, std::optional<uint32_t> maxLayers, std::string& result);
std::atomic<bool> mEnabled = false;
@@ -234,10 +187,6 @@
static const size_t MAX_NUM_LAYER_RECORDS = 200;
static const size_t MAX_NUM_LAYER_STATS = 200;
- // Default is true, so that registration doesn't happen until the device has
- // been booted.
- bool mRegisteredCallback = true;
- std::unique_ptr<StatsEventDelegate> mStatsDelegate = std::make_unique<StatsEventDelegate>();
};
} // namespace impl
diff --git a/services/surfaceflinger/tests/unittests/Android.bp b/services/surfaceflinger/tests/unittests/Android.bp
index b43e939..e4ef19e 100644
--- a/services/surfaceflinger/tests/unittests/Android.bp
+++ b/services/surfaceflinger/tests/unittests/Android.bp
@@ -82,7 +82,6 @@
"perfetto_trace_protos",
],
shared_libs: [
- "libstatssocket",
"libsurfaceflinger",
"libtimestats",
"libtimestats_proto",
diff --git a/services/surfaceflinger/tests/unittests/TimeStatsTest.cpp b/services/surfaceflinger/tests/unittests/TimeStatsTest.cpp
index ec27201..3e808c0 100644
--- a/services/surfaceflinger/tests/unittests/TimeStatsTest.cpp
+++ b/services/surfaceflinger/tests/unittests/TimeStatsTest.cpp
@@ -36,9 +36,7 @@
namespace android {
namespace {
-using testing::_;
using testing::Contains;
-using testing::InSequence;
using testing::SizeIs;
using testing::UnorderedElementsAre;
@@ -134,41 +132,7 @@
}
std::mt19937 mRandomEngine = std::mt19937(std::random_device()());
-
- class FakeStatsEventDelegate : public impl::TimeStats::StatsEventDelegate {
- public:
- FakeStatsEventDelegate() = default;
- ~FakeStatsEventDelegate() override = default;
-
- struct stats_event* addStatsEventToPullData(pulled_stats_event_list*) override {
- return mEvent;
- }
- void registerStatsPullAtomCallback(int32_t atom_tag, stats_pull_atom_callback_t callback,
- pull_atom_metadata*, void* cookie) override {
- mAtomTag = atom_tag;
- mCallback = callback;
- mCookie = cookie;
- }
-
- bool checkStatsService() override { return true; }
-
- bool makePullAtomCallback(int32_t atom_tag, const void* cookie) {
- return (*mCallback)(atom_tag, nullptr, cookie);
- }
-
- MOCK_METHOD2(statsEventSetAtomId, void(struct stats_event*, int32_t));
- MOCK_METHOD2(statsEventWriteInt64, void(struct stats_event*, int64_t));
- MOCK_METHOD1(statsEventBuild, void(struct stats_event*));
-
- struct stats_event* mEvent = stats_event_obtain();
- int32_t mAtomTag = 0;
- stats_pull_atom_callback_t mCallback = nullptr;
- void* mCookie = nullptr;
- };
-
- FakeStatsEventDelegate* mDelegate = new FakeStatsEventDelegate;
- std::unique_ptr<TimeStats> mTimeStats =
- std::make_unique<impl::TimeStats>(std::unique_ptr<FakeStatsEventDelegate>(mDelegate));
+ std::unique_ptr<TimeStats> mTimeStats = std::make_unique<impl::TimeStats>();
};
std::string TimeStatsTest::inputCommand(InputCommand cmd, bool useProto) {
@@ -663,66 +627,6 @@
ASSERT_EQ(0, globalProto.stats_size());
}
-TEST_F(TimeStatsTest, globalStatsCallback_disabledBeforeBoot) {
- EXPECT_TRUE(inputCommand(InputCommand::ENABLE, FMT_STRING).empty());
-
- mTimeStats->setPowerMode(HWC_POWER_MODE_NORMAL);
- mTimeStats->setPresentFenceGlobal(std::make_shared<FenceTime>(3000000));
- mTimeStats->setPresentFenceGlobal(std::make_shared<FenceTime>(5000000));
-
- // Callback should not be registered after flushing global stats
- EXPECT_EQ(nullptr, mDelegate->mCallback);
-}
-
-TEST_F(TimeStatsTest, globalStatsCallback_worksAfterBoot) {
- constexpr size_t TOTAL_FRAMES = 5;
- constexpr size_t MISSED_FRAMES = 4;
- constexpr size_t CLIENT_COMPOSITION_FRAMES = 3;
-
- EXPECT_TRUE(inputCommand(InputCommand::ENABLE, FMT_STRING).empty());
- mTimeStats->onBootFinished();
-
- for (size_t i = 0; i < TOTAL_FRAMES; i++) {
- mTimeStats->incrementTotalFrames();
- }
- for (size_t i = 0; i < MISSED_FRAMES; i++) {
- mTimeStats->incrementMissedFrames();
- }
- for (size_t i = 0; i < CLIENT_COMPOSITION_FRAMES; i++) {
- mTimeStats->incrementClientCompositionFrames();
- }
-
- mTimeStats->setPowerMode(HWC_POWER_MODE_NORMAL);
- mTimeStats->setPresentFenceGlobal(std::make_shared<FenceTime>(3000000));
- mTimeStats->setPresentFenceGlobal(std::make_shared<FenceTime>(5000000));
-
- EXPECT_EQ(android::util::SURFACEFLINGER_STATS_GLOBAL_INFO, mDelegate->mAtomTag);
- EXPECT_NE(nullptr, mDelegate->mCallback);
- EXPECT_EQ(mTimeStats.get(), mDelegate->mCookie);
-
- {
- InSequence seq;
- EXPECT_CALL(*mDelegate,
- statsEventSetAtomId(mDelegate->mEvent,
- android::util::SURFACEFLINGER_STATS_GLOBAL_INFO));
- EXPECT_CALL(*mDelegate, statsEventWriteInt64(mDelegate->mEvent, TOTAL_FRAMES));
- EXPECT_CALL(*mDelegate, statsEventWriteInt64(mDelegate->mEvent, MISSED_FRAMES));
- EXPECT_CALL(*mDelegate, statsEventWriteInt64(mDelegate->mEvent, CLIENT_COMPOSITION_FRAMES));
- EXPECT_CALL(*mDelegate, statsEventWriteInt64(mDelegate->mEvent, _));
- EXPECT_CALL(*mDelegate, statsEventWriteInt64(mDelegate->mEvent, 2));
- EXPECT_CALL(*mDelegate, statsEventBuild(mDelegate->mEvent));
- }
- mDelegate->makePullAtomCallback(mDelegate->mAtomTag, mDelegate->mCookie);
-
- SFTimeStatsGlobalProto globalProto;
- ASSERT_TRUE(globalProto.ParseFromString(inputCommand(InputCommand::DUMP_ALL, FMT_PROTO)));
-
- EXPECT_EQ(0, globalProto.total_frames());
- EXPECT_EQ(0, globalProto.missed_frames());
- EXPECT_EQ(0, globalProto.client_composition_frames());
- EXPECT_EQ(0, globalProto.present_to_present_size());
-}
-
TEST_F(TimeStatsTest, canSurviveMonkey) {
if (g_noSlowTests) {
GTEST_SKIP();
diff --git a/services/surfaceflinger/tests/unittests/mock/MockTimeStats.h b/services/surfaceflinger/tests/unittests/mock/MockTimeStats.h
index 9ada5ef..ec74a42 100644
--- a/services/surfaceflinger/tests/unittests/mock/MockTimeStats.h
+++ b/services/surfaceflinger/tests/unittests/mock/MockTimeStats.h
@@ -28,7 +28,6 @@
TimeStats();
~TimeStats() override;
- MOCK_METHOD0(onBootFinished, void());
MOCK_METHOD3(parseArgs, void(bool, const Vector<String16>&, std::string&));
MOCK_METHOD0(isEnabled, bool());
MOCK_METHOD0(miniDump, std::string());