Merge "Fix failing puller unit tests"

cmds/statsd/src/external/StatsCallbackPuller.cpp

@@ -36,8 +36,9 @@ namespace os {
 namespace statsd {
 
 StatsCallbackPuller::StatsCallbackPuller(int tagId, const sp<IPullAtomCallback>& callback,
-                                         int64_t timeoutNs)
-    : StatsPuller(tagId), mCallback(callback), mTimeoutNs(timeoutNs) {
+                                         const int64_t coolDownNs, int64_t timeoutNs,
+                                         const vector<int> additiveFields)
+    : StatsPuller(tagId, coolDownNs, timeoutNs, additiveFields), mCallback(callback) {
     VLOG("StatsCallbackPuller created for tag %d", tagId);
 }
 
@@ -86,7 +87,7 @@ bool StatsCallbackPuller::PullInternal(vector<shared_ptr<LogEvent>>* data) {
     {
         unique_lock<mutex> unique_lk(*cv_mutex);
         // Wait until the pull finishes, or until the pull timeout.
-        cv->wait_for(unique_lk, chrono::nanoseconds(mTimeoutNs),
+        cv->wait_for(unique_lk, chrono::nanoseconds(mPullTimeoutNs),
                      [pullFinish] { return *pullFinish; });
         if (!*pullFinish) {
             // Note: The parent stats puller will also note that there was a timeout and that the

cmds/statsd/src/external/StatsCallbackPuller.h

@@ -28,12 +28,12 @@ namespace statsd {
 class StatsCallbackPuller : public StatsPuller {
 public:
     explicit StatsCallbackPuller(int tagId, const sp<IPullAtomCallback>& callback,
-                                 int64_t timeoutNs);
+                                 const int64_t coolDownNs, const int64_t timeoutNs,
+                                 const std::vector<int> additiveFields);
 
 private:
     bool PullInternal(vector<std::shared_ptr<LogEvent> >* data) override;
     const sp<IPullAtomCallback> mCallback;
-    const int64_t mTimeoutNs;
 
     FRIEND_TEST(StatsCallbackPullerTest, PullFail);
     FRIEND_TEST(StatsCallbackPullerTest, PullSuccess);

cmds/statsd/src/external/StatsPuller.cpp

@@ -32,17 +32,20 @@ using std::lock_guard;
 sp<UidMap> StatsPuller::mUidMap = nullptr;
 void StatsPuller::SetUidMap(const sp<UidMap>& uidMap) { mUidMap = uidMap; }
 
-StatsPuller::StatsPuller(const int tagId)
-    : mTagId(tagId), mLastPullTimeNs(0) {
+StatsPuller::StatsPuller(const int tagId, const int64_t coolDownNs, const int64_t pullTimeoutNs,
+                         const std::vector<int> additiveFields)
+    : mTagId(tagId),
+      mPullTimeoutNs(pullTimeoutNs),
+      mCoolDownNs(coolDownNs),
+      mAdditiveFields(additiveFields),
+      mLastPullTimeNs(0) {
 }
 
 bool StatsPuller::Pull(std::vector<std::shared_ptr<LogEvent>>* data) {
     lock_guard<std::mutex> lock(mLock);
     int64_t elapsedTimeNs = getElapsedRealtimeNs();
     StatsdStats::getInstance().notePull(mTagId);
-    const bool shouldUseCache =
-            elapsedTimeNs - mLastPullTimeNs <
-            StatsPullerManager::kAllPullAtomInfo.at({.atomTag = mTagId}).coolDownNs;
+    const bool shouldUseCache = elapsedTimeNs - mLastPullTimeNs < mCoolDownNs;
     if (shouldUseCache) {
         if (mHasGoodData) {
             (*data) = mCachedData;
@@ -64,9 +67,7 @@ bool StatsPuller::Pull(std::vector<std::shared_ptr<LogEvent>>* data) {
     }
     const int64_t pullDurationNs = getElapsedRealtimeNs() - elapsedTimeNs;
     StatsdStats::getInstance().notePullTime(mTagId, pullDurationNs);
-    const bool pullTimeOut =
-            pullDurationNs >
-            StatsPullerManager::kAllPullAtomInfo.at({.atomTag = mTagId}).pullTimeoutNs;
+    const bool pullTimeOut = pullDurationNs > mPullTimeoutNs;
     if (pullTimeOut) {
         // Something went wrong. Discard the data.
         clearCacheLocked();
@@ -78,7 +79,7 @@ bool StatsPuller::Pull(std::vector<std::shared_ptr<LogEvent>>* data) {
     }
 
     if (mCachedData.size() > 0) {
-        mapAndMergeIsolatedUidsToHostUid(mCachedData, mUidMap, mTagId);
+        mapAndMergeIsolatedUidsToHostUid(mCachedData, mUidMap, mTagId, mAdditiveFields);
     }
 
     (*data) = mCachedData;
@@ -102,8 +103,7 @@ int StatsPuller::clearCacheLocked() {
 }
 
 int StatsPuller::ClearCacheIfNecessary(int64_t timestampNs) {
-    if (timestampNs - mLastPullTimeNs >
-        StatsPullerManager::kAllPullAtomInfo.at({.atomTag = mTagId}).coolDownNs) {
+    if (timestampNs - mLastPullTimeNs > mCoolDownNs) {
         return clearCache();
     } else {
         return 0;

cmds/statsd/src/external/StatsPuller.h

@@ -32,7 +32,10 @@ namespace statsd {
 
 class StatsPuller : public virtual RefBase {
 public:
-    explicit StatsPuller(const int tagId);
+    explicit StatsPuller(const int tagId,
+                         const int64_t coolDownNs = NS_PER_SEC,
+                         const int64_t pullTimeoutNs = StatsdStats::kPullMaxDelayNs,
+                         const std::vector<int> additiveFields = std::vector<int>());
 
     virtual ~StatsPuller() {}
 
@@ -60,6 +63,12 @@ public:
 protected:
     const int mTagId;
 
+    // Max time allowed to pull this atom.
+    // We cannot reliably kill a pull thread. So we don't terminate the puller.
+    // The data is discarded if the pull takes longer than this and mHasGoodData
+    // marked as false.
+    const int64_t mPullTimeoutNs = StatsdStats::kPullMaxDelayNs;
+
 private:
     mutable std::mutex mLock;
 
@@ -68,6 +77,17 @@ private:
 
     bool mHasGoodData = false;
 
+    // Minimum time before this puller does actual pull again.
+    // Pullers can cause significant impact to system health and battery.
+    // So that we don't pull too frequently.
+    // If a pull request comes before cooldown, a cached version from previous pull
+    // will be returned.
+    const int64_t mCoolDownNs = 1 * NS_PER_SEC;
+
+    // The field numbers of the fields that need to be summed when merging
+    // isolated uid with host uid.
+    const std::vector<int> mAdditiveFields;
+
     int64_t mLastPullTimeNs;
 
     // Cache of data from last pull. If next request comes before cool down finishes,

cmds/statsd/src/external/StatsPullerManager.cpp

@@ -54,49 +54,47 @@ namespace statsd {
 // Values smaller than this may require to update the alarm.
 const int64_t NO_ALARM_UPDATE = INT64_MAX;
 
-std::map<PullerKey, PullAtomInfo> StatsPullerManager::kAllPullAtomInfo = {
+StatsPullerManager::StatsPullerManager()
+    : kAllPullAtomInfo({
+              // subsystem_sleep_state
+              {{.atomTag = android::util::SUBSYSTEM_SLEEP_STATE}, new SubsystemSleepStatePuller()},
 
-        // subsystem_sleep_state
-        {{.atomTag = android::util::SUBSYSTEM_SLEEP_STATE},
-         {.puller = new SubsystemSleepStatePuller()}},
+              // on_device_power_measurement
+              {{.atomTag = android::util::ON_DEVICE_POWER_MEASUREMENT}, new PowerStatsPuller()},
 
-        // on_device_power_measurement
-        {{.atomTag = android::util::ON_DEVICE_POWER_MEASUREMENT},
-         {.puller = new PowerStatsPuller()}},
+              // remaining_battery_capacity
+              {{.atomTag = android::util::REMAINING_BATTERY_CAPACITY},
+               new ResourceHealthManagerPuller(android::util::REMAINING_BATTERY_CAPACITY)},
 
-        // remaining_battery_capacity
-        {{.atomTag = android::util::REMAINING_BATTERY_CAPACITY},
-         {.puller = new ResourceHealthManagerPuller(android::util::REMAINING_BATTERY_CAPACITY)}},
+              // full_battery_capacity
+              {{.atomTag = android::util::FULL_BATTERY_CAPACITY},
+               new ResourceHealthManagerPuller(android::util::FULL_BATTERY_CAPACITY)},
 
-        // full_battery_capacity
-        {{.atomTag = android::util::FULL_BATTERY_CAPACITY},
-         {.puller = new ResourceHealthManagerPuller(android::util::FULL_BATTERY_CAPACITY)}},
+              // battery_voltage
+              {{.atomTag = android::util::BATTERY_VOLTAGE},
+               new ResourceHealthManagerPuller(android::util::BATTERY_VOLTAGE)},
 
-        // battery_voltage
-        {{.atomTag = android::util::BATTERY_VOLTAGE},
-         {.puller = new ResourceHealthManagerPuller(android::util::BATTERY_VOLTAGE)}},
+              // battery_level
+              {{.atomTag = android::util::BATTERY_LEVEL},
+               new ResourceHealthManagerPuller(android::util::BATTERY_LEVEL)},
 
-        // battery_level
-        {{.atomTag = android::util::BATTERY_LEVEL},
-         {.puller = new ResourceHealthManagerPuller(android::util::BATTERY_LEVEL)}},
+              // battery_cycle_count
+              {{.atomTag = android::util::BATTERY_CYCLE_COUNT},
+               new ResourceHealthManagerPuller(android::util::BATTERY_CYCLE_COUNT)},
 
-        // battery_cycle_count
-        {{.atomTag = android::util::BATTERY_CYCLE_COUNT},
-         {.puller = new ResourceHealthManagerPuller(android::util::BATTERY_CYCLE_COUNT)}},
+              // TrainInfo.
+              {{.atomTag = android::util::TRAIN_INFO}, new TrainInfoPuller()},
 
-        // TrainInfo.
-        {{.atomTag = android::util::TRAIN_INFO}, {.puller = new TrainInfoPuller()}},
+              // GpuStatsGlobalInfo
+              {{.atomTag = android::util::GPU_STATS_GLOBAL_INFO},
+               new GpuStatsPuller(android::util::GPU_STATS_GLOBAL_INFO)},
 
-        // GpuStatsGlobalInfo
-        {{.atomTag = android::util::GPU_STATS_GLOBAL_INFO},
-         {.puller = new GpuStatsPuller(android::util::GPU_STATS_GLOBAL_INFO)}},
+              // GpuStatsAppInfo
+              {{.atomTag = android::util::GPU_STATS_APP_INFO},
+               new GpuStatsPuller(android::util::GPU_STATS_APP_INFO)},
 
-        // GpuStatsAppInfo
-        {{.atomTag = android::util::GPU_STATS_APP_INFO},
-         {.puller = new GpuStatsPuller(android::util::GPU_STATS_APP_INFO)}},
-};
-
-StatsPullerManager::StatsPullerManager() : mNextPullTimeNs(NO_ALARM_UPDATE) {
+      }),
+      mNextPullTimeNs(NO_ALARM_UPDATE) {
 }
 
 bool StatsPullerManager::Pull(int tagId, vector<shared_ptr<LogEvent>>* data) {
@@ -108,7 +106,7 @@ bool StatsPullerManager::PullLocked(int tagId, vector<shared_ptr<LogEvent>>* dat
     VLOG("Initiating pulling %d", tagId);
 
     if (kAllPullAtomInfo.find({.atomTag = tagId}) != kAllPullAtomInfo.end()) {
-        bool ret = kAllPullAtomInfo.find({.atomTag = tagId})->second.puller->Pull(data);
+        bool ret = kAllPullAtomInfo.find({.atomTag = tagId})->second->Pull(data);
         VLOG("pulled %d items", (int)data->size());
         if (!ret) {
             StatsdStats::getInstance().notePullFailed(tagId);
@@ -147,7 +145,7 @@ void StatsPullerManager::SetStatsCompanionService(
     sp<IStatsCompanionService> tmpForLock = mStatsCompanionService;
     mStatsCompanionService = statsCompanionService;
     for (const auto& pulledAtom : kAllPullAtomInfo) {
-        pulledAtom.second.puller->SetStatsCompanionService(statsCompanionService);
+        pulledAtom.second->SetStatsCompanionService(statsCompanionService);
     }
     if (mStatsCompanionService != nullptr) {
         updateAlarmLocked();
@@ -279,7 +277,7 @@ void StatsPullerManager::OnAlarmFired(int64_t elapsedTimeNs) {
 int StatsPullerManager::ForceClearPullerCache() {
     int totalCleared = 0;
     for (const auto& pulledAtom : kAllPullAtomInfo) {
-        totalCleared += pulledAtom.second.puller->ForceClearCache();
+        totalCleared += pulledAtom.second->ForceClearCache();
     }
     return totalCleared;
 }
@@ -287,7 +285,7 @@ int StatsPullerManager::ForceClearPullerCache() {
 int StatsPullerManager::ClearPullerCacheIfNecessary(int64_t timestampNs) {
     int totalCleared = 0;
     for (const auto& pulledAtom : kAllPullAtomInfo) {
-        totalCleared += pulledAtom.second.puller->ClearCacheIfNecessary(timestampNs);
+        totalCleared += pulledAtom.second->ClearCacheIfNecessary(timestampNs);
     }
     return totalCleared;
 }
@@ -300,12 +298,8 @@ void StatsPullerManager::RegisterPullAtomCallback(const int uid, const int32_t a
     VLOG("RegisterPullerCallback: adding puller for tag %d", atomTag);
     // TODO: linkToDeath with the callback so that we can remove it and delete the puller.
     StatsdStats::getInstance().notePullerCallbackRegistrationChanged(atomTag, /*registered=*/true);
-    kAllPullAtomInfo[{.atomTag = atomTag}] = {
-            .additiveFields = additiveFields,
-            .coolDownNs = coolDownNs,
-            .puller = new StatsCallbackPuller(atomTag, callback, timeoutNs),
-            .pullTimeoutNs = timeoutNs,
-    };
+    kAllPullAtomInfo[{.atomTag = atomTag}] =
+            new StatsCallbackPuller(atomTag, callback, coolDownNs, timeoutNs, additiveFields);
 }
 
 void StatsPullerManager::UnregisterPullAtomCallback(const int uid, const int32_t atomTag) {

cmds/statsd/src/external/StatsPullerManager.h

@@ -36,24 +36,6 @@ namespace android {
 namespace os {
 namespace statsd {
 
-typedef struct {
-    // The field numbers of the fields that need to be summed when merging
-    // isolated uid with host uid.
-    std::vector<int> additiveFields;
-    // Minimum time before this puller does actual pull again.
-    // Pullers can cause significant impact to system health and battery.
-    // So that we don't pull too frequently.
-    // If a pull request comes before cooldown, a cached version from previous pull
-    // will be returned.
-    int64_t coolDownNs = 1 * NS_PER_SEC;
-    // The actual puller
-    sp<StatsPuller> puller;
-    // Max time allowed to pull this atom.
-    // We cannot reliably kill a pull thread. So we don't terminate the puller.
-    // The data is discarded if the pull takes longer than this and mHasGoodData
-    // marked as false.
-    int64_t pullTimeoutNs = StatsdStats::kPullMaxDelayNs;
-} PullAtomInfo;
 
 typedef struct PullerKey {
     // The uid of the process that registers this puller.
@@ -121,7 +103,7 @@ public:
 
     void UnregisterPullAtomCallback(const int uid, const int32_t atomTag);
 
-    static std::map<PullerKey, PullAtomInfo> kAllPullAtomInfo;
+    std::map<const PullerKey, sp<StatsPuller>> kAllPullAtomInfo;
 
 private:
     sp<IStatsCompanionService> mStatsCompanionService = nullptr;

cmds/statsd/src/external/puller_util.cpp

@@ -17,7 +17,6 @@
 #define DEBUG false  // STOPSHIP if true
 #include "Log.h"
 
-#include "StatsPullerManager.h"
 #include "atoms_info.h"
 #include "puller_util.h"
 
@@ -25,12 +24,7 @@ namespace android {
 namespace os {
 namespace statsd {
 
-using std::list;
-using std::map;
-using std::set;
-using std::shared_ptr;
-using std::sort;
-using std::vector;
+using namespace std;
 
 /**
  * Process all data and merge isolated with host if necessary.
@@ -54,12 +48,7 @@ using std::vector;
  * All atoms should be of the same tagId. All fields should be present.
  */
 void mapAndMergeIsolatedUidsToHostUid(vector<shared_ptr<LogEvent>>& data, const sp<UidMap>& uidMap,
-                                      int tagId) {
-    if (StatsPullerManager::kAllPullAtomInfo.find({.atomTag = tagId}) ==
-        StatsPullerManager::kAllPullAtomInfo.end()) {
-        VLOG("Unknown pull atom id %d", tagId);
-        return;
-    }
+                                      int tagId, const vector<int>& additiveFieldsVec) {
     if ((android::util::AtomsInfo::kAtomsWithAttributionChain.find(tagId) ==
          android::util::AtomsInfo::kAtomsWithAttributionChain.end()) &&
         (android::util::AtomsInfo::kAtomsWithUidField.find(tagId) ==
@@ -120,8 +109,6 @@ void mapAndMergeIsolatedUidsToHostUid(vector<shared_ptr<LogEvent>>& data, const
          });
 
     vector<shared_ptr<LogEvent>> mergedData;
-    const vector<int>& additiveFieldsVec =
-            StatsPullerManager::kAllPullAtomInfo.find({.atomTag = tagId})->second.additiveFields;
     const set<int> additiveFields(additiveFieldsVec.begin(), additiveFieldsVec.end());
     bool needMerge = true;
 

cmds/statsd/src/external/puller_util.h

@@ -26,7 +26,8 @@ namespace os {
 namespace statsd {
 
 void mapAndMergeIsolatedUidsToHostUid(std::vector<std::shared_ptr<LogEvent>>& data,
-                                      const sp<UidMap>& uidMap, int tagId);
+                                      const sp<UidMap>& uidMap, int tagId,
+                                      const vector<int>& additiveFieldsVec);
 
 }  // namespace statsd
 }  // namespace os

cmds/statsd/tests/external/StatsCallbackPuller_test.cpp

@@ -116,7 +116,7 @@ TEST_F(StatsCallbackPullerTest, PullSuccess) {
     pullSuccess = true;
     values.push_back(value);
 
-    StatsCallbackPuller puller(pullTagId, cb, pullTimeoutNs);
+    StatsCallbackPuller puller(pullTagId, cb, pullCoolDownNs, pullTimeoutNs, {});
 
     vector<std::shared_ptr<LogEvent>> dataHolder;
     int64_t startTimeNs = getElapsedRealtimeNs();
@@ -137,7 +137,7 @@ TEST_F(StatsCallbackPullerTest, PullFail) {
     int64_t value = 1234;
     values.push_back(value);
 
-    StatsCallbackPuller puller(pullTagId, cb, pullTimeoutNs);
+    StatsCallbackPuller puller(pullTagId, cb, pullCoolDownNs, pullTimeoutNs, {});
 
     vector<std::shared_ptr<LogEvent>> dataHolder;
     EXPECT_FALSE(puller.PullInternal(&dataHolder));
@@ -152,7 +152,7 @@ TEST_F(StatsCallbackPullerTest, PullTimeout) {
     int64_t value = 4321;
     values.push_back(value);
 
-    StatsCallbackPuller puller(pullTagId, cb, pullTimeoutNs);
+    StatsCallbackPuller puller(pullTagId, cb, pullCoolDownNs, pullTimeoutNs, {});
 
     vector<std::shared_ptr<LogEvent>> dataHolder;
     int64_t startTimeNs = getElapsedRealtimeNs();

cmds/statsd/tests/external/StatsPuller_test.cpp

@@ -45,7 +45,7 @@ long pullDelayNs;
 
 class FakePuller : public StatsPuller {
 public:
-    FakePuller() : StatsPuller(pullTagId){};
+    FakePuller() : StatsPuller(pullTagId, /*coolDown=*/NS_PER_SEC, /*timeout=*/NS_PER_SEC / 2){};
 
 private:
     bool PullInternal(vector<std::shared_ptr<LogEvent>>* data) override {

cmds/statsd/tests/external/puller_util_test.cpp

@@ -34,8 +34,9 @@ using testing::Contains;
 /*
  * Test merge isolated and host uid
  */
-
+namespace {
 int uidAtomTagId = android::util::CPU_CLUSTER_TIME;
+const vector<int> uidAdditiveFields = {3};
 int nonUidAtomTagId = android::util::SYSTEM_UPTIME;
 int timestamp = 1234;
 int isolatedUid = 30;
@@ -57,6 +58,7 @@ void extractIntoVector(vector<shared_ptr<LogEvent>> events,
     ret.push_back(vec);
   }
 }
+}  // anonymous namespace
 
 TEST(puller_util, MergeNoDimension) {
   vector<shared_ptr<LogEvent>> inputData;
@@ -81,7 +83,7 @@ TEST(puller_util, MergeNoDimension) {
       .WillRepeatedly(Return(hostUid));
   EXPECT_CALL(*uidMap, getHostUidOrSelf(Ne(isolatedUid)))
       .WillRepeatedly(ReturnArg<0>());
-  mapAndMergeIsolatedUidsToHostUid(inputData, uidMap, uidAtomTagId);
+  mapAndMergeIsolatedUidsToHostUid(inputData, uidMap, uidAtomTagId, uidAdditiveFields);
 
   vector<vector<int>> actual;
   extractIntoVector(inputData, actual);
@@ -121,7 +123,7 @@ TEST(puller_util, MergeWithDimension) {
       .WillRepeatedly(Return(hostUid));
   EXPECT_CALL(*uidMap, getHostUidOrSelf(Ne(isolatedUid)))
       .WillRepeatedly(ReturnArg<0>());
-  mapAndMergeIsolatedUidsToHostUid(inputData, uidMap, uidAtomTagId);
+  mapAndMergeIsolatedUidsToHostUid(inputData, uidMap, uidAtomTagId, uidAdditiveFields);
 
   vector<vector<int>> actual;
   extractIntoVector(inputData, actual);
@@ -155,7 +157,7 @@ TEST(puller_util, NoMergeHostUidOnly) {
       .WillRepeatedly(Return(hostUid));
   EXPECT_CALL(*uidMap, getHostUidOrSelf(Ne(isolatedUid)))
       .WillRepeatedly(ReturnArg<0>());
-  mapAndMergeIsolatedUidsToHostUid(inputData, uidMap, uidAtomTagId);
+  mapAndMergeIsolatedUidsToHostUid(inputData, uidMap, uidAtomTagId, uidAdditiveFields);
 
   // 20->32->31
   // 20->22->21
@@ -191,7 +193,7 @@ TEST(puller_util, IsolatedUidOnly) {
       .WillRepeatedly(Return(hostUid));
   EXPECT_CALL(*uidMap, getHostUidOrSelf(Ne(isolatedUid)))
       .WillRepeatedly(ReturnArg<0>());
-  mapAndMergeIsolatedUidsToHostUid(inputData, uidMap, uidAtomTagId);
+  mapAndMergeIsolatedUidsToHostUid(inputData, uidMap, uidAtomTagId, uidAdditiveFields);
 
   // 20->32->31
   // 20->22->21
@@ -232,7 +234,7 @@ TEST(puller_util, MultipleIsolatedUidToOneHostUid) {
 
   sp<MockUidMap> uidMap = new NaggyMock<MockUidMap>();
   EXPECT_CALL(*uidMap, getHostUidOrSelf(_)).WillRepeatedly(Return(hostUid));
-  mapAndMergeIsolatedUidsToHostUid(inputData, uidMap, uidAtomTagId);
+  mapAndMergeIsolatedUidsToHostUid(inputData, uidMap, uidAtomTagId, uidAdditiveFields);
 
   vector<vector<int>> actual;
   extractIntoVector(inputData, actual);
@@ -257,7 +259,7 @@ TEST(puller_util, NoNeedToMerge) {
   inputData.push_back(event);
 
   sp<MockUidMap> uidMap = new NaggyMock<MockUidMap>();
-  mapAndMergeIsolatedUidsToHostUid(inputData, uidMap, nonUidAtomTagId);
+  mapAndMergeIsolatedUidsToHostUid(inputData, uidMap, nonUidAtomTagId, {} /*no additive fields*/);
 
   EXPECT_EQ(2, (int)inputData.size());
 }