Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23825004:
PerformanceMonitor: UMA alert for high browser CPU usage. (Closed)
|
Created:
7 years, 3 months ago by oystein (OOO til 10th of July) Modified:
7 years, 2 months ago CC:
chromium-reviews, Ilya Sherman, jar (doing other things), asvitkine+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionModified the PerformanceMonitor to not use test/ code for enumerating Chrome processes.
Separated out basic performance metrics gathering in the PerformanceMonitor to run independently from the metrics/events database code. The former will now run even without the performance-monitor-gathering flag.
Added an UMA event which triggers when the browser process never runs below X CPU utilization over a period of Y seconds.
BUG=279660, 273320
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226466
Patch Set 1 #Patch Set 2 : Buildfix #
Total comments: 1
Patch Set 3 : Build fixes, plus moved base UMA alert from out of about://flags switch #Patch Set 4 : Fixed dependencies. #
Total comments: 64
Patch Set 5 : browser_tests buildfix #Patch Set 6 : One more found of test buildfixes #Patch Set 7 : Review fixes #
Total comments: 32
Patch Set 8 : Review fixes: Round two #
Total comments: 32
Patch Set 9 : Third round of review fixes #Patch Set 10 : Minor simplication #Patch Set 11 : Docs #
Total comments: 12
Patch Set 12 : Fourth round of review fixes. #Patch Set 13 : Buildfix #Patch Set 14 : Small review fix #Patch Set 15 : Fix for flaky Windows test #Patch Set 16 : Rebase #Patch Set 17 : Rebase #Messages
Total messages: 32 (0 generated)
This looks really solid to me. I'd start working with the appropriate owners to get this landed https://codereview.chromium.org/23825004/diff/6001/chrome/browser/performance... File chrome/browser/performance_monitor/performance_monitor.h (right): https://codereview.chromium.org/23825004/diff/6001/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.h:82: // Prevents Start() from starting the collection timer. Used for tests. I'm not quite sure how I feel about increasing the API surface area for the purpose of unittests. Is there another way to poke at this without adding test code to the chrome binary itself? But this isn't really up to me. I'd check with chrome/browser/performance_monitor/OWNERS
On 2013/09/10 17:58:50, tonyg wrote: > This looks really solid to me. I'd start working with the appropriate owners to > get this landed > > https://codereview.chromium.org/23825004/diff/6001/chrome/browser/performance... > File chrome/browser/performance_monitor/performance_monitor.h (right): > > https://codereview.chromium.org/23825004/diff/6001/chrome/browser/performance... > chrome/browser/performance_monitor/performance_monitor.h:82: // Prevents Start() > from starting the collection timer. Used for tests. > I'm not quite sure how I feel about increasing the API surface area for the > purpose of unittests. Is there another way to poke at this without adding test > code to the chrome binary itself? > > But this isn't really up to me. I'd check with > chrome/browser/performance_monitor/OWNERS I got rid of the setters which were just used by the unittests and just directly set the members instead; at least it reduces the API.
On 2013/09/11 17:31:11, Oystein wrote: > On 2013/09/10 17:58:50, tonyg wrote: > > This looks really solid to me. I'd start working with the appropriate owners > to > > get this landed > > > > > https://codereview.chromium.org/23825004/diff/6001/chrome/browser/performance... > > File chrome/browser/performance_monitor/performance_monitor.h (right): > > > > > https://codereview.chromium.org/23825004/diff/6001/chrome/browser/performance... > > chrome/browser/performance_monitor/performance_monitor.h:82: // Prevents > Start() > > from starting the collection timer. Used for tests. > > I'm not quite sure how I feel about increasing the API surface area for the > > purpose of unittests. Is there another way to poke at this without adding test > > code to the chrome binary itself? > > > > But this isn't really up to me. I'd check with > > chrome/browser/performance_monitor/OWNERS > > I got rid of the setters which were just used by the unittests and just directly > set the members instead; at least it reduces the API. Thanks, I like that better. The lg2m, now it is up to the owners to review.
Cursory overview. More to come, but my laptop's about to die. I'm thrilled to see PerformanceMonitor being used somewhere! Thanks for your work on this! :) https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... File chrome/browser/performance_monitor/event_type.h (right): https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/event_type.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:108: disable_timer_autostart_(false) {} nit: please place { and } on separate lines, e.g. disable_timer_autostart_(false) { } I know this is different throughout the codebase, but I think in p_m, we do it this way. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:144: ->GetSwitchValueASCII(switches::kPerformanceMonitorGathering) nit: though arbitrary, I've always seen -> on the preceding line. Please conform. :) https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:148: CommandLine::ForCurrentProcess()->GetSwitchValueASCII( nit: operator ! does not count as a separate scope - i.e., indent should be: if (!base::StringToInt( CommandLine::.... https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:164: next_collection_time_ = base::Time::Now() + base::TimeDelta::FromSeconds( nit: To me, this just looks odd (though I understand the reasoning). Prefer: next_collection_time_ = base::Time::Now() + base::TimeDelta::FromSeconds(gather_interval_in_seconds_); or next_collection_time_ = base::Time::Now() + base::TimeDelta::FromSeconds(gather_interval_in_seconds_); https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:199: // Events and notifications are only useful if we're line wrapping is off. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:212: if (!disable_timer_autostart_) { nit: for single-line if-statements, prefer no brackets. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:231: DCHECK(database_logging_enabled_); given that this is a private method called from one place, this seems redundant. Probably safe to remove. :) https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:264: DCHECK(database_logging_enabled_); same as above; probably safe to remove. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:285: DCHECK(database_logging_enabled_); If you want to have a check for database logging, perhaps it makes sense to have a wrapper around database->Add[Metric|Event]()? Most of these checks are extraneous, but if you want to have some extra security, let's put it all in one place. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:366: if (database_logging_enabled_) { nit: no brackets. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:374: void PerformanceMonitor::GatherMetricsMapOnUIThread() { here, a DCHECK (or even CHECK) for On UI Thread would be good. *Really* bad things happen if we're not, and thread checking is generally more prone to mistakes than synchronous function calls. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:381: for (content::RenderProcessHost::iterator i = i is very nondescriptive for a non-standard iterator like this. Prefer something like rph_iter. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:384: i.Advance()) { nit: put this on the previous line. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:415: ProcessMetricsHistory& process_metrics = process_metrics_iter->second; We don't typically use non-const references if we can help it. Prefer a pointer. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:455: // Find all child processes (does not include renderers), nit: line wrapping. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:498: // Time for a collection to be made, of previously sampled metrics. nit: "Make a collection of previously-sampled metrics." Move the "timing may be off" part to the header file (in a comment for next_collection_time_). https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:512: ++iter) { nit: put this on the previous line. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:514: ProcessMetricsHistory& process_metrics = iter->second; nit: avoid non-const references when possible. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:527: database_->AddMetric(Metric(METRIC_CPU_USAGE, time_now, cpu_usage)); This seems bad to me. I don't think we should be doing this much work if we don't have a database to log it. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:558: if (!disable_timer_autostart_) { nit: no brackets https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.h (right): https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:10: nit: no newline https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:72: bool IsDatabaseLoggingEnabled() const { return database_logging_enabled_; } usually we use unix_hacker_style for methods which just return, e.g., a datamember. Perhaps simply: database_logging_enabled() const { return databse_logging_enabled_; } so callers know this doesn't perform any additional functionality? https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:197: // Next time we should collect averages from the performance metrics, nit: proper grammar in full comments - "The next time..."; also, no comma - "... from the process metrics and act on them." https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor_browsertest.cc (right): https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. If we're trying to brush off the dust from PerformanceMonitor, it'd be great to see how many of the miscellaneous disabled/flaky tests we can have pass. Don't kill yourself trying to get them to run, but if the improvements make them work, I'd love to have them back in action. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... File chrome/browser/performance_monitor/process_metrics_history.cc (right): https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/process_metrics_history.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/process_metrics_history.cc:61: if (!process_metrics_->GetMemoryBytes(&private_bytes, &shared_bytes)) { nit: no brackets https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/process_metrics_history.cc:78: if (process_type_ != content::PROCESS_TYPE_BROWSER) { nit: no brackets https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... File chrome/browser/performance_monitor/process_metrics_history.h (right): https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/process_metrics_history.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/process_metrics_history.h:36: void SetLastUpdateSequence(int new_update_sequence) { for functions that don't do (real) work, prefer unix_hacker_style. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/process_metrics_history.h:40: int GetLastUpdateSequence() const { return last_update_sequence_; } unix_hacker_style - "last_updated_sequence()" https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/process_metrics_history.h:45: // // Average mem usage over all the past sampling points since last reset. double comment?
Thanks for the review :). https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... File chrome/browser/performance_monitor/event_type.h (right): https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/event_type.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/09/12 00:44:47, D Cronin wrote: > nit: no (c) Done. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:108: disable_timer_autostart_(false) {} On 2013/09/12 00:44:47, D Cronin wrote: > nit: please place { and } on separate lines, e.g. > disable_timer_autostart_(false) { > } > I know this is different throughout the codebase, but I think in p_m, we do it > this way. Done. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:144: ->GetSwitchValueASCII(switches::kPerformanceMonitorGathering) On 2013/09/12 00:44:47, D Cronin wrote: > nit: though arbitrary, I've always seen -> on the preceding line. Please > conform. :) Done. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:148: CommandLine::ForCurrentProcess()->GetSwitchValueASCII( On 2013/09/12 00:44:47, D Cronin wrote: > nit: operator ! does not count as a separate scope - i.e., indent should be: > if (!base::StringToInt( > CommandLine::.... Done. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:164: next_collection_time_ = base::Time::Now() + base::TimeDelta::FromSeconds( On 2013/09/12 00:44:47, D Cronin wrote: > nit: To me, this just looks odd (though I understand the reasoning). Prefer: > next_collection_time_ = > base::Time::Now() + > base::TimeDelta::FromSeconds(gather_interval_in_seconds_); > or > next_collection_time_ = base::Time::Now() + > base::TimeDelta::FromSeconds(gather_interval_in_seconds_); Done. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:199: // Events and notifications are only useful if we're On 2013/09/12 00:44:47, D Cronin wrote: > line wrapping is off. Done. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:212: if (!disable_timer_autostart_) { On 2013/09/12 00:44:47, D Cronin wrote: > nit: for single-line if-statements, prefer no brackets. Done. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:231: DCHECK(database_logging_enabled_); On 2013/09/12 00:44:47, D Cronin wrote: > given that this is a private method called from one place, this seems redundant. > Probably safe to remove. :) Safe with the current code paths sure, but why remove it? https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:285: DCHECK(database_logging_enabled_); On 2013/09/12 00:44:47, D Cronin wrote: > If you want to have a check for database logging, perhaps it makes sense to have > a wrapper around database->Add[Metric|Event]()? Most of these checks are > extraneous, but if you want to have some extra security, let's put it all in one > place. Reorganizing release code to account for debug-only asserts feels a bit fishy. I'll remove these if you want, but my general principle with regards to debug-only asserts has always been "more is better". https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:366: if (database_logging_enabled_) { On 2013/09/12 00:44:47, D Cronin wrote: > nit: no brackets. Sure, though out of curiosity: Is this an actual Chrome rule? The only guideline I could find was "do whatever you prefer" (and I definitely prefer braces for safety). https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:374: void PerformanceMonitor::GatherMetricsMapOnUIThread() { On 2013/09/12 00:44:47, D Cronin wrote: > here, a DCHECK (or even CHECK) for On UI Thread would be good. *Really* bad > things happen if we're not, and thread checking is generally more prone to > mistakes than synchronous function calls. Done. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:381: for (content::RenderProcessHost::iterator i = On 2013/09/12 00:44:47, D Cronin wrote: > i is very nondescriptive for a non-standard iterator like this. Prefer something > like rph_iter. Done. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:384: i.Advance()) { On 2013/09/12 00:44:47, D Cronin wrote: > nit: put this on the previous line. Done. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:415: ProcessMetricsHistory& process_metrics = process_metrics_iter->second; On 2013/09/12 00:44:47, D Cronin wrote: > We don't typically use non-const references if we can help it. Prefer a pointer. Demoting the reference to a pointer loses important type information, and adds ambiguity. The rules regarding non-const references apply to function arguments, not local scoped variables as far as I can see. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:455: // Find all child processes (does not include renderers), On 2013/09/12 00:44:47, D Cronin wrote: > nit: line wrapping. Not sure what you mean here :). The comment can't fit on one line. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:498: // Time for a collection to be made, of previously sampled metrics. On 2013/09/12 00:44:47, D Cronin wrote: > nit: "Make a collection of previously-sampled metrics." Move the "timing may be > off" part to the header file (in a comment for next_collection_time_). Why the move? That seems to take information away from the only point where it's relevant. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:527: database_->AddMetric(Metric(METRIC_CPU_USAGE, time_now, cpu_usage)); On 2013/09/12 00:44:47, D Cronin wrote: > This seems bad to me. I don't think we should be doing this much work if we > don't have a database to log it. EndOfCycle() always needs to be called. I can call it in a separate loop, away from the metrics summing, but that will actually be less efficient (likely by an order of magnitude) than the above approach due to the cache misses you incur from iterating through an std::map. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:558: if (!disable_timer_autostart_) { On 2013/09/12 00:44:47, D Cronin wrote: > nit: no brackets Done. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.h (right): https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:10: On 2013/09/12 00:44:47, D Cronin wrote: > nit: no newline Done. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:72: bool IsDatabaseLoggingEnabled() const { return database_logging_enabled_; } On 2013/09/12 00:44:47, D Cronin wrote: > usually we use unix_hacker_style for methods which just return, e.g., a > datamember. Perhaps simply: > database_logging_enabled() const { return databse_logging_enabled_; } > so callers know this doesn't perform any additional functionality? Done. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:197: // Next time we should collect averages from the performance metrics, On 2013/09/12 00:44:47, D Cronin wrote: > nit: proper grammar in full comments - "The next time..."; also, no comma - "... > from the process metrics and act on them." Done. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... File chrome/browser/performance_monitor/process_metrics_history.cc (right): https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/process_metrics_history.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/09/12 00:44:47, D Cronin wrote: > no (c) Done. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/process_metrics_history.cc:61: if (!process_metrics_->GetMemoryBytes(&private_bytes, &shared_bytes)) { On 2013/09/12 00:44:47, D Cronin wrote: > nit: no brackets Done. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/process_metrics_history.cc:78: if (process_type_ != content::PROCESS_TYPE_BROWSER) { On 2013/09/12 00:44:47, D Cronin wrote: > nit: no brackets Done. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... File chrome/browser/performance_monitor/process_metrics_history.h (right): https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/process_metrics_history.h:36: void SetLastUpdateSequence(int new_update_sequence) { On 2013/09/12 00:44:47, D Cronin wrote: > for functions that don't do (real) work, prefer unix_hacker_style. Done. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/process_metrics_history.h:40: int GetLastUpdateSequence() const { return last_update_sequence_; } On 2013/09/12 00:44:47, D Cronin wrote: > unix_hacker_style - "last_updated_sequence()" Done. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/process_metrics_history.h:45: // // Average mem usage over all the past sampling points since last reset. On 2013/09/12 00:44:47, D Cronin wrote: > double comment? Done.
Round 2 :) https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:415: ProcessMetricsHistory& process_metrics = process_metrics_iter->second; On 2013/09/12 15:02:31, Oystein wrote: > On 2013/09/12 00:44:47, D Cronin wrote: > > We don't typically use non-const references if we can help it. Prefer a > pointer. > > Demoting the reference to a pointer loses important type information, and adds > ambiguity. The rules regarding non-const references apply to function arguments, > not local scoped variables as far as I can see. I don't feel real strongly about it, but I'm not sure I see why demoting it to a pointer loses any type information. Isn't this the same as: ProcessMetricsHistory* process_metrics = &process_metrics_iter->second; process_metrics->SetLastUpdateSequence(current_update_sequence); ? https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:455: // Find all child processes (does not include renderers), On 2013/09/12 15:02:31, Oystein wrote: > On 2013/09/12 00:44:47, D Cronin wrote: > > nit: line wrapping. > > Not sure what you mean here :). The comment can't fit on one line. If a comment doesn't fit on one line, it should line-wrap when it reaches the end of the line, e.g. prefer: xxxxxx xx over xxxx xxxx Here: // Find all child processes (does not include renderers), which has to be done // on the IO thread. https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:498: // Time for a collection to be made, of previously sampled metrics. On 2013/09/12 15:02:31, Oystein wrote: > On 2013/09/12 00:44:47, D Cronin wrote: > > nit: "Make a collection of previously-sampled metrics." Move the "timing may > be > > off" part to the header file (in a comment for next_collection_time_). > > Why the move? That seems to take information away from the only point where it's > relevant. Again, no strong feelings, so if you think it makes more sense here that's fine. But I think it's relevant enough to the variable (next_collection_time_) to include in the header. It lets me know that |next_collection_time_| is not a hard guarantee. https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:138: DCHECK(!has_initialized); this should probably be: if (has_initialized) { NOTREACHED(); return; } To gracefully handle anything weird in release. https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:143: database_logging_enabled_ = true; This can potentially introduce race conditions. It should only be set once the database is actually ready. https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:145: if (!CommandLine::ForCurrentProcess()-> nit: operator ! doesn't count for indentation level. Should be if (!CommandLine::ForCurrentProcess()->HasSwitch( switches::kPerformanceMonitorGathering)) { https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:149: if (!base::StringToInt( nit: indendation. https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:284: no newline https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:375: static int current_update_sequence = 0; Prefer a member variable over static int for something like this. https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:384: no newline https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:484: while (iter != metrics_map_.end()) { Don't need two separate loops for this and determining metrics. They can easily be combined. https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:511: no newline https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... File chrome/browser/performance_monitor/performance_monitor.h (right): https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.h:159: void ProcessIsAlive(const base::ProcessHandle& handle, function comment needed https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... File chrome/browser/performance_monitor/process_metrics_history.h (right): https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/process_metrics_history.h:43: double GetAverageCPUUsage() { return accumulated_cpu_usage_ / sample_count_; } const https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/process_metrics_history.h:45: // // Average mem usage. 1. you have two comment starts here - "// //" should be "//" 2. Please use full sentences/proper grammar (your comment for GetAverageCPUUsage() is a perfect example) :) https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/process_metrics_history.h:46: void GetAverageMemoryBytes(size_t* private_bytes, size_t* shared_bytes) { const
https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:415: ProcessMetricsHistory& process_metrics = process_metrics_iter->second; On 2013/09/12 18:20:42, D Cronin wrote: > On 2013/09/12 15:02:31, Oystein wrote: > > On 2013/09/12 00:44:47, D Cronin wrote: > > > We don't typically use non-const references if we can help it. Prefer a > > pointer. > > > > Demoting the reference to a pointer loses important type information, and adds > > ambiguity. The rules regarding non-const references apply to function > arguments, > > not local scoped variables as far as I can see. > > I don't feel real strongly about it, but I'm not sure I see why demoting it to a > pointer loses any type information. Isn't this the same as: > ProcessMetricsHistory* process_metrics = &process_metrics_iter->second; > process_metrics->SetLastUpdateSequence(current_update_sequence); > ? What's lost is the constraint that the variable can't be NULL, basically. What would you gain from demoting this to a pointer? https://codereview.chromium.org/23825004/diff/19001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:455: // Find all child processes (does not include renderers), On 2013/09/12 18:20:42, D Cronin wrote: > On 2013/09/12 15:02:31, Oystein wrote: > > On 2013/09/12 00:44:47, D Cronin wrote: > > > nit: line wrapping. > > > > Not sure what you mean here :). The comment can't fit on one line. > > If a comment doesn't fit on one line, it should line-wrap when it reaches the > end of the line, e.g. prefer: > xxxxxx > xx > over > xxxx > xxxx > > Here: > // Find all child processes (does not include renderers), which has to be done > // on the IO thread. > Done. https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:138: DCHECK(!has_initialized); On 2013/09/12 18:20:42, D Cronin wrote: > this should probably be: > if (has_initialized) { > NOTREACHED(); > return; > } > > To gracefully handle anything weird in release. Done. https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:143: database_logging_enabled_ = true; On 2013/09/12 18:20:42, D Cronin wrote: > This can potentially introduce race conditions. It should only be set once the > database is actually ready. That's not the meaning of the flag, it's more of the database logging *system* is enabled rather than the database itself being ready for logging. There could only be race conditions here if they were already present before this CL, as far as I can tell. https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:145: if (!CommandLine::ForCurrentProcess()-> On 2013/09/12 18:20:42, D Cronin wrote: > nit: operator ! doesn't count for indentation level. Should be > if (!CommandLine::ForCurrentProcess()->HasSwitch( > switches::kPerformanceMonitorGathering)) { Done. https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:149: if (!base::StringToInt( On 2013/09/12 18:20:42, D Cronin wrote: > nit: indendation. Done. https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:284: On 2013/09/12 18:20:42, D Cronin wrote: > no newline Done. https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:375: static int current_update_sequence = 0; On 2013/09/12 18:20:42, D Cronin wrote: > Prefer a member variable over static int for something like this. The idea is that the sequence ID should be carried along in the chain of jobs as an argument rather than being encoded in object state, as an added safeguard in case of concurrent runs (less shared state between threads). Moving this to a member var increases the likelihood of the member being used by the background/IO threads rather than the argument. https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:384: On 2013/09/12 18:20:42, D Cronin wrote: > no newline Done. https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:484: while (iter != metrics_map_.end()) { On 2013/09/12 18:20:42, D Cronin wrote: > Don't need two separate loops for this and determining metrics. They can easily > be combined. This loop is run through on every iteration; the below loop isn't. You don't want to make the common case more expensive (by adding a time comparison in the inner loop) to optimize away the rare case (the below loop). https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:511: On 2013/09/12 18:20:42, D Cronin wrote: > no newline Done. https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... File chrome/browser/performance_monitor/performance_monitor.h (right): https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.h:159: void ProcessIsAlive(const base::ProcessHandle& handle, On 2013/09/12 18:20:42, D Cronin wrote: > function comment needed Done. https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... File chrome/browser/performance_monitor/process_metrics_history.h (right): https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/process_metrics_history.h:43: double GetAverageCPUUsage() { return accumulated_cpu_usage_ / sample_count_; } On 2013/09/12 18:20:42, D Cronin wrote: > const Done. https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/process_metrics_history.h:45: // // Average mem usage. On 2013/09/12 18:20:42, D Cronin wrote: > 1. you have two comment starts here - "// //" should be "//" > 2. Please use full sentences/proper grammar (your comment for > GetAverageCPUUsage() is a perfect example) :) Done. https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/process_metrics_history.h:46: void GetAverageMemoryBytes(size_t* private_bytes, size_t* shared_bytes) { On 2013/09/12 18:20:42, D Cronin wrote: > const Done.
https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:375: static int current_update_sequence = 0; On 2013/09/12 19:03:24, Oystein wrote: > On 2013/09/12 18:20:42, D Cronin wrote: > > Prefer a member variable over static int for something like this. > > The idea is that the sequence ID should be carried along in the chain of jobs as > an argument rather than being encoded in object state, as an added safeguard in > case of concurrent runs (less shared state between threads). Moving this to a > member var increases the likelihood of the member being used by the > background/IO threads rather than the argument. Is this really a concern, though? GatherMetricsMapOnIOThread() is only called from GatherMetricsMapOnUIThread(), and we don't reset the timer until that task is completed, right? So aren't we guaranteed that only one of these will be running at a time (either GatherMetricsOnUI or GatherMetricsOnIO)? If not, you have a much bigger problem than a boolean variable - metrics_map_ is referenced in both. :) https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:484: while (iter != metrics_map_.end()) { On 2013/09/12 19:03:24, Oystein wrote: > On 2013/09/12 18:20:42, D Cronin wrote: > > Don't need two separate loops for this and determining metrics. They can > easily > > be combined. > > This loop is run through on every iteration; the below loop isn't. You don't > want to make the common case more expensive (by adding a time comparison in the > inner loop) to optimize away the rare case (the below loop). base::Time time_now = base::Time::Now(); bool gather_metrics = time_now > next_collection_time_; double cpu_usage = 0.0; size_t private_memory_sum = 0; size_t shared_memory_sum = 0; for (MetricsMap::iterator iter = metrics_map_.begin(); iter != metrics_map_.end(); /* no increment */) { // Update metrics for all watched processes; remove dead entries from // the map. ProcessMetricsHistory* process_metrics = &iter->second; if (process_metrics->last_update_sequence() != current_update_sequence) { // Not touched this iteration; let's get rid of it. metrics_map_.erase(iter++); } else { process_metrics.SampleMetrics(); if (gather_metrics) { cpu_usage += process_metrics.GetAverageCPUUsage(); size_t private_memory = 0; size_t shared_memory = 0; process_metrics.GetAverageMemoryBytes(&private_memory, &shared_memory); private_memory_sum += private_memory; shared_memory_sum += shared_memory; process_metrics->EndOfCycle(); } } } if (gather_metrics && database_logging_enabled_) { database_->AddMetric(Metric(METRIC_CPU_USAGE, time_now, cpu_usage)); database_->AddMetric(Metric(METRIC_PRIVATE_MEMORY_USAGE, time_now, static_cast<double>(private_memory_sum))); database_->AddMetric(Metric(METRIC_SHARED_MEMORY_USAGE, time_now, static_cast<double>(shared_memory_sum))); database_->AddMetric( Metric(METRIC_NETWORK_BYTES_READ, time_now, static_cast<double>( performance_data_for_io_thread.network_bytes_read))); } The increased complexity in the loop is equivalent to checking a boolean expression (or practically 0) - far less than the cost of having a second loop, even if it's only entered on occasion (not to mention the code simplification). https://codereview.chromium.org/23825004/diff/12001/chrome/browser/chrome_bro... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/23825004/diff/12001/chrome/browser/chrome_bro... chrome/browser/chrome_browser_main.cc:1624: performance_monitor::PerformanceMonitor::GetInstance()->Start(); (One of) the reason(s) we never turned this on by default was because we weren't entirely sure how much of a performance impact it would have (and it would be terrible if we reduced performance significantly while watching for significantly-reduced performance). I just want to make sure that this has been checked, and everyone is okay with the potential overhead. https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:155: &specified_interval) || indenation https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:403: if (handle == 0) { Since we don't care about any type other than content::PROCESS_TYPE_BROWSER (per ProcessMetricsHistory::RunPerformanceTriggers()), do we need to add it to the map at all, if we don't have database_logging_enabled_? i.e., could this instead be: if (handle == 0 || (process_type != content::PROCESS_TYPE_BROWSER && !database_logging_enabled_)) { // If the process is invalid, or we are not interested in the process type, // do not add the process to the map. return; } https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.h (left): https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:12: #include "base/files/file_path.h" still need this one. https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.h (right): https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:23: namespace content { you #include this, and have an instance in PerformanceMonitor. No need to forward declare the class. https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:160: void ProcessIsAlive(const base::ProcessHandle& handle, Please mention in the comment that this will begin tracking the process if we are not already. https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:199: base::Time next_collection_time_; #include "base/time/time.h" https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:208: base::DelayTimer<PerformanceMonitor> timer_; #include "base/timer/timer.h" https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... File chrome/browser/performance_monitor/process_metrics_history.cc (right): https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/process_metrics_history.cc:67: sample_count_++; nit: ++sample_count; https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/process_metrics_history.cc:82: if (min_cpu_usage_ > kHighCPUUtilizationThreshold) { Just FYI: The different OSs report the CPU usage differently. Linux and Mac will report on a per-CPU basis, so having a CPU usage of 90% on these OSs is high, but not terrible (in the initial testing for this, we saw readings into 200% - 400% usage). Windows, though, reports on a [0, 100] range. I'm sure this can be accounted for (just as the different types and powers of CPUs must be accounted for), but wanted to make sure you were aware.
https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:375: static int current_update_sequence = 0; On 2013/09/13 18:14:09, D Cronin wrote: > On 2013/09/12 19:03:24, Oystein wrote: > > On 2013/09/12 18:20:42, D Cronin wrote: > > > Prefer a member variable over static int for something like this. > > > > The idea is that the sequence ID should be carried along in the chain of jobs > as > > an argument rather than being encoded in object state, as an added safeguard > in > > case of concurrent runs (less shared state between threads). Moving this to a > > member var increases the likelihood of the member being used by the > > background/IO threads rather than the argument. > > Is this really a concern, though? GatherMetricsMapOnIOThread() is only called > from GatherMetricsMapOnUIThread(), and we don't reset the timer until that task > is completed, right? So aren't we guaranteed that only one of these will be > running at a time (either GatherMetricsOnUI or GatherMetricsOnIO)? > > If not, you have a much bigger problem than a boolean variable - metrics_map_ is > referenced in both. :) Oh I'll readily admit it's mainly an artifact from an earlier version of the code which was still using RepeatingTimer rather than manually starting a DelayTimer, but as moving this to a member variable gives no real benefit (4 bytes size increase in PerformanceMonitor vs 4 bytes extra per temporary job) and *does* make the code less robust, however slightly, it doesn't seem sensible to change..? https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:484: while (iter != metrics_map_.end()) { On 2013/09/13 18:14:09, D Cronin wrote: > On 2013/09/12 19:03:24, Oystein wrote: > > On 2013/09/12 18:20:42, D Cronin wrote: > > > Don't need two separate loops for this and determining metrics. They can > > easily > > > be combined. > > > > This loop is run through on every iteration; the below loop isn't. You don't > > want to make the common case more expensive (by adding a time comparison in > the > > inner loop) to optimize away the rare case (the below loop). > > base::Time time_now = base::Time::Now(); > bool gather_metrics = time_now > next_collection_time_; > double cpu_usage = 0.0; > size_t private_memory_sum = 0; > size_t shared_memory_sum = 0; > > for (MetricsMap::iterator iter = metrics_map_.begin(); > iter != metrics_map_.end(); /* no increment */) { > // Update metrics for all watched processes; remove dead entries from > // the map. > ProcessMetricsHistory* process_metrics = &iter->second; > if (process_metrics->last_update_sequence() != current_update_sequence) { > // Not touched this iteration; let's get rid of it. > metrics_map_.erase(iter++); > } else { > process_metrics.SampleMetrics(); > if (gather_metrics) { > cpu_usage += process_metrics.GetAverageCPUUsage(); > > size_t private_memory = 0; > size_t shared_memory = 0; > process_metrics.GetAverageMemoryBytes(&private_memory, &shared_memory); > private_memory_sum += private_memory; > shared_memory_sum += shared_memory; > > process_metrics->EndOfCycle(); > } > } > } > > if (gather_metrics && database_logging_enabled_) { > database_->AddMetric(Metric(METRIC_CPU_USAGE, time_now, cpu_usage)); > database_->AddMetric(Metric(METRIC_PRIVATE_MEMORY_USAGE, > time_now, > static_cast<double>(private_memory_sum))); > database_->AddMetric(Metric(METRIC_SHARED_MEMORY_USAGE, > time_now, > static_cast<double>(shared_memory_sum))); > > database_->AddMetric( > Metric(METRIC_NETWORK_BYTES_READ, > time_now, > static_cast<double>( > performance_data_for_io_thread.network_bytes_read))); > } > > The increased complexity in the loop is equivalent to checking a boolean > expression (or practically 0) - far less than the cost of having a second loop, > even if it's only entered on occasion (not to mention the code simplification). Right, I see what you mean now. Note that introducing a branch has a pretty big impact on CPU instruction pipelining and is in no way "practically 0", but in this case it's dwarfed by the std::map iteration anyway so it's fine. https://codereview.chromium.org/23825004/diff/12001/chrome/browser/chrome_bro... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/23825004/diff/12001/chrome/browser/chrome_bro... chrome/browser/chrome_browser_main.cc:1624: performance_monitor::PerformanceMonitor::GetInstance()->Start(); On 2013/09/13 18:14:09, D Cronin wrote: > (One of) the reason(s) we never turned this on by default was because we weren't > entirely sure how much of a performance impact it would have (and it would be > terrible if we reduced performance significantly while watching for > significantly-reduced performance). I just want to make sure that this has been > checked, and everyone is okay with the potential overhead. That's why all of the DB stuff is disabled by default still; the only impact on the UI thread now is the initial task creation and then enumerating the render hosts every 10 seconds. https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:155: &specified_interval) || On 2013/09/13 18:14:09, D Cronin wrote: > indenation That's the fourth round of this. If you want a specific indentation change here, please give the exact one. https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:403: if (handle == 0) { On 2013/09/13 18:14:09, D Cronin wrote: > Since we don't care about any type other than content::PROCESS_TYPE_BROWSER (per > ProcessMetricsHistory::RunPerformanceTriggers()), do we need to add it to the > map at all, if we don't have database_logging_enabled_? i.e., could this > instead be: > if (handle == 0 || > (process_type != content::PROCESS_TYPE_BROWSER && > !database_logging_enabled_)) { > // If the process is invalid, or we are not interested in the process type, > // do not add the process to the map. > return; > } Monitoring browser performance is just the first step, we'll add more as we go along. Not sure if there's any point in adding a temporary filter like this. https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.h (left): https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:12: #include "base/files/file_path.h" On 2013/09/13 18:14:09, D Cronin wrote: > still need this one. Done https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.h (right): https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:23: namespace content { On 2013/09/13 18:14:09, D Cronin wrote: > you #include this, and have an instance in PerformanceMonitor. No need to > forward declare the class. Done. https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:160: void ProcessIsAlive(const base::ProcessHandle& handle, On 2013/09/13 18:14:09, D Cronin wrote: > Please mention in the comment that this will begin tracking the process if we > are not already. Why does that matter in terms of the API? https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:199: base::Time next_collection_time_; On 2013/09/13 18:14:09, D Cronin wrote: > #include "base/time/time.h" Done. https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:208: base::DelayTimer<PerformanceMonitor> timer_; On 2013/09/13 18:14:09, D Cronin wrote: > #include "base/timer/timer.h" This really just feels like unnecessary clutter, but sure. https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... File chrome/browser/performance_monitor/process_metrics_history.cc (right): https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/process_metrics_history.cc:67: sample_count_++; On 2013/09/13 18:14:09, D Cronin wrote: > nit: ++sample_count; Why? This is a POD, not an iterator or another complex type with a pre/post increment operator. https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/process_metrics_history.cc:82: if (min_cpu_usage_ > kHighCPUUtilizationThreshold) { On 2013/09/13 18:14:09, D Cronin wrote: > Just FYI: > > The different OSs report the CPU usage differently. Linux and Mac will report > on a per-CPU basis, so having a CPU usage of 90% on these OSs is high, but not > terrible (in the initial testing for this, we saw readings into 200% - 400% > usage). Windows, though, reports on a [0, 100] range. > > I'm sure this can be accounted for (just as the different types and powers of > CPUs must be accounted for), but wanted to make sure you were aware. That's fine; for this initial test we want to catch as few cases as possible, and then gradually increase scope.
https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:375: static int current_update_sequence = 0; On 2013/09/13 20:41:55, Oystein wrote: > On 2013/09/13 18:14:09, D Cronin wrote: > > On 2013/09/12 19:03:24, Oystein wrote: > > > On 2013/09/12 18:20:42, D Cronin wrote: > > > > Prefer a member variable over static int for something like this. > > > > > > The idea is that the sequence ID should be carried along in the chain of > jobs > > as > > > an argument rather than being encoded in object state, as an added safeguard > > in > > > case of concurrent runs (less shared state between threads). Moving this to > a > > > member var increases the likelihood of the member being used by the > > > background/IO threads rather than the argument. > > > > Is this really a concern, though? GatherMetricsMapOnIOThread() is only called > > from GatherMetricsMapOnUIThread(), and we don't reset the timer until that > task > > is completed, right? So aren't we guaranteed that only one of these will be > > running at a time (either GatherMetricsOnUI or GatherMetricsOnIO)? > > > > If not, you have a much bigger problem than a boolean variable - metrics_map_ > is > > referenced in both. :) > > Oh I'll readily admit it's mainly an artifact from an earlier version of the > code which was still using RepeatingTimer rather than manually starting a > DelayTimer, but as moving this to a member variable gives no real benefit (4 > bytes size increase in PerformanceMonitor vs 4 bytes extra per temporary job) > and *does* make the code less robust, however slightly, it doesn't seem sensible > to change..? It wouldn't be for performance benefits - the 4 bytes it saves by having only one copy (in PM) vs a permanent and a temporary copy are inconsequential overall. It's mostly just because the variable is used like a member variable - it's referenced in multiple methods of the object, and persists across method calls. But if you feel strongly about it, it's fine to stay as-is. https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:155: &specified_interval) || On 2013/09/13 20:41:55, Oystein wrote: > On 2013/09/13 18:14:09, D Cronin wrote: > > indenation > > That's the fourth round of this. If you want a specific indentation change here, > please give the exact one. if (!base::StringToInt( CommandLine::ForCurrentProcess()->GetSwitchValueASCII( switches::kPerformanceMonitorGathering, &specified_interval) || specified_interval <= 0) { ... } Yeah, sorry - this line's tricky. :) Indentation rules in effect here: line "1": no indentation line "2": +4 for the if-statement, +4 for the continued statement (to avoid confusion with to separate clauses in the if-statement) line "3": +4 on top of line "2" for parameter indentation line "4": same level as line "3", since it's another parameter to the same function call - there should be no reason it is indented less than the previous. line "5": +4 for the if-statement, so it's on the same indentation as the first clause of the if (i.e. "!base::String..."). https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:403: if (handle == 0) { On 2013/09/13 20:41:55, Oystein wrote: > On 2013/09/13 18:14:09, D Cronin wrote: > > Since we don't care about any type other than content::PROCESS_TYPE_BROWSER > (per > > ProcessMetricsHistory::RunPerformanceTriggers()), do we need to add it to the > > map at all, if we don't have database_logging_enabled_? i.e., could this > > instead be: > > if (handle == 0 || > > (process_type != content::PROCESS_TYPE_BROWSER && > > !database_logging_enabled_)) { > > // If the process is invalid, or we are not interested in the process type, > > // do not add the process to the map. > > return; > > } > > Monitoring browser performance is just the first step, we'll add more as we go > along. Not sure if there's any point in adding a temporary filter like this. How soon will the rest be added? This can (greatly) reduce the number of processes we need to watch (and thus, reduce the effort of iterating through, and the cost of storing them in-memory). And, of course, the question is - why not? It requires no redesign, nor much additional code, and is a quick switch when you decide to monitor the rest. https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.h (right): https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:160: void ProcessIsAlive(const base::ProcessHandle& handle, On 2013/09/13 20:41:55, Oystein wrote: > On 2013/09/13 18:14:09, D Cronin wrote: > > Please mention in the comment that this will begin tracking the process if we > > are not already. > > Why does that matter in terms of the API? To me, it's otherwise unclear when we may or may not add any new processes to the map. Anyone looking at the code itself will understand, but we generally try to practice having descriptive method names that accurately describe what we are doing. While external callers won't care, that could be said of any private method. https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:208: base::DelayTimer<PerformanceMonitor> timer_; On 2013/09/13 20:41:55, Oystein wrote: > On 2013/09/13 18:14:09, D Cronin wrote: > > #include "base/timer/timer.h" > > This really just feels like unnecessary clutter, but sure. Matter of opinion, but yeah. In some cases, I happen to agree (for instance, I think that scoped_ptr or <string> is all but standard, and should be exempt). But we have an agreed-upon style, which says we "include what we use". This makes refactors that kill transitive includes much easier, and makes it clear which files use what. https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... File chrome/browser/performance_monitor/process_metrics_history.cc (right): https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/process_metrics_history.cc:67: sample_count_++; On 2013/09/13 20:41:55, Oystein wrote: > On 2013/09/13 18:14:09, D Cronin wrote: > > nit: ++sample_count; > > Why? This is a POD, not an iterator or another complex type with a pre/post > increment operator. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Preincrement_a...
https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://codereview.chromium.org/23825004/diff/7013/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:375: static int current_update_sequence = 0; On 2013/09/13 21:38:08, D Cronin wrote: > On 2013/09/13 20:41:55, Oystein wrote: > > On 2013/09/13 18:14:09, D Cronin wrote: > > > On 2013/09/12 19:03:24, Oystein wrote: > > > > On 2013/09/12 18:20:42, D Cronin wrote: > > > > > Prefer a member variable over static int for something like this. > > > > > > > > The idea is that the sequence ID should be carried along in the chain of > > jobs > > > as > > > > an argument rather than being encoded in object state, as an added > safeguard > > > in > > > > case of concurrent runs (less shared state between threads). Moving this > to > > a > > > > member var increases the likelihood of the member being used by the > > > > background/IO threads rather than the argument. > > > > > > Is this really a concern, though? GatherMetricsMapOnIOThread() is only > called > > > from GatherMetricsMapOnUIThread(), and we don't reset the timer until that > > task > > > is completed, right? So aren't we guaranteed that only one of these will be > > > running at a time (either GatherMetricsOnUI or GatherMetricsOnIO)? > > > > > > If not, you have a much bigger problem than a boolean variable - > metrics_map_ > > is > > > referenced in both. :) > > > > Oh I'll readily admit it's mainly an artifact from an earlier version of the > > code which was still using RepeatingTimer rather than manually starting a > > DelayTimer, but as moving this to a member variable gives no real benefit (4 > > bytes size increase in PerformanceMonitor vs 4 bytes extra per temporary job) > > and *does* make the code less robust, however slightly, it doesn't seem > sensible > > to change..? > > It wouldn't be for performance benefits - the 4 bytes it saves by having only > one copy (in PM) vs a permanent and a temporary copy are inconsequential > overall. It's mostly just because the variable is used like a member variable - > it's referenced in multiple methods of the object, and persists across method > calls. But if you feel strongly about it, it's fine to stay as-is. Mainly it's because the value is a property of the update sequence and not the PerformanceMonitor instance, hence it makes sense to follow the job chain. This also helps signal that only the original function should/can modify the generator of the sequence (the variable itself). https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:155: &specified_interval) || On 2013/09/13 21:38:08, D Cronin wrote: > On 2013/09/13 20:41:55, Oystein wrote: > > On 2013/09/13 18:14:09, D Cronin wrote: > > > indenation > > > > That's the fourth round of this. If you want a specific indentation change > here, > > please give the exact one. > > if (!base::StringToInt( > CommandLine::ForCurrentProcess()->GetSwitchValueASCII( > switches::kPerformanceMonitorGathering, > &specified_interval) || > specified_interval <= 0) { > ... > } > > Yeah, sorry - this line's tricky. :) > > Indentation rules in effect here: > line "1": no indentation > line "2": +4 for the if-statement, +4 for the continued statement (to avoid > confusion with to separate clauses in the if-statement) > line "3": +4 on top of line "2" for parameter indentation > line "4": same level as line "3", since it's another parameter to the same > function call - there should be no reason it is indented less than the previous. > line "5": +4 for the if-statement, so it's on the same indentation as the first > clause of the if (i.e. "!base::String..."). Line 4 isn't another parameter to the same function call as line 3, it's a parameter to base::StringToInt(). https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:403: if (handle == 0) { On 2013/09/13 21:38:08, D Cronin wrote: > On 2013/09/13 20:41:55, Oystein wrote: > > On 2013/09/13 18:14:09, D Cronin wrote: > > > Since we don't care about any type other than content::PROCESS_TYPE_BROWSER > > (per > > > ProcessMetricsHistory::RunPerformanceTriggers()), do we need to add it to > the > > > map at all, if we don't have database_logging_enabled_? i.e., could this > > > instead be: > > > if (handle == 0 || > > > (process_type != content::PROCESS_TYPE_BROWSER && > > > !database_logging_enabled_)) { > > > // If the process is invalid, or we are not interested in the process > type, > > > // do not add the process to the map. > > > return; > > > } > > > > Monitoring browser performance is just the first step, we'll add more as we go > > along. Not sure if there's any point in adding a temporary filter like this. > > How soon will the rest be added? This can (greatly) reduce the number of > processes we need to watch (and thus, reduce the effort of iterating through, > and the cost of storing them in-memory). And, of course, the question is - why > not? It requires no redesign, nor much additional code, and is a quick switch > when you decide to monitor the rest. If there's any issues with monitoring all the Chrome processes in the system, I'd rather we find out now rather than after more time is spent on this. https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... File chrome/browser/performance_monitor/process_metrics_history.cc (right): https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/process_metrics_history.cc:67: sample_count_++; On 2013/09/13 21:38:08, D Cronin wrote: > On 2013/09/13 20:41:55, Oystein wrote: > > On 2013/09/13 18:14:09, D Cronin wrote: > > > nit: ++sample_count; > > > > Why? This is a POD, not an iterator or another complex type with a pre/post > > increment operator. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Preincrement_a... Yeah that's my point. "For simple scalar (non-object) values there is no reason to prefer one form and we allow either. For iterators and other template types, use pre-increment.".
https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:155: &specified_interval) || On 2013/09/13 21:57:50, Oystein wrote: > On 2013/09/13 21:38:08, D Cronin wrote: > > On 2013/09/13 20:41:55, Oystein wrote: > > > On 2013/09/13 18:14:09, D Cronin wrote: > > > > indenation > > > > > > That's the fourth round of this. If you want a specific indentation change > > here, > > > please give the exact one. > > > > if (!base::StringToInt( > > CommandLine::ForCurrentProcess()->GetSwitchValueASCII( > > switches::kPerformanceMonitorGathering, > > &specified_interval) || > > specified_interval <= 0) { > > ... > > } > > > > Yeah, sorry - this line's tricky. :) > > > > Indentation rules in effect here: > > line "1": no indentation > > line "2": +4 for the if-statement, +4 for the continued statement (to avoid > > confusion with to separate clauses in the if-statement) > > line "3": +4 on top of line "2" for parameter indentation > > line "4": same level as line "3", since it's another parameter to the same > > function call - there should be no reason it is indented less than the > previous. > > line "5": +4 for the if-statement, so it's on the same indentation as the > first > > clause of the if (i.e. "!base::String..."). > > Line 4 isn't another parameter to the same function call as line 3, it's a > parameter to base::StringToInt(). I would indent &specified_interval one more space. This would also look less ugly overall if you used a temporary variable to hold the switch value. https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.h (right): https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:160: void ProcessIsAlive(const base::ProcessHandle& handle, On 2013/09/13 21:38:08, D Cronin wrote: > On 2013/09/13 20:41:55, Oystein wrote: > > On 2013/09/13 18:14:09, D Cronin wrote: > > > Please mention in the comment that this will begin tracking the process if > we > > > are not already. > > > > Why does that matter in terms of the API? > > To me, it's otherwise unclear when we may or may not add any new processes to > the map. Anyone looking at the code itself will understand, but we generally > try to practice having descriptive method names that accurately describe what we > are doing. While external callers won't care, that could be said of any private > method. As a comment on a private method, you're not just documenting for people who want to know how to use it (the API expectations), but for people who may want to understand or modify this code. https://codereview.chromium.org/23825004/diff/16001/chrome/browser/performanc... File chrome/browser/performance_monitor/constants.h (right): https://codereview.chromium.org/23825004/diff/16001/chrome/browser/performanc... chrome/browser/performance_monitor/constants.h:25: const int kSampleInterval = 10; Can this have units (like the constant below it)? Is this in seconds? https://codereview.chromium.org/23825004/diff/16001/chrome/browser/performanc... chrome/browser/performance_monitor/constants.h:46: // Performance alert thresholds Please document what this threshold is, and that it is a percent. https://codereview.chromium.org/23825004/diff/16001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://codereview.chromium.org/23825004/diff/16001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:166: gather_interval_in_seconds_ = This can be combined with 161. https://codereview.chromium.org/23825004/diff/16001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.h (right): https://codereview.chromium.org/23825004/diff/16001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:159: void ProcessIsAlive(const base::ProcessHandle& handle, The way this is named suggests that it's a boolean const function. Better would be a name like WatchAliveProcess or MarkProcessAlive, something that indicates what it's actually doing. https://codereview.chromium.org/23825004/diff/16001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:218: bool disable_timer_autostart_; disable_timer_autostart_for_testing_ ?
https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:155: &specified_interval) || On 2013/09/18 23:01:42, Yoyo Zhou wrote: > On 2013/09/13 21:57:50, Oystein wrote: > > On 2013/09/13 21:38:08, D Cronin wrote: > > > On 2013/09/13 20:41:55, Oystein wrote: > > > > On 2013/09/13 18:14:09, D Cronin wrote: > > > > > indenation > > > > > > > > That's the fourth round of this. If you want a specific indentation change > > > here, > > > > please give the exact one. > > > > > > if (!base::StringToInt( > > > CommandLine::ForCurrentProcess()->GetSwitchValueASCII( > > > switches::kPerformanceMonitorGathering, > > > &specified_interval) || > > > specified_interval <= 0) { > > > ... > > > } > > > > > > Yeah, sorry - this line's tricky. :) > > > > > > Indentation rules in effect here: > > > line "1": no indentation > > > line "2": +4 for the if-statement, +4 for the continued statement (to avoid > > > confusion with to separate clauses in the if-statement) > > > line "3": +4 on top of line "2" for parameter indentation > > > line "4": same level as line "3", since it's another parameter to the same > > > function call - there should be no reason it is indented less than the > > previous. > > > line "5": +4 for the if-statement, so it's on the same indentation as the > > first > > > clause of the if (i.e. "!base::String..."). > > > > Line 4 isn't another parameter to the same function call as line 3, it's a > > parameter to base::StringToInt(). > > I would indent &specified_interval one more space. > This would also look less ugly overall if you used a temporary variable to hold > the switch value. Done. I didn't write the code just moved it, so I didn't want to mess with it too much. https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.h (right): https://codereview.chromium.org/23825004/diff/12001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:160: void ProcessIsAlive(const base::ProcessHandle& handle, On 2013/09/18 23:01:42, Yoyo Zhou wrote: > On 2013/09/13 21:38:08, D Cronin wrote: > > On 2013/09/13 20:41:55, Oystein wrote: > > > On 2013/09/13 18:14:09, D Cronin wrote: > > > > Please mention in the comment that this will begin tracking the process if > > we > > > > are not already. > > > > > > Why does that matter in terms of the API? > > > > To me, it's otherwise unclear when we may or may not add any new processes to > > the map. Anyone looking at the code itself will understand, but we generally > > try to practice having descriptive method names that accurately describe what > we > > are doing. While external callers won't care, that could be said of any > private > > method. > > As a comment on a private method, you're not just documenting for people who > want to know how to use it (the API expectations), but for people who may want > to understand or modify this code. Done. https://codereview.chromium.org/23825004/diff/16001/chrome/browser/performanc... File chrome/browser/performance_monitor/constants.h (right): https://codereview.chromium.org/23825004/diff/16001/chrome/browser/performanc... chrome/browser/performance_monitor/constants.h:25: const int kSampleInterval = 10; On 2013/09/18 23:01:42, Yoyo Zhou wrote: > Can this have units (like the constant below it)? Is this in seconds? Done. https://codereview.chromium.org/23825004/diff/16001/chrome/browser/performanc... chrome/browser/performance_monitor/constants.h:46: // Performance alert thresholds On 2013/09/18 23:01:42, Yoyo Zhou wrote: > Please document what this threshold is, and that it is a percent. Done. https://codereview.chromium.org/23825004/diff/16001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://codereview.chromium.org/23825004/diff/16001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:166: gather_interval_in_seconds_ = On 2013/09/18 23:01:42, Yoyo Zhou wrote: > This can be combined with 161. Can you expand on this? Do you mean only capping the value if it's overridden by the switch, or do you mean moving specified_interval out of the if() and using it in 166? https://codereview.chromium.org/23825004/diff/16001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.h (right): https://codereview.chromium.org/23825004/diff/16001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:159: void ProcessIsAlive(const base::ProcessHandle& handle, On 2013/09/18 23:01:42, Yoyo Zhou wrote: > The way this is named suggests that it's a boolean const function. Better would > be a name like WatchAliveProcess or MarkProcessAlive, something that indicates > what it's actually doing. If you prefer, sure. "ProcessIsAlive" is already declarative though, not interrogative. https://codereview.chromium.org/23825004/diff/16001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.h:218: bool disable_timer_autostart_; On 2013/09/18 23:01:42, Yoyo Zhou wrote: > disable_timer_autostart_for_testing_ ? Done.
LGTM https://codereview.chromium.org/23825004/diff/16001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://codereview.chromium.org/23825004/diff/16001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:166: gather_interval_in_seconds_ = On 2013/09/19 14:16:24, Oystein wrote: > On 2013/09/18 23:01:42, Yoyo Zhou wrote: > > This can be combined with 161. > > Can you expand on this? Do you mean only capping the value if it's overridden by > the switch, or do you mean moving specified_interval out of the if() and using > it in 166? I meant only capping if overridden by the switch. I assume that capping wouldn't apply with the default value, because it would just be pointless to set the default value lower than the cap. But I don't feel strongly about this.
Thanks :) https://codereview.chromium.org/23825004/diff/16001/chrome/browser/performanc... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://codereview.chromium.org/23825004/diff/16001/chrome/browser/performanc... chrome/browser/performance_monitor/performance_monitor.cc:166: gather_interval_in_seconds_ = On 2013/09/19 19:28:23, Yoyo Zhou wrote: > On 2013/09/19 14:16:24, Oystein wrote: > > On 2013/09/18 23:01:42, Yoyo Zhou wrote: > > > This can be combined with 161. > > > > Can you expand on this? Do you mean only capping the value if it's overridden > by > > the switch, or do you mean moving specified_interval out of the if() and using > > it in 166? > > I meant only capping if overridden by the switch. I assume that capping wouldn't > apply with the default value, because it would just be pointless to set the > default value lower than the cap. But I don't feel strongly about this. Done. Yeah a DCHECK is fine instead of the capping in this case.
thakis: chrome/browser/chrome_browser_main.cc please :)
Ping :)
browser_main.cc lgtm (Please ping me when I don't respond within a day, sorry that I added almost a week here :-/)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oysteine@chromium.org/23825004/106001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oysteine@chromium.org/23825004/142001
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oysteine@chromium.org/23825004/142001
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oysteine@chromium.org/23825004/142001
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oysteine@chromium.org/23825004/180001
Failed to apply patch for tools/metrics/actions/chromeactions.txt: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file tools/metrics/actions/chromeactions.txt Hunk #1 FAILED at 1440. 1 out of 1 hunk FAILED -- saving rejects to file tools/metrics/actions/chromeactions.txt.rej Patch: tools/metrics/actions/chromeactions.txt Index: tools/metrics/actions/chromeactions.txt diff --git a/tools/metrics/actions/chromeactions.txt b/tools/metrics/actions/chromeactions.txt index a9c51e1a929fbe3fabfd9c44962c0537ad89c9e0..7a782b6e83206bc2a91dcd0588aa7cfabb7dc177 100644 --- a/tools/metrics/actions/chromeactions.txt +++ b/tools/metrics/actions/chromeactions.txt @@ -1440,6 +1440,7 @@ 0xa7613e3a71f2d755 PasswordManager_RemoveSavedPassword 0x36bb6559696dc912 Paste 0x5d0e6942f354a06c PasteAndMatchStyle +0xb0c1b1bb1cef8c89 PerformanceMonitor.HighCPU.Browser 0xca471265efb33961 PluginContextMenu_RotateClockwise 0xa260d9dde6e550c4 PluginContextMenu_RotateCounterclockwise 0xd0aab07db234653a Plugin_200ForByteRange
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oysteine@chromium.org/23825004/191001
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oysteine@chromium.org/23825004/191001
Message was sent while issue was closed.
Change committed as 226466 |
