Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
472383002:
Add ProcessPowerCollector to audit power information. (Closed)
|
Created:
6 years, 4 months ago by Daniel Nishi Modified:
6 years, 3 months ago CC:
chromium-reviews, tfarina, darin-cc_chromium.org, jam, chromium-apps-reviews_chromium.org, scheib, Siva Chandra, Daniel Erat Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd ProcessPowerCollector to audit CPU and power information.
This is part of the battery auditing feature, which will be surfaced in the Website Settings options page in the next patch.
BUG=372598
Committed: https://crrev.com/3cf1d2aa622e9513c593fff948b6df149737e8d2
Cr-Commit-Position: refs/heads/master@{#291999}
Patch Set 1 : #Patch Set 2 : Audit power in chrome_browser. #
Total comments: 26
Patch Set 3 : Comments and fixing some memory issues. #
Total comments: 32
Patch Set 4 : #
Total comments: 19
Patch Set 5 : Merge methods, hopefully fix Windows tests. #
Total comments: 20
Patch Set 6 : #
Total comments: 27
Patch Set 7 : Unittest clean-up. #
Total comments: 17
Patch Set 8 : Use PowerManagerClient. #
Total comments: 24
Patch Set 9 : #
Total comments: 2
Patch Set 10 : Shutdown() no longer exists -- now in d'tor. #
Total comments: 26
Patch Set 11 : Observer now balanced. #Patch Set 12 : Missed one move. #
Total comments: 6
Patch Set 13 : Generalize CrOS values. #
Total comments: 16
Patch Set 14 : #
Total comments: 2
Patch Set 15 : #Patch Set 16 : Fix a lifetime issue where DBusThreadManager could not exist when ProcessPowerCollector is destroye… #Patch Set 17 : iOS doesn't actually run chrome_browser_main, so removed ifdefs related to ios. #
Messages
Total messages: 44 (0 generated)
asargent: PTAL at apps/app_window.h sky: PTAL at rest. +scheib, derat, sivachandra FYI
apps/ lgtm
i didn't look at the tests yet https://codereview.chromium.org/472383002/diff/80001/apps/app_window.h File apps/app_window.h (right): https://codereview.chromium.org/472383002/diff/80001/apps/app_window.h#newcod... apps/app_window.h:350: void SetAppWindowContentsForTesting(AppWindowContents* contents) { nit: pass a scoped_ptr<AppWindowContents> so the ownership transfer is self-documenting https://codereview.chromium.org/472383002/diff/80001/chrome/browser/chrome_br... File chrome/browser/chrome_browser_main.h (right): https://codereview.chromium.org/472383002/diff/80001/chrome/browser/chrome_br... chrome/browser/chrome_browser_main.h:154: #if !defined(OS_ANDROID) || !defined(OS_IOS) have you tried to compile this on either android or ios? i suspect that you meant && here https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... chrome/browser/power/process_power_collector.cc:42: update_requests_++; why do you need to track the number of updates? if you use a timer (see below), you can just add a DCHECK() here that it isn't already running https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... chrome/browser/power/process_power_collector.cc:56: base::MessageLoop::current()->PostDelayedTask( any reason not to use base::RepeatingTimer instead? you won't need the WeakPtrFactory then. https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... chrome/browser/power/process_power_collector.cc:121: chromeos::PowerDataCollector* power_data_collector = can you just observe chromeos/dbus/power_manager_client.h here for PowerChanged()? seems more direct than adding a dependency on PowerDataCollector https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... chrome/browser/power/process_power_collector.cc:130: if (sample.external_power) { nit: omit curly brackets https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... chrome/browser/power/process_power_collector.cc:133: #endif shouldn't you also be checking whether we're on battery power on other systems? https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... chrome/browser/power/process_power_collector.cc:141: site_delta = site_delta * sample.battery_discharge_rate / cpu_cycle; i don't understand this code -- what does this scaling do, and why is it only present on chrome os? https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... File chrome/browser/power/process_power_collector.h (right): https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... chrome/browser/power/process_power_collector.h:45: ~PerProcessData() {} nit: add a blank line after this one https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... chrome/browser/power/process_power_collector.h:46: base::ProcessMetrics* metrics; nit: add comments documenting ownership of |metrics| and |profile| https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... chrome/browser/power/process_power_collector.h:56: scoped_ptr<ProcessMetricsMap> metrics_map_; move this data member declaration below all of the private methods https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... chrome/browser/power/process_power_collector.h:62: // Update the |metrics_map_| with currently active processes. nit: s/Update/Updates/
https://codereview.chromium.org/472383002/diff/80001/apps/app_window.h File apps/app_window.h (right): https://codereview.chromium.org/472383002/diff/80001/apps/app_window.h#newcod... apps/app_window.h:350: void SetAppWindowContentsForTesting(AppWindowContents* contents) { On 2014/08/15 20:22:12, Daniel Erat wrote: > nit: pass a scoped_ptr<AppWindowContents> so the ownership transfer is > self-documenting Done. https://codereview.chromium.org/472383002/diff/80001/chrome/browser/chrome_br... File chrome/browser/chrome_browser_main.h (right): https://codereview.chromium.org/472383002/diff/80001/chrome/browser/chrome_br... chrome/browser/chrome_browser_main.h:154: #if !defined(OS_ANDROID) || !defined(OS_IOS) On 2014/08/15 20:22:12, Daniel Erat wrote: > have you tried to compile this on either android or ios? i suspect that you > meant && here I haven't tried that yet, but you are correct. I started up some bots to make sure this works. https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... chrome/browser/power/process_power_collector.cc:42: update_requests_++; On 2014/08/15 20:22:12, Daniel Erat wrote: > why do you need to track the number of updates? if you use a timer (see below), > you can just add a DCHECK() here that it isn't already running Done. https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... chrome/browser/power/process_power_collector.cc:56: base::MessageLoop::current()->PostDelayedTask( On 2014/08/15 20:22:12, Daniel Erat wrote: > any reason not to use base::RepeatingTimer instead? you won't need the > WeakPtrFactory then. Done. https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... chrome/browser/power/process_power_collector.cc:121: chromeos::PowerDataCollector* power_data_collector = On 2014/08/15 20:22:12, Daniel Erat wrote: > can you just observe chromeos/dbus/power_manager_client.h here for > PowerChanged()? seems more direct than adding a dependency on PowerDataCollector The only power event this needs access to is the most recent one. Observing for PowerChanged() seems like it'd wake up this class for mostly power events that wouldn't get used. https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... chrome/browser/power/process_power_collector.cc:130: if (sample.external_power) { On 2014/08/15 20:22:12, Daniel Erat wrote: > nit: omit curly brackets Done. https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... chrome/browser/power/process_power_collector.cc:133: #endif On 2014/08/15 20:22:12, Daniel Erat wrote: > shouldn't you also be checking whether we're on battery power on other systems? The API for monitoring battery status on other platforms right now is currently in development. I'm keeping my eye on content/browser/battery_status/. https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... chrome/browser/power/process_power_collector.cc:141: site_delta = site_delta * sample.battery_discharge_rate / cpu_cycle; On 2014/08/15 20:22:12, Daniel Erat wrote: > i don't understand this code -- what does this scaling do, and why is it only > present on chrome os? This attributes the actual discharge rate of the system proportionally to the origins. We can only query the battery discharge in CrOS right now, so it's a CrOS only heuristic. https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... File chrome/browser/power/process_power_collector.h (right): https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... chrome/browser/power/process_power_collector.h:45: ~PerProcessData() {} On 2014/08/15 20:22:13, Daniel Erat wrote: > nit: add a blank line after this one Done. https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... chrome/browser/power/process_power_collector.h:46: base::ProcessMetrics* metrics; On 2014/08/15 20:22:13, Daniel Erat wrote: > nit: add comments documenting ownership of |metrics| and |profile| Done. https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... chrome/browser/power/process_power_collector.h:56: scoped_ptr<ProcessMetricsMap> metrics_map_; On 2014/08/15 20:22:13, Daniel Erat wrote: > move this data member declaration below all of the private methods Done. https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... chrome/browser/power/process_power_collector.h:62: // Update the |metrics_map_| with currently active processes. On 2014/08/15 20:22:13, Daniel Erat wrote: > nit: s/Update/Updates/ Done.
(still haven't looked at tests yet) https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... chrome/browser/power/process_power_collector.cc:133: #endif On 2014/08/18 17:38:48, Daniel Nishi wrote: > On 2014/08/15 20:22:12, Daniel Erat wrote: > > shouldn't you also be checking whether we're on battery power on other > systems? > > The API for monitoring battery status on other platforms right now is currently > in development. I'm keeping my eye on content/browser/battery_status/. what about base/power_monitor/power_monitor.h? sadly, it doesn't work on chrome os: http://crbug.com/326534 https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:58: if (render_process->GetBrowserContext()->IsOffTheRecord()) { nit: omit curly brackets https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:125: while (it != metrics_map_->end()) { nit: just use a for loop here https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:126: double site_delta = it->second->last_cpu; please give this variable a better name -- is it really e.g. last_process_cpu_percent? https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:151: (*metrics_map_)[handle] = PerProcessData( does this even compile on mac? how about restructuring it like this: (*metrics_map_)[handle] = linked_ptr<PerProcessData>(new PerProcessData( #if defined(OS_MACOSX) base::ProcessMetrics::CreateProcessMetrics(handle, NULL) #else base::ProcessMetrics::CreateProcessMetrics(handle) #endif )); to make it obvious why the ifdef is needed to limit the amonut of duplicated code? https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:158: (*metrics_map_)[handle]->last_origin = origin; nit: copy the linked_ptr to a local variable so you don't need to do a bunch of repetitive lookups here https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.h (right): https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:43: explicit PerProcessData(base::ProcessMetrics* metrics) scoped_ptr https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:44: : metrics(metrics), last_cpu(-1) {} nit: one per line if they don't all fit on the same line as the signature https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:49: base::ProcessMetrics* metrics; make this be a scoped_ptr since it's owned by this class (which should be a class with underscore-suffixed members and DISALLOW_COPY_AND_ASSIGN instead; otherwise it's easy to accidentally copy the struct and double-free the metrics) https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:53: GURL last_origin; please add comments documenting what all of these fields represent https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:54: double total_usage; i don't see this member being used anywhere in the .cc file https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:55: int last_cpu; last_cpu_percent? https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:75: void RecordCpuUsageByOrigin(double cpu_cycle); i think that the difference between "Populate" and "Record" in this method and the previous one is non-obvious. since they both work with the same data and only get called back-to-back, would you be okay with merging them into a single method? https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:80: // Adds the information from a given RenderProcessHost to the map at the for nit: fix grammar in this comment (should it be "... to |metrics_map_| for a given origin"?) https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:85: ProcessMetricsMap* GetMetricsMapForTesting() { return metrics_map_.get(); } nit: make this public and rename it to metrics_map_for_testing(); can the map also be const? https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:87: scoped_ptr<ProcessMetricsMap> metrics_map_; does this really need to be a scoped_ptr? if not, just do: ProcessMetricsMap metrics_map_; https://codereview.chromium.org/472383002/diff/100001/content/public/test/moc... File content/public/test/mock_render_process_host.h (right): https://codereview.chromium.org/472383002/diff/100001/content/public/test/moc... content/public/test/mock_render_process_host.h:125: scoped_ptr<int> process_handle; is this portable across platforms? base/process/process_handle.h looks like it uses a pointer on windows, for instance. why can't you just store a base::ProcessHandle here?
https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/80001/chrome/browser/power/pro... chrome/browser/power/process_power_collector.cc:133: #endif On 2014/08/18 20:43:04, Daniel Erat wrote: > On 2014/08/18 17:38:48, Daniel Nishi wrote: > > On 2014/08/15 20:22:12, Daniel Erat wrote: > > > shouldn't you also be checking whether we're on battery power on other > > systems? > > > > The API for monitoring battery status on other platforms right now is > currently > > in development. I'm keeping my eye on content/browser/battery_status/. > > what about base/power_monitor/power_monitor.h? sadly, it doesn't work on chrome > os: http://crbug.com/326534 It's probably worth saying that the main reason we stop collecting power information on battery power is that CrOS reports a negative discharge rate during charging, making power attribution via discharge rate impossible. We don't have that problem in other OSes for their heuristics, so I'm not sure if it is worth stopping when on battery. It looks like it'd work, though, if we wanted to go that route on those platforms. https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:58: if (render_process->GetBrowserContext()->IsOffTheRecord()) { On 2014/08/18 20:43:04, Daniel Erat wrote: > nit: omit curly brackets Done. https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:125: while (it != metrics_map_->end()) { On 2014/08/18 20:43:04, Daniel Erat wrote: > nit: just use a for loop here Done. https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:126: double site_delta = it->second->last_cpu; On 2014/08/18 20:43:04, Daniel Erat wrote: > please give this variable a better name -- is it really e.g. > last_process_cpu_percent? On non-CrOS platforms, that's correct. It's scaled by system power consumption in CrOS, though. I've changed the name to try and best capture that meaning. https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:151: (*metrics_map_)[handle] = PerProcessData( On 2014/08/18 20:43:04, Daniel Erat wrote: > does this even compile on mac? how about restructuring it like this: > > (*metrics_map_)[handle] = linked_ptr<PerProcessData>(new PerProcessData( > #if defined(OS_MACOSX) > base::ProcessMetrics::CreateProcessMetrics(handle, NULL) > #else > base::ProcessMetrics::CreateProcessMetrics(handle) > #endif > )); > > to make it obvious why the ifdef is needed to limit the amonut of duplicated > code? SGTM. I'm trying to pick up a OS X box, but I'll use tryjobs in the future to try and avoid this problem. https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:158: (*metrics_map_)[handle]->last_origin = origin; On 2014/08/18 20:43:04, Daniel Erat wrote: > nit: copy the linked_ptr to a local variable so you don't need to do a bunch of > repetitive lookups here Is this fine now that most of the lookups have gotten wrapped into a constructor? https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.h (right): https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:43: explicit PerProcessData(base::ProcessMetrics* metrics) On 2014/08/18 20:43:05, Daniel Erat wrote: > scoped_ptr Done. https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:44: : metrics(metrics), last_cpu(-1) {} On 2014/08/18 20:43:04, Daniel Erat wrote: > nit: one per line if they don't all fit on the same line as the signature Done. https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:49: base::ProcessMetrics* metrics; On 2014/08/18 20:43:04, Daniel Erat wrote: > make this be a scoped_ptr since it's owned by this class (which should be a > class with underscore-suffixed members and DISALLOW_COPY_AND_ASSIGN instead; > otherwise it's easy to accidentally copy the struct and double-free the metrics) Done. https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:53: GURL last_origin; On 2014/08/18 20:43:04, Daniel Erat wrote: > please add comments documenting what all of these fields represent Done. https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:54: double total_usage; On 2014/08/18 20:43:04, Daniel Erat wrote: > i don't see this member being used anywhere in the .cc file You're right -- that functionality got moved into the OriginPowerMap. Done. https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:55: int last_cpu; On 2014/08/18 20:43:04, Daniel Erat wrote: > last_cpu_percent? Done. https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:75: void RecordCpuUsageByOrigin(double cpu_cycle); On 2014/08/18 20:43:04, Daniel Erat wrote: > i think that the difference between "Populate" and "Record" in this method and > the previous one is non-obvious. since they both work with the same data and > only get called back-to-back, would you be okay with merging them into a single > method? I originally had them merged, but I split it into two functions so I could use test values between the population and the recording. https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:80: // Adds the information from a given RenderProcessHost to the map at the for On 2014/08/18 20:43:04, Daniel Erat wrote: > nit: fix grammar in this comment (should it be "... to |metrics_map_| for a > given origin"?) Done. https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:85: ProcessMetricsMap* GetMetricsMapForTesting() { return metrics_map_.get(); } On 2014/08/18 20:43:04, Daniel Erat wrote: > nit: make this public and rename it to metrics_map_for_testing(); can the map > also be const? Renamed. This also results in PerProcessData and the ProcessMetricsMap typedef moving to public as well. It can't be const because we modify the values inside the map to make mock CPU usage during testing. https://codereview.chromium.org/472383002/diff/100001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:87: scoped_ptr<ProcessMetricsMap> metrics_map_; On 2014/08/18 20:43:05, Daniel Erat wrote: > does this really need to be a scoped_ptr? if not, just do: > > ProcessMetricsMap metrics_map_; Done. https://codereview.chromium.org/472383002/diff/100001/content/public/test/moc... File content/public/test/mock_render_process_host.h (right): https://codereview.chromium.org/472383002/diff/100001/content/public/test/moc... content/public/test/mock_render_process_host.h:125: scoped_ptr<int> process_handle; On 2014/08/18 20:43:05, Daniel Erat wrote: > is this portable across platforms? base/process/process_handle.h looks like it > uses a pointer on windows, for instance. why can't you just store a > base::ProcessHandle here? Done.
some more high-level comments https://codereview.chromium.org/472383002/diff/180001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/180001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:103: if (it->second->last_cpu_used() < 0) { should you be doing this after you update the last percent instead of before it? (i admit that this is in conflict with the next comment; perhaps i'm voting for the previous method and this method to be merged) https://codereview.chromium.org/472383002/diff/180001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:104: metrics_map_.erase(it++); how about moving the stale-process-clearing to the method that's currently named UpdateMetricsMap()? it feels strange to add processes in one method but delete them somewhere else instead of having a single method that ensure the map holds the correct entries. https://codereview.chromium.org/472383002/diff/180001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:110: if (it->second->last_cpu_used() < 0) { hmm, so you skip negative usages here. this seems weird; won't you just erase the process the next time around? is this code all depending on undocumented (i think) behavior where ProcessMonitor returns -1 if the process has gone away? if so, please find a safer way to detect this case. https://codereview.chromium.org/472383002/diff/180001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.h (right): https://codereview.chromium.org/472383002/diff/180001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:31: explicit PerProcessData(base::ProcessMetrics* metrics, nit: don't need explicit here anymore; also change first argument to scoped_ptr<base::ProcessMetrics> https://codereview.chromium.org/472383002/diff/180001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:38: base::ProcessMetrics* metrics() const { return metrics_.get(); } return a pointer-to-const? https://codereview.chromium.org/472383002/diff/180001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:41: int last_cpu_used() const { return last_cpu_percent_; } rename accessor to last_cpu_percent() to match data member https://codereview.chromium.org/472383002/diff/180001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:43: void SetLastCpuPercent(int new_cpu); this is a simple setter; rename it to set_last_cpu_percent() and inline it https://codereview.chromium.org/472383002/diff/180001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:85: void UpdateMetricsMap(); this method name seems too vague; the next two methods also update the map. SynchronizeProcesses()? https://codereview.chromium.org/472383002/diff/180001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:89: double PopulateCpuUsageByOrigin(); s/Populate/Collect/, maybe? https://codereview.chromium.org/472383002/diff/180001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:91: // Attributes the power usage to the profiles and origins given a total amount mention that this uses the data written to |metrics_map_| by {Populate,Collect}CpuUsageByOrigin()
https://codereview.chromium.org/472383002/diff/180001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/180001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:103: if (it->second->last_cpu_used() < 0) { On 2014/08/20 00:10:42, Daniel Erat wrote: > should you be doing this after you update the last percent instead of before it? > (i admit that this is in conflict with the next comment; perhaps i'm voting for > the previous method and this method to be merged) We remove before we query the last percent to avoid needless queries of processes that no longer exist. I've merged this into what was UpdateMetricsMap() and is now SynchronizeProcesses() to move the adding and removing into the same method. https://codereview.chromium.org/472383002/diff/180001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:110: if (it->second->last_cpu_used() < 0) { On 2014/08/20 00:10:42, Daniel Erat wrote: > hmm, so you skip negative usages here. this seems weird; won't you just erase > the process the next time around? > > is this code all depending on undocumented (i think) behavior where > ProcessMonitor returns -1 if the process has gone away? if so, please find a > safer way to detect this case. Good catch. The intent was to just skip, since the process was still around, but the undocumented removal behavior would kick in during the second loop. I've added a boolean of |seen_this_cycle| to make this process better documented. https://codereview.chromium.org/472383002/diff/180001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.h (right): https://codereview.chromium.org/472383002/diff/180001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:31: explicit PerProcessData(base::ProcessMetrics* metrics, On 2014/08/20 00:10:42, Daniel Erat wrote: > nit: don't need explicit here anymore; also change first argument to > scoped_ptr<base::ProcessMetrics> Done. https://codereview.chromium.org/472383002/diff/180001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:38: base::ProcessMetrics* metrics() const { return metrics_.get(); } On 2014/08/20 00:10:42, Daniel Erat wrote: > return a pointer-to-const? ProcessMetrics::GetCpuUsage() isn't const, so I don't think I can do that. https://codereview.chromium.org/472383002/diff/180001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:41: int last_cpu_used() const { return last_cpu_percent_; } On 2014/08/20 00:10:42, Daniel Erat wrote: > rename accessor to last_cpu_percent() to match data member Done. https://codereview.chromium.org/472383002/diff/180001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:43: void SetLastCpuPercent(int new_cpu); On 2014/08/20 00:10:42, Daniel Erat wrote: > this is a simple setter; rename it to set_last_cpu_percent() and inline it Done. https://codereview.chromium.org/472383002/diff/180001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:85: void UpdateMetricsMap(); On 2014/08/20 00:10:42, Daniel Erat wrote: > this method name seems too vague; the next two methods also update the map. > SynchronizeProcesses()? Seems like a good name. Done. https://codereview.chromium.org/472383002/diff/180001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:89: double PopulateCpuUsageByOrigin(); On 2014/08/20 00:10:42, Daniel Erat wrote: > s/Populate/Collect/, maybe? Done. https://codereview.chromium.org/472383002/diff/180001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:91: // Attributes the power usage to the profiles and origins given a total amount On 2014/08/20 00:10:42, Daniel Erat wrote: > mention that this uses the data written to |metrics_map_| by > {Populate,Collect}CpuUsageByOrigin() Done.
https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:37: metrics_ = metrics.Pass(); nit: can you just do: metrics_(metrics.Pass()) in the initialization list? https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:61: RecordCpuUsageByOrigin(SynchronizeProcesses()); nit: break this into two lines to make it clearer what's being passed: const double total_cpu_percent = SynchronizeProcesses(); RecordCpuUsageByOrigin(total_cpu_percent); https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:93: // Remoev invalid processes and sum up the cpu cycle. nit: Remove https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:94: double cpu_cycle = 0.0; nit: rename to |total_cpu_percent|? https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:102: it->second->set_last_cpu_percent(it->second->metrics()->GetCPUUsage()); could you also do this in AddProcessToMap()? maybe it could be renamed to something like AddOrUpdateProcess() or UpdateProcessInMap()? https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:103: it->second->set_seen_this_cycle(true); do you actually need this call? AddProcessToMap() already does this, right? https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:118: void ProcessPowerCollector::RecordCpuUsageByOrigin(double cpu_cycle) { nit: rename |cpu_cycle| to |total_cpu_percent|? (ditto for header) https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:140: if (sample.battery_percent > -1 && cpu_cycle > 0) nit: use curly brackets here since the statement portion doesn't fit on a single line https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.h (right): https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:50: Profile* profile_; nit: mind adding a blank line above all of these comments to make them a bit easier to read? https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:99: void UpdatePowerConsumption(); nit: think this is gone now
https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:37: metrics_ = metrics.Pass(); On 2014/08/20 17:47:58, Daniel Erat wrote: > nit: can you just do: > > metrics_(metrics.Pass()) > > in the initialization list? For some reason I was under the impression you couldn't, but you can. Done. https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:61: RecordCpuUsageByOrigin(SynchronizeProcesses()); On 2014/08/20 17:47:58, Daniel Erat wrote: > nit: break this into two lines to make it clearer what's being passed: > > const double total_cpu_percent = SynchronizeProcesses(); > RecordCpuUsageByOrigin(total_cpu_percent); Done. https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:93: // Remoev invalid processes and sum up the cpu cycle. On 2014/08/20 17:47:58, Daniel Erat wrote: > nit: Remove Done. https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:94: double cpu_cycle = 0.0; On 2014/08/20 17:47:59, Daniel Erat wrote: > nit: rename to |total_cpu_percent|? Done. https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:102: it->second->set_last_cpu_percent(it->second->metrics()->GetCPUUsage()); On 2014/08/20 17:47:59, Daniel Erat wrote: > could you also do this in AddProcessToMap()? maybe it could be renamed to > something like AddOrUpdateProcess() or UpdateProcessInMap()? Added and changed to UpdateProcessInMap() https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:103: it->second->set_seen_this_cycle(true); On 2014/08/20 17:47:58, Daniel Erat wrote: > do you actually need this call? AddProcessToMap() already does this, right? Correct. Removed. https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:118: void ProcessPowerCollector::RecordCpuUsageByOrigin(double cpu_cycle) { On 2014/08/20 17:47:58, Daniel Erat wrote: > nit: rename |cpu_cycle| to |total_cpu_percent|? > > (ditto for header) Done. https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:140: if (sample.battery_percent > -1 && cpu_cycle > 0) On 2014/08/20 17:47:59, Daniel Erat wrote: > nit: use curly brackets here since the statement portion doesn't fit on a single > line Done. https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.h (right): https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:50: Profile* profile_; On 2014/08/20 17:47:59, Daniel Erat wrote: > nit: mind adding a blank line above all of these comments to make them a bit > easier to read? I don't mind at all. https://codereview.chromium.org/472383002/diff/280001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:99: void UpdatePowerConsumption(); On 2014/08/20 17:47:59, Daniel Erat wrote: > nit: think this is gone now Done.
cool, onward to the tests. steven and james, i'm cc-ing you for some questions in the _unittest.cc file. https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:104: if (it->second->last_cpu_percent() < 0) { nit: move this into UpdateProcessInMap() too https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:106: continue; this will be addressed by the previous comment, but this is a bit weird since the iterator doesn't get incremented (although i think you're saved from an infinite loop since the percent gets set to 0) https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:161: if (metrics_map_.find(handle) == metrics_map_.end()) { nit: to avoid the repeated lookups below, do something like this instead: ProcessMetricsMap::iterator it = metrics_map_.find(handle); if (it == metrics_map_.end()) { // maybe break the linked_ptr construction into its own statement // to make this easier to read it = metrics_map_.insert(std::make_pair(handle, linked_ptr... ).first; } it->second->set_last_cpu_percent(...); it->second->set_seen_this_cycle(true); https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.h (right): https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:34: // Accessors nit: delete unnecessary comment https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:36: nit: delete blank line https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:102: // for a given origin. nit: add to comment: "Called by SynchronizeProcesses()." https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector_unittest.cc (right): https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:44: chromeos::DBusThreadManager::Shutdown(); this is weird; adding stevenjb@ for advice. shouldn't we already be using a fake here? https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:140: // Manually update the map to make the CPU usage work. please add a way to fake the cpu usage so that the tests can exercise the real code by calling the timer callback. for example, you could add an optional callback to the collector that UpdateProcessInMap() calls to get the cpu usage. if the callback is unset it just gets it from the process metrics like usual. these tests can supply a callback that always returns 5, or returns different percentages for different processes, or whatever. https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:167: rph->SetProcessHandle(MakeProcessHandle(1).Pass()); nit: condense these to e.g. process(browser())->SetProcessHandle(MakeProcessHandle(1).Pass()); ? https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:177: EXPECT_EQ(size_t(3), metrics_map->size()); nit: static_cast<size_t>(3) or just do 3U https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:307: const char kTestAppId[] = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; nit: move this inside of the test that uses it https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:313: // Install an app (an extension*). this should be indented two spaces https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:315: base::FilePath extension_path(FILE_PATH_LITERAL("c:\\foo")); probably there's some other way of creating a fake extension for a test that doesn't require constructing a bogus path in a non-portable way, right? cc-ing jamescook@ as he almost certainly knows offhand what that is. https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:332: GURL url("chrome-extension://aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); use the above constant here https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:334: // dependencies. i don't fully understand this comment (partially the "hopefully", either it does or it doesn't). do you mean you're using chrome-extension to avoid loading the app in some other way? if so, hardcoding "chrome-extension://" here doesn't seem great to me since it just hides the dependency within a string instead of making it explicit.
https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:104: if (it->second->last_cpu_percent() < 0) { On 2014/08/20 19:55:07, Daniel Erat wrote: > nit: move this into UpdateProcessInMap() too Done. https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.h (right): https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:34: // Accessors On 2014/08/20 19:55:08, Daniel Erat wrote: > nit: delete unnecessary comment Done. https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:36: On 2014/08/20 19:55:07, Daniel Erat wrote: > nit: delete blank line Done. https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:102: // for a given origin. On 2014/08/20 19:55:07, Daniel Erat wrote: > nit: add to comment: "Called by SynchronizeProcesses()." Done. https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector_unittest.cc (right): https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:140: // Manually update the map to make the CPU usage work. On 2014/08/20 19:55:08, Daniel Erat wrote: > please add a way to fake the cpu usage so that the tests can exercise the real > code by calling the timer callback. for example, you could add an optional > callback to the collector that UpdateProcessInMap() calls to get the cpu usage. > if the callback is unset it just gets it from the process metrics like usual. > these tests can supply a callback that always returns 5, or returns different > percentages for different processes, or whatever. Added a way to set a cpu usage getting callback for testing. https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:167: rph->SetProcessHandle(MakeProcessHandle(1).Pass()); On 2014/08/20 19:55:08, Daniel Erat wrote: > nit: condense these to e.g. > > process(browser())->SetProcessHandle(MakeProcessHandle(1).Pass()); > > ? Done. https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:177: EXPECT_EQ(size_t(3), metrics_map->size()); On 2014/08/20 19:55:08, Daniel Erat wrote: > nit: static_cast<size_t>(3) or just do 3U Done. https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:307: const char kTestAppId[] = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; On 2014/08/20 19:55:08, Daniel Erat wrote: > nit: move this inside of the test that uses it Done. https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:313: // Install an app (an extension*). On 2014/08/20 19:55:08, Daniel Erat wrote: > this should be indented two spaces Done. https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:332: GURL url("chrome-extension://aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); On 2014/08/20 19:55:08, Daniel Erat wrote: > use the above constant here I think the standard way to do this is with hardcoded addresses, rather than constants. Not 100% sure, though. [0] https://code.google.com/p/chromium/codesearch#search/&q=%5C%22chrome-extensio... https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:334: // dependencies. On 2014/08/20 19:55:08, Daniel Erat wrote: > i don't fully understand this comment (partially the "hopefully", either it does > or it doesn't). do you mean you're using chrome-extension to avoid loading the > app in some other way? if so, hardcoding "chrome-extension://" here doesn't seem > great to me since it just hides the dependency within a string instead of making > it explicit. Oh, whoops. That comment was one I left as a note to myself that was supposed to get removed.
https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector_unittest.cc (right): https://codereview.chromium.org/472383002/diff/320001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:334: // dependencies. On 2014/08/20 20:56:35, Daniel Nishi wrote: > On 2014/08/20 19:55:08, Daniel Erat wrote: > > i don't fully understand this comment (partially the "hopefully", either it > does > > or it doesn't). do you mean you're using chrome-extension to avoid loading the > > app in some other way? if so, hardcoding "chrome-extension://" here doesn't > seem > > great to me since it just hides the dependency within a string instead of > making > > it explicit. > > Oh, whoops. That comment was one I left as a note to myself that was supposed to > get removed. > FWIW, it was a comment left from when this was originally a browser test. I was trying to avoid depending on the browser by making an AppWindow here.
https://codereview.chromium.org/472383002/diff/360001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/360001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:132: if (sample.battery_percent > -1 && total_cpu_percent > 0) { nit: sample.battery_percent > 0? and can't you pull this condition outside of the loop, and skip it on all platforms if total_cpu_percent <= 0? on chrome os, why would sample.external_power be false while the battery is reporting a bogus value? https://codereview.chromium.org/472383002/diff/360001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:133: last_process_power_usage = last_process_power_usage * nit: last_process_power_usage *= sample.battery_discharge_rate / total_cpu_percent; https://codereview.chromium.org/472383002/diff/360001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:187: double ProcessPowerCollector::UpdatePowerConsumptionForTesting() { instead of duplicating UpdatePowerConsumptionCallback here, rename this to e.g. UpdatePowerConsumptionInternal and make both UpdatePowerConsumptionCallback and UpdatePowerConsumptionForTesting call it. the callback could probably just be named something like HandleUpdateTimeout, too. i'd also recommend documenting how these fits together in the header, e.g. public: // Calls UpdatePowerConsumption() and returns the total CPU percent. double UpdatePowerConsumptionForTesting(...); private: // Helper method for UpdateTimeout() and // UpdatePowerConsumptionForTesting(). Does etc. and returns the total // CPU percent. double UpdatePowerConsumption(); // Calls UpdatePowerConsumption(). Invoked by |timer_|. void HandleUpdateTimeout(); https://codereview.chromium.org/472383002/diff/360001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.h (right): https://codereview.chromium.org/472383002/diff/360001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:78: void SetGetCpuUsageCallbackForTesting(const CpuUsageCallback& cpu_callback); nit: inline this and name it set_cpu_usage_callback_for_testing() https://codereview.chromium.org/472383002/diff/360001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:84: FRIEND_TEST_ALL_PREFIXES(BrowserProcessPowerTest, NoSite); do you need these still, or can the tests do everything that they need to through the public interface now? (i find that using the public interface is generally better since it often reduces the amount of testing code that needs to be updated whenever the interface changes.) https://codereview.chromium.org/472383002/diff/360001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector_unittest.cc (right): https://codereview.chromium.org/472383002/diff/360001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:47: fake_dbus_thread_manager->SetFakeClients(); while we're waiting for steven to reply: what happens here if you don't do all of this? i'm still wondering if this all would be much simpler if the collector just observed PowerManagerClient instead; then the test could just call the collector's PowerChanged() method directly. there's no concern about overhead from doing this; powerd only sends updates every 5 or 30 seconds. https://codereview.chromium.org/472383002/diff/360001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:72: int ReturnCpuAs5(base::ProcessHandle handle) { return 5; } probably cleaner to pass the desired CPU usage into this method and bind 5 to it within the tests https://codereview.chromium.org/472383002/diff/360001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:179: EXPECT_DOUBLE_EQ(15, collector.UpdatePowerConsumptionForTesting()); i think i'm still missing some understanding of how this class will be used. so you have three processes that are each using 5% of a CPU, and then the origin map reports 33% for each origin since they're each using a third of the total CPU consumption. this doesn't seem like it's a useful number after several iterations, since the total CPU consumption will vary from cycle to cycle. is there a document somewhere that describes how this data will be visualized? i didn't see anything when glancing through the doc that's linked off of the bug. https://codereview.chromium.org/472383002/diff/360001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:180: EXPECT_EQ(33, origin_power_map->GetPowerForOrigin(url1)); side question that i should've asked earlier: what's the reason for this map returning ints instead of doubles?
https://codereview.chromium.org/472383002/diff/360001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/360001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:132: if (sample.battery_percent > -1 && total_cpu_percent > 0) { On 2014/08/20 21:53:14, Daniel Erat wrote: > nit: sample.battery_percent > 0? > > and can't you pull this condition outside of the loop, and skip it on all > platforms if total_cpu_percent <= 0? > > on chrome os, why would sample.external_power be false while the battery is > reporting a bogus value? I tested on a Chromebox a while back and Chromeboxes report a -1% battery with a discharge rate of 0. I neglected to test if it counts as on external power, but that's probably a safe assumption, so I'll knock out that part of the check. https://codereview.chromium.org/472383002/diff/360001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:133: last_process_power_usage = last_process_power_usage * On 2014/08/20 21:53:14, Daniel Erat wrote: > nit: > > last_process_power_usage *= sample.battery_discharge_rate / > total_cpu_percent; Done. https://codereview.chromium.org/472383002/diff/360001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:187: double ProcessPowerCollector::UpdatePowerConsumptionForTesting() { On 2014/08/20 21:53:14, Daniel Erat wrote: > instead of duplicating UpdatePowerConsumptionCallback here, rename this to e.g. > UpdatePowerConsumptionInternal and make both UpdatePowerConsumptionCallback and > UpdatePowerConsumptionForTesting call it. the callback could probably just be > named > something like HandleUpdateTimeout, too. > > i'd also recommend documenting how these fits together in the header, e.g. > > public: > // Calls UpdatePowerConsumption() and returns the total CPU percent. > double UpdatePowerConsumptionForTesting(...); > > private: > // Helper method for UpdateTimeout() and > // UpdatePowerConsumptionForTesting(). Does etc. and returns the total > // CPU percent. > double UpdatePowerConsumption(); > > // Calls UpdatePowerConsumption(). Invoked by |timer_|. > void HandleUpdateTimeout(); Implemented similar to this. https://codereview.chromium.org/472383002/diff/360001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.h (right): https://codereview.chromium.org/472383002/diff/360001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:78: void SetGetCpuUsageCallbackForTesting(const CpuUsageCallback& cpu_callback); On 2014/08/20 21:53:14, Daniel Erat wrote: > nit: inline this and name it set_cpu_usage_callback_for_testing() Done. https://codereview.chromium.org/472383002/diff/360001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:84: FRIEND_TEST_ALL_PREFIXES(BrowserProcessPowerTest, NoSite); On 2014/08/20 21:53:14, Daniel Erat wrote: > do you need these still, or can the tests do everything that they need to > through the public interface now? (i find that using the public interface is > generally better since it often reduces the amount of testing code that needs to > be updated whenever the interface changes.) Whoops. I meant to take those out. Done. https://codereview.chromium.org/472383002/diff/360001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector_unittest.cc (right): https://codereview.chromium.org/472383002/diff/360001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:47: fake_dbus_thread_manager->SetFakeClients(); On 2014/08/20 21:53:15, Daniel Erat wrote: > while we're waiting for steven to reply: what happens here if you don't do all > of this? i'm still wondering if this all would be much simpler if the collector > just observed PowerManagerClient instead; then the test could just call the > collector's PowerChanged() method directly. there's no concern about overhead > from doing this; powerd only sends updates every 5 or 30 seconds. Hm... I didn't know the powerd sent updates that rarely. I've changed over to PowerManagerClient and now just call PowerChanged(). https://codereview.chromium.org/472383002/diff/360001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:72: int ReturnCpuAs5(base::ProcessHandle handle) { return 5; } On 2014/08/20 21:53:15, Daniel Erat wrote: > probably cleaner to pass the desired CPU usage into this method and bind 5 to it > within the tests Done. We curry now. https://codereview.chromium.org/472383002/diff/360001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:180: EXPECT_EQ(33, origin_power_map->GetPowerForOrigin(url1)); On 2014/08/20 21:53:14, Daniel Erat wrote: > side question that i should've asked earlier: what's the reason for this map > returning ints instead of doubles? To answer this question and the above, here is a Google-internal link: https://folio.googleplex.com/chrome-ux/mocks/236-fizz/content-settings/071714... The display is int, rather than double, so I went with that. For CrOS, the value represents the discharge rate of the system in watts multiplied by the ratio of CPU time used in the cycle. In other OSes, it is simply the sum of the %, so 5% would result in a 5 getting added. The return value here is a scaled map where it is represented as a % of the total for the given heuristic.
https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:23: #include "chromeos/dbus/power_manager/power_supply_properties.pb.h" nit: don't need this and power_manager_client.h here; they're already included by the header (although the proto could be forward-declared there instead, i think) https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:75: CHECK(dbus_manager); you don't need this CHECK(); Get() already does it for you https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:82: const power_manager::PowerSupplyProperties& prop) { thanks, listening for this feels much cleaner to me https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:83: should_update = (prop.external_power() == as the person who works on the powerd side of this: i think you should be doing something like this instead: if (prop.battery_state() == etc::DISCHARGING) { should_update_ = true; battery_discharge_rate_ = prop.battery_discharge_rate(); } else { should_Update_ = false; battery_discharge_rate_ = 0.0; } https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:182: if (cpu_usage_callback_.is_null()) { nit: omit curly brackets https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:189: if (process_data->last_cpu_percent() < 0) { nit: omit curly brackets https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.h (right): https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:89: // Can only be called after the DBusThreadManager has been initialized. nit: start this sentence with "On Chrome OS, can only ..." https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:93: void Shutdown(); can you move this into the d'tor so you don't need to worry about someone forgetting to call it? then, add a PowerManagerClient* member and set it in Initialize() so you know whether you need to unregister in the d'tor or not https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:97: void set_cpu_usage_callback_for_testing(const CpuUsageCallback& callback) { nit: move this after the d'tor -- getters and setters usually appear there in chrome https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:137: bool should_update; nit: should_update_ (with trailing underscore) you could perhaps also move this outside of the ifdef and just leave it set to true on other platforms. is the eventual plan to only collect data when on battery power on all platforms? if so, a name like |on_battery_| may be clearer. https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector_unittest.cc (right): https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:42: power_manager::PowerSupplyProperties prop1; nit: s/prop1/prop/
https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:23: #include "chromeos/dbus/power_manager/power_supply_properties.pb.h" On 2014/08/21 23:51:20, Daniel Erat wrote: > nit: don't need this and power_manager_client.h here; they're already included > by the header (although the proto could be forward-declared there instead, i > think) Done. https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:75: CHECK(dbus_manager); On 2014/08/21 23:51:20, Daniel Erat wrote: > you don't need this CHECK(); Get() already does it for you Done. https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:82: const power_manager::PowerSupplyProperties& prop) { On 2014/08/21 23:51:20, Daniel Erat wrote: > thanks, listening for this feels much cleaner to me Done. https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:83: should_update = (prop.external_power() == On 2014/08/21 23:51:21, Daniel Erat wrote: > as the person who works on the powerd side of this: i think you should be doing > something like this instead: > > if (prop.battery_state() == etc::DISCHARGING) { > should_update_ = true; > battery_discharge_rate_ = prop.battery_discharge_rate(); > } else { > should_Update_ = false; > battery_discharge_rate_ = 0.0; > } Done. https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:182: if (cpu_usage_callback_.is_null()) { On 2014/08/21 23:51:20, Daniel Erat wrote: > nit: omit curly brackets Done. https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:189: if (process_data->last_cpu_percent() < 0) { On 2014/08/21 23:51:21, Daniel Erat wrote: > nit: omit curly brackets Done. https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.h (right): https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:89: // Can only be called after the DBusThreadManager has been initialized. On 2014/08/21 23:51:21, Daniel Erat wrote: > nit: start this sentence with "On Chrome OS, can only ..." Done. https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:93: void Shutdown(); On 2014/08/21 23:51:21, Daniel Erat wrote: > can you move this into the d'tor so you don't need to worry about someone > forgetting to call it? then, add a PowerManagerClient* member and set it in > Initialize() so you know whether you need to unregister in the d'tor or not I tried that, but during the unittests the BrowserWithTestWindowTest::TearDown() occurs before the actual destruction of the object, and that destroys the DBusThreadManager. Would it be preferable to move this into the d'tor, or to more directly control the ProcessPowerCollector's lifetime in the unittest? https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:97: void set_cpu_usage_callback_for_testing(const CpuUsageCallback& callback) { On 2014/08/21 23:51:21, Daniel Erat wrote: > nit: move this after the d'tor -- getters and setters usually appear there in > chrome Done. https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:137: bool should_update; On 2014/08/21 23:51:21, Daniel Erat wrote: > nit: should_update_ (with trailing underscore) > > you could perhaps also move this outside of the ifdef and just leave it set to > true on other platforms. is the eventual plan to only collect data when on > battery power on all platforms? if so, a name like |on_battery_| may be clearer. Nit done. Ultimately, we'd like to be able to capture data, even when we're on battery power. As of right now that's not possible yet, but there's work being done to make that happen later on (at least of the last time I talked with the CrOS kernel team). https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector_unittest.cc (right): https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:42: power_manager::PowerSupplyProperties prop1; On 2014/08/21 23:51:21, Daniel Erat wrote: > nit: s/prop1/prop/ Done.
https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.h (right): https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:93: void Shutdown(); On 2014/08/22 01:06:19, Daniel Nishi wrote: > On 2014/08/21 23:51:21, Daniel Erat wrote: > > can you move this into the d'tor so you don't need to worry about someone > > forgetting to call it? then, add a PowerManagerClient* member and set it in > > Initialize() so you know whether you need to unregister in the d'tor or not > > I tried that, but during the unittests the BrowserWithTestWindowTest::TearDown() > occurs before the actual destruction of the object, and that destroys the > DBusThreadManager. > > Would it be preferable to move this into the d'tor, or to more directly control > the ProcessPowerCollector's lifetime in the unittest? the latter -- store the collector in a scoped_ptr and explicitly destroy it via reset() before calling the base test class's TearDown() method. https://codereview.chromium.org/472383002/diff/440001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.h (right): https://codereview.chromium.org/472383002/diff/440001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:27: namespace power_manager { nit: put this inside an OS_CHROMEOS ifdef
https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.h (right): https://codereview.chromium.org/472383002/diff/380001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:93: void Shutdown(); On 2014/08/22 03:13:49, Daniel Erat wrote: > On 2014/08/22 01:06:19, Daniel Nishi wrote: > > On 2014/08/21 23:51:21, Daniel Erat wrote: > > > can you move this into the d'tor so you don't need to worry about someone > > > forgetting to call it? then, add a PowerManagerClient* member and set it in > > > Initialize() so you know whether you need to unregister in the d'tor or not > > > > I tried that, but during the unittests the > BrowserWithTestWindowTest::TearDown() > > occurs before the actual destruction of the object, and that destroys the > > DBusThreadManager. > > > > Would it be preferable to move this into the d'tor, or to more directly > control > > the ProcessPowerCollector's lifetime in the unittest? > > the latter -- store the collector in a scoped_ptr and explicitly destroy it via > reset() before calling the base test class's TearDown() method. Done. https://codereview.chromium.org/472383002/diff/440001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.h (right): https://codereview.chromium.org/472383002/diff/440001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:27: namespace power_manager { On 2014/08/22 03:13:49, Daniel Erat wrote: > nit: put this inside an OS_CHROMEOS ifdef Done.
https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:59: dbus_manager->GetPowerManagerClient()->RemoveObserver(this); you have an unbalanced Remove call here if Initialize doesn't get called. can you move the AddObserver call to the c'tor? if not: - add a chromeos::PowerManagerClient* power_manager_client_ member - initialize it to NULL in the c'tor - set it to dbus_manager->GetPowerManagerClient() and call AddObserver() in Initialize() - call power_manager_client_->RemoveObserver() in the d'tor if power_manager_client_ is non-NULL https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:135: if (total_cpu_percent <= 0) actually, doing this means that you won't call set_seen_this_cycle(false) for all of the entries. how about moving the set_seen_this_cycle(false) calls to the beginning of SynchronizeProcesses()? https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:182: if (cpu_usage_callback_.is_null()) nit: reorder this, maybe: process_data->set_last_cpu_percent( cpu_usage_callback_.is_null() ? process_data->metrics()->GetCPUUsage() : cpu_usage_callback_.Run(handle)); https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:188: if (process_data->last_cpu_percent() < 0) { nit: omit curly brackets here https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:204: void ProcessPowerCollector::HandleUpdateTimeout() { please reorder the methods in the .cc file to match the order in the header https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.h (right): https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:103: ProcessMetricsMap* metrics_map_for_testing() { return &metrics_map_; } nit: move this up below set_cpu_usage_callback_for_testing() (setters/getters should be grouped together) https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:122: // information from CollectCpuUsageByOrigin() given a total amount nit: s/CollectCpuUsageByOrigin/SynchronizeProcesses/ https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:127: // for a given origin. Called by SynchronizedProcesses(). nit: s/Synchronized/Synchronize/ https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector_unittest.cc (right): https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:69: content::MockRenderProcessHost* process(Browser* browser) { names_like_this() are only used for simple setters and getters; this should probably be GetProcess() instead. also, please move it above the data members within the protected section: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:77: scoped_ptr<base::ProcessHandle> MakeProcessHandle(int process_id) { move this above the data members too https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:92: web_contents_.reset(web_contents); nit: move this to an initialization list https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:93: } nit: blank line after this one https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:96: const GURL& url) OVERRIDE{}; for all of these: add a space between OVERRIDE and {} and no trailing semicolon
https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:59: dbus_manager->GetPowerManagerClient()->RemoveObserver(this); On 2014/08/22 22:41:38, Daniel Erat wrote: > you have an unbalanced Remove call here if Initialize doesn't get called. > > can you move the AddObserver call to the c'tor? > > if not: > - add a chromeos::PowerManagerClient* power_manager_client_ member > - initialize it to NULL in the c'tor > - set it to dbus_manager->GetPowerManagerClient() and call AddObserver() in > Initialize() > - call power_manager_client_->RemoveObserver() in the d'tor if > power_manager_client_ is non-NULL Since we're controlling the life time, we can put it in the c'tor now. Done. https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:135: if (total_cpu_percent <= 0) On 2014/08/22 22:41:38, Daniel Erat wrote: > actually, doing this means that you won't call set_seen_this_cycle(false) for > all of the entries. how about moving the set_seen_this_cycle(false) calls to the > beginning of SynchronizeProcesses()? Done. https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:182: if (cpu_usage_callback_.is_null()) On 2014/08/22 22:41:38, Daniel Erat wrote: > nit: reorder this, maybe: > > process_data->set_last_cpu_percent( > cpu_usage_callback_.is_null() ? > process_data->metrics()->GetCPUUsage() : > cpu_usage_callback_.Run(handle)); Done. https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:188: if (process_data->last_cpu_percent() < 0) { On 2014/08/22 22:41:38, Daniel Erat wrote: > nit: omit curly brackets here Done. https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:204: void ProcessPowerCollector::HandleUpdateTimeout() { On 2014/08/22 22:41:38, Daniel Erat wrote: > please reorder the methods in the .cc file to match the order in the header Done. https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.h (right): https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:103: ProcessMetricsMap* metrics_map_for_testing() { return &metrics_map_; } On 2014/08/22 22:41:38, Daniel Erat wrote: > nit: move this up below set_cpu_usage_callback_for_testing() (setters/getters > should be grouped together) Done. https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:122: // information from CollectCpuUsageByOrigin() given a total amount On 2014/08/22 22:41:38, Daniel Erat wrote: > nit: s/CollectCpuUsageByOrigin/SynchronizeProcesses/ Done. https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:127: // for a given origin. Called by SynchronizedProcesses(). On 2014/08/22 22:41:38, Daniel Erat wrote: > nit: s/Synchronized/Synchronize/ Done. https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector_unittest.cc (right): https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:69: content::MockRenderProcessHost* process(Browser* browser) { On 2014/08/22 22:41:39, Daniel Erat wrote: > names_like_this() are only used for simple setters and getters; this should > probably be GetProcess() instead. also, please move it above the data members > within the protected section: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order Done. https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:77: scoped_ptr<base::ProcessHandle> MakeProcessHandle(int process_id) { On 2014/08/22 22:41:38, Daniel Erat wrote: > move this above the data members too Done. https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:92: web_contents_.reset(web_contents); On 2014/08/22 22:41:38, Daniel Erat wrote: > nit: move this to an initialization list Done. https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:93: } On 2014/08/22 22:41:39, Daniel Erat wrote: > nit: blank line after this one Done. https://codereview.chromium.org/472383002/diff/460001/chrome/browser/power/pr... chrome/browser/power/process_power_collector_unittest.cc:96: const GURL& url) OVERRIDE{}; On 2014/08/22 22:41:39, Daniel Erat wrote: > for all of these: add a space between OVERRIDE and {} and no trailing semicolon Done.
lgtm
sky@: PTAL and stamp?
https://codereview.chromium.org/472383002/diff/540001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/540001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:41: ProcessPowerCollector::PerProcessData::PerProcessData() { Initially all pod types here, eg profile_, last_cpu_percent_ and seen_this_cycle_. https://codereview.chromium.org/472383002/diff/540001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:71: should_update_ = true; Does should_update_ and battery_discharge_rate_ need to be chromeos specific? should_update_ is really something like is_enabled_ and battery_discharge_rate_ is just a scale factor. It would be nice if this function could in turn set those two values and make the two values non-platform specific. That would reduce the number of ifdefs here and make for easier to read code. https://codereview.chromium.org/472383002/diff/540001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:74: should_update_ = false; Is there a reason when this goes false the timer needs to still be running?
https://codereview.chromium.org/472383002/diff/540001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/540001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:41: ProcessPowerCollector::PerProcessData::PerProcessData() { On 2014/08/25 15:59:31, sky wrote: > Initially all pod types here, eg profile_, last_cpu_percent_ and > seen_this_cycle_. Done. https://codereview.chromium.org/472383002/diff/540001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:71: should_update_ = true; On 2014/08/25 15:59:31, sky wrote: > Does should_update_ and battery_discharge_rate_ need to be chromeos specific? > should_update_ is really something like is_enabled_ and battery_discharge_rate_ > is just a scale factor. It would be nice if this function could in turn set > those two values and make the two values non-platform specific. That would > reduce the number of ifdefs here and make for easier to read code. I've changed the |should_update| into changing if the timer is running and generalized the discharge rate to a scale factor. https://codereview.chromium.org/472383002/diff/540001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:74: should_update_ = false; On 2014/08/25 15:59:31, sky wrote: > Is there a reason when this goes false the timer needs to still be running? Changed to stop the timer.
https://codereview.chromium.org/472383002/diff/560001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/560001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:73: Initialize(); Init/Initialize methods are generally one shot functions the creator calls. Having this call Initialize(), especially when the previous line is is_initialized_, is confusing. How about adding a StartTimer that this and Initialize call? I would also be inclined to get rid of is_initialized_ https://codereview.chromium.org/472383002/diff/560001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:143: it++; ++it; https://codereview.chromium.org/472383002/diff/560001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:150: for (ProcessMetricsMap::iterator it = metrics_map_.begin(); nit: It's a bit weird to do this pass in a function called RecordCpuUsageByOrig. Maybe it should move to the call site as well as the early out if <= 0. https://codereview.chromium.org/472383002/diff/560001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:157: if (total_cpu_percent <= 0) I could see the total being 0, but < 0? Isn't that an error? https://codereview.chromium.org/472383002/diff/560001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:194: cpu_usage_callback_.is_null() ? process_data->metrics()->GetCPUUsage() Is GetCPUUsage an expensive call? Does it do IO or anything? https://codereview.chromium.org/472383002/diff/560001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:198: process_data->set_last_cpu_percent(0); nit: std::max(0, ... on 193 would take care of this.
https://codereview.chromium.org/472383002/diff/560001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/560001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:73: Initialize(); On 2014/08/25 19:38:39, sky wrote: > Init/Initialize methods are generally one shot functions the creator calls. > Having this call Initialize(), especially when the previous line is > is_initialized_, is confusing. How about adding a StartTimer that this and > Initialize call? I would also be inclined to get rid of is_initialized_ Now in a StartTimer(). I've changed the |is_initialized_| to a |has_started_| -- basically, I need to guard against the possibility that a power event update in Chrome OS hits that tries to activate the timer before it has been manually activated through StartTimer(). This should stop the timer from being accidentally activated during unit tests and such. https://codereview.chromium.org/472383002/diff/560001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:143: it++; On 2014/08/25 19:38:40, sky wrote: > ++it; Done. https://codereview.chromium.org/472383002/diff/560001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:143: it++; On 2014/08/25 19:38:40, sky wrote: > ++it; Done. https://codereview.chromium.org/472383002/diff/560001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:150: for (ProcessMetricsMap::iterator it = metrics_map_.begin(); On 2014/08/25 19:38:40, sky wrote: > nit: It's a bit weird to do this pass in a function called RecordCpuUsageByOrig. > Maybe it should move to the call site as well as the early out if <= 0. Moved to call site. https://codereview.chromium.org/472383002/diff/560001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:157: if (total_cpu_percent <= 0) On 2014/08/25 19:38:39, sky wrote: > I could see the total being 0, but < 0? Isn't that an error? Changed to DCHECKing on less than, return on ==. https://codereview.chromium.org/472383002/diff/560001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:194: cpu_usage_callback_.is_null() ? process_data->metrics()->GetCPUUsage() On 2014/08/25 19:38:40, sky wrote: > Is GetCPUUsage an expensive call? Does it do IO or anything? It shouldn't be that costly -- in the Linux implementation, it does a lookup on the virtual /proc filesystem to get the CPU time, but that's a virtual filesystem stored in RAM.
https://codereview.chromium.org/472383002/diff/560001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/560001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:4: #include "chrome/browser/power/process_power_collector.h" nit: newline between 3/4. https://codereview.chromium.org/472383002/diff/560001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:73: Initialize(); On 2014/08/25 20:19:24, Daniel Nishi wrote: > On 2014/08/25 19:38:39, sky wrote: > > Init/Initialize methods are generally one shot functions the creator calls. > > Having this call Initialize(), especially when the previous line is > > is_initialized_, is confusing. How about adding a StartTimer that this and > > Initialize call? I would also be inclined to get rid of is_initialized_ > > Now in a StartTimer(). > > I've changed the |is_initialized_| to a |has_started_| -- basically, I need to > guard against the possibility that a power event update in Chrome OS hits that > tries to activate the timer before it has been manually activated through > StartTimer(). This should stop the timer from being accidentally activated > during unit tests and such. This patchset had the Initialize() function. The expectation with an init/initialize function is that it runs costly functions and therefor isn't part of the constructor. The other expectation is that Init is invoked immediately after the constructor. The current patchset does away with Init() and instead expects StartTimer() to be invoked. This is a slight different behavior. In particular StartTimer() doesn't imply the function shouldn't be invoked more than once (it shouldn't) and it doesn't imply that it'll be invoked immediately after the constructor (as Init() does). The code would be clearer if you make StartTimer() private, add back the Init() that invokes StartTimer() and do away with has_started_. where as in the current patchset you've gotten rid of it. https://codereview.chromium.org/472383002/diff/580001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.h (right): https://codereview.chromium.org/472383002/diff/580001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:140: bool has_started_; Add description.
https://codereview.chromium.org/472383002/diff/560001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.cc (right): https://codereview.chromium.org/472383002/diff/560001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:4: #include "chrome/browser/power/process_power_collector.h" On 2014/08/25 20:43:43, sky wrote: > nit: newline between 3/4. Done. https://codereview.chromium.org/472383002/diff/560001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.cc:73: Initialize(); On 2014/08/25 20:43:43, sky wrote: > On 2014/08/25 20:19:24, Daniel Nishi wrote: > > On 2014/08/25 19:38:39, sky wrote: > > > Init/Initialize methods are generally one shot functions the creator calls. > > > Having this call Initialize(), especially when the previous line is > > > is_initialized_, is confusing. How about adding a StartTimer that this and > > > Initialize call? I would also be inclined to get rid of is_initialized_ > > > > Now in a StartTimer(). > > > > I've changed the |is_initialized_| to a |has_started_| -- basically, I need to > > guard against the possibility that a power event update in Chrome OS hits that > > tries to activate the timer before it has been manually activated through > > StartTimer(). This should stop the timer from being accidentally activated > > during unit tests and such. > > This patchset had the Initialize() function. The expectation with an > init/initialize function is that it runs costly functions and therefor isn't > part of the constructor. The other expectation is that Init is invoked > immediately after the constructor. > > The current patchset does away with Init() and instead expects StartTimer() to > be invoked. This is a slight different behavior. In particular StartTimer() > doesn't imply the function shouldn't be invoked more than once (it shouldn't) > and it doesn't imply that it'll be invoked immediately after the constructor (as > Init() does). The code would be clearer if you make StartTimer() private, add > back the Init() that invokes StartTimer() and do away with has_started_. > where as in the current patchset you've gotten rid of it. Initialize() brought back, StartTimer() made private, |has_started_| removed. https://codereview.chromium.org/472383002/diff/580001/chrome/browser/power/pr... File chrome/browser/power/process_power_collector.h (right): https://codereview.chromium.org/472383002/diff/580001/chrome/browser/power/pr... chrome/browser/power/process_power_collector.h:140: bool has_started_; On 2014/08/25 20:43:44, sky wrote: > Add description. |has_started_| removed.
LGTM
The CQ bit was checked by dhnishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dhnishi@chromium.org/472383002/600001
The CQ bit was checked by dhnishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dhnishi@chromium.org/472383002/620001
The CQ bit was unchecked by dhnishi@chromium.org
The CQ bit was checked by dhnishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dhnishi@chromium.org/472383002/640001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by dhnishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dhnishi@chromium.org/472383002/640001
Message was sent while issue was closed.
Committed patchset #17 (640001) as 1a9eda4d8289e13b43efd2b35ee032e016479caa
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/3cf1d2aa622e9513c593fff948b6df149737e8d2 Cr-Commit-Position: refs/heads/master@{#291999} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
