[chromeos] Limit the data samples stored by PowerDataCollector.

chromeos/power/power_data_collector.h

Index: chromeos/power/power_data_collector.h
diff --git a/chromeos/power/power_data_collector.h b/chromeos/power/power_data_collector.h
index 581faa0edf9163088547f8ff823d96f8d4a410a2..73faff1e7ee62bd5c9ccb32bd52533c1ddcfadce 100644
--- a/chromeos/power/power_data_collector.h
+++ b/chromeos/power/power_data_collector.h
@@ -5,10 +5,11 @@
 #ifndef CHROMEOS_POWER_POWER_DATA_COLLECTOR_H_
 #define CHROMEOS_POWER_POWER_DATA_COLLECTOR_H_
 
-#include <vector>
+#include <list>
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
+#include "base/gtest_prod_util.h"
 #include "base/time/time.h"
 #include "chromeos/chromeos_export.h"
 #include "chromeos/dbus/power_manager_client.h"
@@ -39,7 +40,7 @@ class CHROMEOS_EXPORT PowerDataCollector : public PowerManagerClient::Observer {
     double battery_percent;
   };
 
-  const std::vector<PowerSupplySnapshot>& power_supply_data() const {
+  const std::list<PowerSupplySnapshot>& power_supply_data() const {
     return power_supply_data_;
   }
 
@@ -53,16 +54,27 @@ class CHROMEOS_EXPORT PowerDataCollector : public PowerManagerClient::Observer {
   // Returns the global instance of PowerDataCollector.
   static PowerDataCollector* Get();
 
-  // PowerManagerClient::Observer implementation:
+  // PowerManagerClient::Observer::PowerChanged implementation:
   virtual void PowerChanged(
       const power_manager::PowerSupplyProperties& prop) OVERRIDE;
 
+  // Only those power data samples which fall within the last
+  // |kSampleCountTimeLimitSec| are stored in memory.
+  static const unsigned int kSampleCountTimeLimitSec;

[Daniel Erat, 2014/01/10 19:40:19]

nit: remove "count" from the name? (at least, it doesn't seem like it has
anything to do with the count)

[Siva Chandra, 2014/01/10 20:19:21]

Done.

+
  private:
+  FRIEND_TEST_ALL_PREFIXES(PowerDataCollectorTest, AddSnapshot);
+
   PowerDataCollector();
 
   virtual ~PowerDataCollector();
 
-  std::vector<PowerSupplySnapshot> power_supply_data_;
+  // Add a snapshot to |power_supply_data_|.
+  // It dumps snapshots |kSampleCountTimeLimitSec| or more older than
+  // |snapshot|.
+  void AddSnapshot(const PowerSupplySnapshot& snapshot);
+
+  std::list<PowerSupplySnapshot> power_supply_data_;

[Daniel Erat, 2014/01/10 19:40:19]

using std::list (which is a double-linked list) will significantly increase the
memory usage here. did you see the ring buffer template that i linked to
earlier? you should be able to move that into base/ so you can use it here.

if it doesn't support pruning entries from the beginning of the buffer and you
feel like that's needed instead of just capping the maximum number of entries,
it shouldn't be more than a few lines of code to implement a ring buffer
yourself within this class (e.g. add |first_index_| and |last_index_| members,
etc.)

one downside to rolling your own implementation is that users of this class may
need to handle wrapping around the buffer themselves, but since there's only one
consumer, that's probably not a big deal either.

[Daniel Erat, 2014/01/10 19:50:32]

actually, just using std::deque is probably the best approach here. see e.g.
http://www.gotw.ca/publications/mill14.htm.

[Siva Chandra, 2014/01/10 20:19:21]

I was about to ask you about deque!

Your last comment on the bug seemed to indicate that the type/size of limit was
not important as long as there was a limit. You also said I can do the simplest
I can to enforce a limit. So, list was an obvious personal choice :-)

[Daniel Erat, 2014/01/10 21:44:13]

well, i meant not to stress about keeping one day versus two days. adding at
least 16 bytes of pointer overhead for each (nominally) 17-byte struct seems a
bit excessive. :-P

 
   DISALLOW_COPY_AND_ASSIGN(PowerDataCollector);
 };