Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
134623002:
[chromeos] Limit the data samples stored by PowerDataCollector. (Closed)
|
Created:
6 years, 11 months ago by Siva Chandra Modified:
6 years, 11 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, oshima+watch_chromium.org, arv (Not doing code reviews) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[chromeos] Limit the data samples stored by PowerDataCollector.
BUG=332217
TBR=arv@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244435
Patch Set 1 #
Total comments: 6
Patch Set 2 : Use deque instead of list #
Total comments: 14
Patch Set 3 : Address comments #
Total comments: 7
Patch Set 4 : Address comments #Patch Set 5 : Address comments #
Total comments: 2
Patch Set 6 : Address nits #
Messages
Total messages: 19 (0 generated)
https://chromiumcodereview.appspot.com/134623002/diff/1/chromeos/power/power_... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/134623002/diff/1/chromeos/power/power_... chromeos/power/power_data_collector.h:63: static const unsigned int kSampleCountTimeLimitSec; nit: remove "count" from the name? (at least, it doesn't seem like it has anything to do with the count) https://chromiumcodereview.appspot.com/134623002/diff/1/chromeos/power/power_... chromeos/power/power_data_collector.h:77: std::list<PowerSupplySnapshot> power_supply_data_; 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.
https://chromiumcodereview.appspot.com/134623002/diff/1/chromeos/power/power_... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/134623002/diff/1/chromeos/power/power_... chromeos/power/power_data_collector.h:77: std::list<PowerSupplySnapshot> power_supply_data_; On 2014/01/10 19:40:19, Daniel Erat wrote: > 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. actually, just using std::deque is probably the best approach here. see e.g. http://www.gotw.ca/publications/mill14.htm.
PTaL at patch set 2. Using deque instead of list. https://chromiumcodereview.appspot.com/134623002/diff/1/chromeos/power/power_... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/134623002/diff/1/chromeos/power/power_... chromeos/power/power_data_collector.h:63: static const unsigned int kSampleCountTimeLimitSec; On 2014/01/10 19:40:19, Daniel Erat wrote: > nit: remove "count" from the name? (at least, it doesn't seem like it has > anything to do with the count) Done. https://chromiumcodereview.appspot.com/134623002/diff/1/chromeos/power/power_... chromeos/power/power_data_collector.h:77: std::list<PowerSupplySnapshot> power_supply_data_; On 2014/01/10 19:50:32, Daniel Erat wrote: > On 2014/01/10 19:40:19, Daniel Erat wrote: > > 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. > > actually, just using std::deque is probably the best approach here. see e.g. > http://www.gotw.ca/publications/mill14.htm. 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 :-)
https://chromiumcodereview.appspot.com/134623002/diff/60001/chromeos/power/po... File chromeos/power/power_data_collector_unittest.cc (right): https://chromiumcodereview.appspot.com/134623002/diff/60001/chromeos/power/po... chromeos/power/power_data_collector_unittest.cc:54: EXPECT_DOUBLE_EQ(prop2.battery_percent(), data2[1].battery_percent); |data2| is probably not required. |data1| and |data2| hold a reference to the same object. But it is good to keep |data2| I think. They are references to const data.
https://chromiumcodereview.appspot.com/134623002/diff/1/chromeos/power/power_... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/134623002/diff/1/chromeos/power/power_... chromeos/power/power_data_collector.h:77: std::list<PowerSupplySnapshot> power_supply_data_; On 2014/01/10 20:19:21, Siva Chandra wrote: > On 2014/01/10 19:50:32, Daniel Erat wrote: > > On 2014/01/10 19:40:19, Daniel Erat wrote: > > > 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. > > > > actually, just using std::deque is probably the best approach here. see e.g. > > http://www.gotw.ca/publications/mill14.htm. > > 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 :-) 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 https://chromiumcodereview.appspot.com/134623002/diff/60001/chromeos/power/po... File chromeos/power/power_data_collector.cc (right): https://chromiumcodereview.appspot.com/134623002/diff/60001/chromeos/power/po... chromeos/power/power_data_collector.cc:66: while (power_supply_data_.size() > 0) { nit: while (!power_supply_data_.empty()) https://chromiumcodereview.appspot.com/134623002/diff/60001/chromeos/power/po... chromeos/power/power_data_collector.cc:67: PowerSupplySnapshot first = power_supply_data_.front(); nit: const PowerSupplySnapshot& first https://chromiumcodereview.appspot.com/134623002/diff/60001/chromeos/power/po... chromeos/power/power_data_collector.cc:68: if (snapshot.time - first.time >= nit: should probably be > instead of >=, based on the constant's comment https://chromiumcodereview.appspot.com/134623002/diff/60001/chromeos/power/po... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/134623002/diff/60001/chromeos/power/po... chromeos/power/power_data_collector.h:62: // |kSampleCountTimeLimitSec| are stored in memory. nit: kSampleTimeLimitSec https://chromiumcodereview.appspot.com/134623002/diff/60001/chromeos/power/po... chromeos/power/power_data_collector.h:73: // It dumps snapshots |kSampleCountTimeLimitSec| or more older than nit: kSampleTimeLimitSec https://chromiumcodereview.appspot.com/134623002/diff/60001/chromeos/power/po... File chromeos/power/power_data_collector_unittest.cc (right): https://chromiumcodereview.appspot.com/134623002/diff/60001/chromeos/power/po... chromeos/power/power_data_collector_unittest.cc:65: power_data_collector_->AddSnapshot(snapshot1); nit: instead of making this test a friend and calling a private method, just use the public PowerChanged() interface
https://chromiumcodereview.appspot.com/134623002/diff/60001/chromeos/power/po... File chromeos/power/power_data_collector.cc (right): https://chromiumcodereview.appspot.com/134623002/diff/60001/chromeos/power/po... chromeos/power/power_data_collector.cc:66: while (power_supply_data_.size() > 0) { On 2014/01/10 21:44:14, Daniel Erat wrote: > nit: while (!power_supply_data_.empty()) Done. https://chromiumcodereview.appspot.com/134623002/diff/60001/chromeos/power/po... chromeos/power/power_data_collector.cc:67: PowerSupplySnapshot first = power_supply_data_.front(); On 2014/01/10 21:44:14, Daniel Erat wrote: > nit: const PowerSupplySnapshot& first Done. https://chromiumcodereview.appspot.com/134623002/diff/60001/chromeos/power/po... chromeos/power/power_data_collector.cc:68: if (snapshot.time - first.time >= On 2014/01/10 21:44:14, Daniel Erat wrote: > nit: should probably be > instead of >=, based on the constant's comment Done. https://chromiumcodereview.appspot.com/134623002/diff/60001/chromeos/power/po... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/134623002/diff/60001/chromeos/power/po... chromeos/power/power_data_collector.h:62: // |kSampleCountTimeLimitSec| are stored in memory. On 2014/01/10 21:44:14, Daniel Erat wrote: > nit: kSampleTimeLimitSec Done. https://chromiumcodereview.appspot.com/134623002/diff/60001/chromeos/power/po... chromeos/power/power_data_collector.h:73: // It dumps snapshots |kSampleCountTimeLimitSec| or more older than On 2014/01/10 21:44:14, Daniel Erat wrote: > nit: kSampleTimeLimitSec Done. https://chromiumcodereview.appspot.com/134623002/diff/60001/chromeos/power/po... File chromeos/power/power_data_collector_unittest.cc (right): https://chromiumcodereview.appspot.com/134623002/diff/60001/chromeos/power/po... chromeos/power/power_data_collector_unittest.cc:65: power_data_collector_->AddSnapshot(snapshot1); On 2014/01/10 21:44:14, Daniel Erat wrote: > nit: instead of making this test a friend and calling a private method, just use > the public PowerChanged() interface PowerDataCollector::PowerChanged takes a PowerSupplyProperties arg which does not have a time stamp. It inserts the time stamp into PowerSupplySnapshot when recording the sample. Since we want to test eviction of old samples here, I think we can do it either this way, or by mocking TimeTicks::Now() if we have to test using only the public interface. Do you suggest the latter approach?
https://chromiumcodereview.appspot.com/134623002/diff/60001/chromeos/power/po... File chromeos/power/power_data_collector_unittest.cc (right): https://chromiumcodereview.appspot.com/134623002/diff/60001/chromeos/power/po... chromeos/power/power_data_collector_unittest.cc:65: power_data_collector_->AddSnapshot(snapshot1); On 2014/01/10 22:18:09, Siva Chandra wrote: > On 2014/01/10 21:44:14, Daniel Erat wrote: > > nit: instead of making this test a friend and calling a private method, just > use > > the public PowerChanged() interface > > PowerDataCollector::PowerChanged takes a PowerSupplyProperties arg which does > not have a time stamp. It inserts the time stamp into PowerSupplySnapshot when > recording the sample. Since we want to test eviction of old samples here, I > think we can do it either this way, or by mocking TimeTicks::Now() if we have to > test using only the public interface. Do you suggest the latter approach? whoops, i missed that. i don't think that chrome makes it easy to mock out ::Now() (i.e. you'd need to add some set_now_for_testing() method to this class and then use that instead of ::Now() if it's set). that's kind of a pain, so calling AddSnapshot() from the test seems okay to me now. https://chromiumcodereview.appspot.com/134623002/diff/140001/chromeos/power/p... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/134623002/diff/140001/chromeos/power/p... chromeos/power/power_data_collector.h:63: static const unsigned int kSampleTimeLimitSec; nit: using 'static const int' here should be fine (i.e. default to using int unless there are particular size constraints or needs for matching the same type that's used somewhere else; otherwise use a more-specific type like uint32 or int64 or whatever) https://chromiumcodereview.appspot.com/134623002/diff/140001/chromeos/power/p... File chromeos/power/power_data_collector_unittest.cc (right): https://chromiumcodereview.appspot.com/134623002/diff/140001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:63: PowerDataCollector::kSampleTimeLimitSec); nit: this is a bit racy now; you're depending on the fact that the two base::TimeTicks::Now() calls return different values (since otherwise, the first snapshot would be retained). something like this would be better: snapshot1.time = base::TimeTicks::FromInternalValue(1000); // abritrary snapshot2.time = snapshot1.time + base::TimeDelta::FromSeconds(PowerDataCollector::kSampleTimeLimitSec + 1); https://chromiumcodereview.appspot.com/134623002/diff/140001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:73: ASSERT_EQ(static_cast<size_t>(1), data2.size()); nit: also do: EXPECT_EQ(snapshot2.time.ToInternalValue(), data2[0].time.ToInternalValue()); to verify that the second snapshot was the one that was retained?
PTaL at patch set 5. https://chromiumcodereview.appspot.com/134623002/diff/140001/chromeos/power/p... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/134623002/diff/140001/chromeos/power/p... chromeos/power/power_data_collector.h:63: static const unsigned int kSampleTimeLimitSec; On 2014/01/10 22:28:47, Daniel Erat wrote: > nit: using 'static const int' here should be fine (i.e. default to using int > unless there are particular size constraints or needs for matching the same type > that's used somewhere else; otherwise use a more-specific type like uint32 or > int64 or whatever) Done. I am assuming you are saying that the 'unsigned' qualifier is unnecessary here though your comment refers to the size of the type. https://chromiumcodereview.appspot.com/134623002/diff/140001/chromeos/power/p... File chromeos/power/power_data_collector_unittest.cc (right): https://chromiumcodereview.appspot.com/134623002/diff/140001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:63: PowerDataCollector::kSampleTimeLimitSec); On 2014/01/10 22:28:47, Daniel Erat wrote: > nit: this is a bit racy now; you're depending on the fact that the two > base::TimeTicks::Now() calls return different values (since otherwise, the first > snapshot would be retained). something like this would be better: > > snapshot1.time = base::TimeTicks::FromInternalValue(1000); // abritrary > snapshot2.time = snapshot1.time + > base::TimeDelta::FromSeconds(PowerDataCollector::kSampleTimeLimitSec + 1); Done. https://chromiumcodereview.appspot.com/134623002/diff/140001/chromeos/power/p... chromeos/power/power_data_collector_unittest.cc:73: ASSERT_EQ(static_cast<size_t>(1), data2.size()); On 2014/01/10 22:28:47, Daniel Erat wrote: > nit: also do: > > EXPECT_EQ(snapshot2.time.ToInternalValue(), > data2[0].time.ToInternalValue()); > > to verify that the second snapshot was the one that was retained? Done.
https://chromiumcodereview.appspot.com/134623002/diff/140001/chromeos/power/p... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/134623002/diff/140001/chromeos/power/p... chromeos/power/power_data_collector.h:63: static const unsigned int kSampleTimeLimitSec; On 2014/01/10 22:54:53, Siva Chandra wrote: > On 2014/01/10 22:28:47, Daniel Erat wrote: > > nit: using 'static const int' here should be fine (i.e. default to using int > > unless there are particular size constraints or needs for matching the same > type > > that's used somewhere else; otherwise use a more-specific type like uint32 or > > int64 or whatever) > > Done. I am assuming you are saying that the 'unsigned' qualifier is unnecessary > here though your comment refers to the size of the type. yeah, that's correct https://chromiumcodereview.appspot.com/134623002/diff/250001/chromeos/power/p... File chromeos/power/power_data_collector.h (right): https://chromiumcodereview.appspot.com/134623002/diff/250001/chromeos/power/p... chromeos/power/power_data_collector.h:57: // PowerManagerClient::Observer::PowerChanged implementation: nit: comment was correct before; this just describes the _interface_ that this block of methods (there's only one here) is implementing https://chromiumcodereview.appspot.com/134623002/diff/250001/chromeos/power/p... chromeos/power/power_data_collector.h:72: // Add a snapshot to |power_supply_data_|. nit: s/Add/Adds/
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sivachandra@chromium.org/134623002/330001
+r arv@ for power_ui.cc OWNERS approval.
TBR-ing is fine for minimal parts of a change, like changing names in method calls or the types of return values
On 2014/01/11 00:49:53, Daniel Erat wrote: > TBR-ing is fine for minimal parts of a change, like changing names in method > calls or the types of return values Thanks for editing and putting it back on the CQ.
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sivachandra@chromium.org/134623002/330001
Message was sent while issue was closed.
Change committed as 244435
Message was sent while issue was closed.
LGTM |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
