Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10656052:
Performance monitor stats gathering. (Closed)
|
Created:
8 years, 6 months ago by mitchellwrosen Modified:
8 years, 4 months ago CC:
chromium-reviews, Aaron Boodman, Matt Tytel, clintstaley Base URL:
http://git.chromium.org/chromium/src.git@cpm_main Visibility:
Public. |
DescriptionPerformance monitor stats gathering.
Dependent upon https://chromiumcodereview.appspot.com/10568014
TBR=brettw@chromium.org
BUG=130212
TEST=included
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149766
Patch Set 1 #
Total comments: 26
Patch Set 2 : Changes per comments #
Total comments: 28
Patch Set 3 : Numero 3 #
Total comments: 25
Patch Set 4 : Fixes #
Total comments: 2
Patch Set 5 : Changes per comments #Patch Set 6 : #
Total comments: 15
Patch Set 7 : Nits #
Total comments: 4
Patch Set 8 : Fixed Windows bug, duplicate CPU usage gather, couple nits #
Total comments: 12
Patch Set 9 : #
Total comments: 8
Patch Set 10 : Nits #
Total comments: 2
Patch Set 11 : Sync with trunk #
Messages
Total messages: 37 (0 generated)
Sorry for the late review, I was out last week. https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.cc:36: T GetMedian(std::vector<T> vec) { Since you never expect this to be called with size 0, DCHECKing the size is a good idea. https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.cc:187: UpdateMetricsMap(); I don't understand why this comes in the middle, and I can see from the test that it's intentional. Is it worth explaining in a comment what's going on in this order? https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.cc:194: if (metrics_map_.size() != 0) { nit: you use size() at 227 but size() != 0 here. I don't have a strong preference, but be consistent. https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.cc:237: // Remove old process handles nit: comments should end in . https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.cc:240: if (iter->first == base::kNullProcessHandle) { I don't understand how the key of the metrics_map_ is going to change. https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.cc:241: // base::CloseProcessHandle(iter->first); // Necessary? This is commented out. https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.cc:245: iter->second->GetCPUUsage(); Do you need this? Seems like it should have been primed when you added it to metrics_map_ the first time. https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.cc:263: if (metrics_map_.find(process_handle) == metrics_map_.end()) { You can use ContainsKey from stl_util.h. https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... File chrome/browser/performance_monitor/performance_monitor.h (right): https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.h:100: void AddEventOnBackgroundThread(scoped_ptr<Event> event); nit: since AddEventOnBackgroundThread is not described by the comment above, it should have a newline before it. Same for AddMetric... https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... File chrome/browser/performance_monitor/performance_monitor_browsertest.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:382: // Spin for a while, so CPU usage isn't 0 Why is this necessary? Could CPU usage be 0 in the real world? https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:383: for (int i = 0; i < 1000000000; ++i) { I'd be careful with something like this... a compiler could perhaps even optimize it out. Perhaps i could be moved to function scope, and then ASSERT_EQ(100000000000, i).
https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.cc:36: T GetMedian(std::vector<T> vec) { On 2012/07/09 21:33:36, Yoyo Zhou wrote: > Since you never expect this to be called with size 0, DCHECKing the size is a > good idea. Done. https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.cc:187: UpdateMetricsMap(); On 2012/07/09 21:33:36, Yoyo Zhou wrote: > I don't understand why this comes in the middle, and I can see from the test > that it's intentional. Is it worth explaining in a comment what's going on in > this order? Done. If the comment isn't clear enough, let me know. I couldn't really decide on a good way to word what I was trying to say. Basically, a call to ProcessMetrics::GetCPUUsage returns a percentage of CPU clock cycles that the process has used since the last call. That's why it's called before the metrics map is updated, to collect the average over the past, say, two minutes. Memory usage, on the other hand, is collected as an instantaneous usage. Thus immediately after updating the metrics map with the current running processes, it's safe to collect the memory usage. https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.cc:194: if (metrics_map_.size() != 0) { On 2012/07/09 21:33:36, Yoyo Zhou wrote: > nit: you use size() at 227 but size() != 0 here. I don't have a strong > preference, but be consistent. Done. https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.cc:237: // Remove old process handles On 2012/07/09 21:33:36, Yoyo Zhou wrote: > nit: comments should end in . Done. https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.cc:240: if (iter->first == base::kNullProcessHandle) { On 2012/07/09 21:33:36, Yoyo Zhou wrote: > I don't understand how the key of the metrics_map_ is going to change. The challenge I was trying to overcome is not gathering statistics for dead processes. Presumably if performance monitor gathers statistics every two minutes, there will be processes that have closed in that time. That being said, you're right. A ProcessHandle isn't just going to become 0, you'd have to open a new ProcessHandle and check it. I was basing this code off of the fact that calling handle() of a Process will return 0 if the process is no longer running. That being said, would this be a fair solution? - Keep a map of Process to ProcessMetrics (instead of ProcessHandle to ProcessMetrics) - Check process.handle() before attempting to gather statistics https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.cc:241: // base::CloseProcessHandle(iter->first); // Necessary? On 2012/07/09 21:33:36, Yoyo Zhou wrote: > This is commented out. Yeah, I left that in there because I was hoping you'd have some advice for me on what to do with a dead process =) I open a process with OpenProcessHandle. The process dies. Do I then need to close the handle with CloseProcessHandle? (btw - if a Process->ProcessMetrics map makes more sense per my previous comment, then just ignore this question =)) https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.cc:245: iter->second->GetCPUUsage(); On 2012/07/09 21:33:36, Yoyo Zhou wrote: > Do you need this? Seems like it should have been primed when you added it to > metrics_map_ the first time. This re-primes the processes already in the map. The next one below primes the new processes. The code may be simpler if I just wipe the entire map every time this method is called, instead of trying to stitch together the new processes with the old ones (and tossing out the dead ones). What do you think? https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.cc:263: if (metrics_map_.find(process_handle) == metrics_map_.end()) { On 2012/07/09 21:33:36, Yoyo Zhou wrote: > You can use ContainsKey from stl_util.h. Done. https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... File chrome/browser/performance_monitor/performance_monitor.h (right): https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.h:100: void AddEventOnBackgroundThread(scoped_ptr<Event> event); On 2012/07/09 21:33:36, Yoyo Zhou wrote: > nit: since AddEventOnBackgroundThread is not described by the comment above, it > should have a newline before it. Same for AddMetric... Done. https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... File chrome/browser/performance_monitor/performance_monitor_browsertest.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:382: // Spin for a while, so CPU usage isn't 0 On 2012/07/09 21:33:36, Yoyo Zhou wrote: > Why is this necessary? Could CPU usage be 0 in the real world? Yeah, the CPU usage could be zero in the real world. I was trying to force the process to consume a noticeable amount of cycles just to test that GetCPUUsage is working properly. It's a rubbish test, I agree. Do you have any other suggestions? https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:383: for (int i = 0; i < 1000000000; ++i) { On 2012/07/09 21:33:36, Yoyo Zhou wrote: > I'd be careful with something like this... a compiler could perhaps even > optimize it out. Perhaps i could be moved to function scope, and then > ASSERT_EQ(100000000000, i). Done.
https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.cc:240: if (iter->first == base::kNullProcessHandle) { On 2012/07/09 22:45:23, mitchellwrosen wrote: > On 2012/07/09 21:33:36, Yoyo Zhou wrote: > > I don't understand how the key of the metrics_map_ is going to change. > > The challenge I was trying to overcome is not gathering statistics for dead > processes. Presumably if performance monitor gathers statistics every two > minutes, there will be processes that have closed in that time. > > That being said, you're right. A ProcessHandle isn't just going to become 0, > you'd have to open a new ProcessHandle and check it. I was basing this code off > of the fact that calling handle() of a Process will return 0 if the process is > no longer running. > > That being said, would this be a fair solution? > - Keep a map of Process to ProcessMetrics (instead of ProcessHandle to > ProcessMetrics) > - Check process.handle() before attempting to gather statistics Seems like you should be checking GetTerminationStatus to see if the process has exited. https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.cc:241: // base::CloseProcessHandle(iter->first); // Necessary? On 2012/07/09 22:45:23, mitchellwrosen wrote: > On 2012/07/09 21:33:36, Yoyo Zhou wrote: > > This is commented out. > > Yeah, I left that in there because I was hoping you'd have some advice for me on > what to do with a dead process =) > > I open a process with OpenProcessHandle. The process dies. Do I then need to > close the handle with CloseProcessHandle? (btw - if a Process->ProcessMetrics > map makes more sense per my previous comment, then just ignore this question =)) I would say "yes, you need to close it" based on the comment on process_util.h:OpenProcessHandle. https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor.cc:245: iter->second->GetCPUUsage(); On 2012/07/09 22:45:23, mitchellwrosen wrote: > On 2012/07/09 21:33:36, Yoyo Zhou wrote: > > Do you need this? Seems like it should have been primed when you added it to > > metrics_map_ the first time. > > This re-primes the processes already in the map. The next one below primes the > new processes. The code may be simpler if I just wipe the entire map every time > this method is called, instead of trying to stitch together the new processes > with the old ones (and tossing out the dead ones). What do you think? Either seems fine. https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... File chrome/browser/performance_monitor/performance_monitor_browsertest.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/1/chrome/browser/perform... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:382: // Spin for a while, so CPU usage isn't 0 On 2012/07/09 22:45:23, mitchellwrosen wrote: > On 2012/07/09 21:33:36, Yoyo Zhou wrote: > > Why is this necessary? Could CPU usage be 0 in the real world? > > Yeah, the CPU usage could be zero in the real world. I was trying to force the > process to consume a noticeable amount of cycles just to test that GetCPUUsage > is working properly. It's a rubbish test, I agree. Do you have any other > suggestions? Well, the LOG(INFO) also suggests that these are tests that have to be looked at by a human. When these tests are run by the bots, then, you should expect that only assertion failures will be looked at. Remember that you're testing your system, not necessarily the low-level components it's built on. (Although I don't see tests for GetCPUUsage and such, presumably because they're hard to test.) I don't know what to suggest, but here are some thoughts: - Change the LOG(INFO) to a DVLOG(1) or some such, so that it doesn't spam the test logs. You would have to run it yourself with a debug build and --vmodule=performance_monitor_browsertest=1 to see the messages though. - Ensure that the CPU spinning doesn't slow down the bots running tests too much. - If it's possible, check that the medians you compute are actually medians. https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.cc:265: linked_ptr<base::ProcessMetrics> process_metrics( This should be moved to inside the if statement.
https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... File chrome/browser/performance_monitor/constants.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/constants.cc:9: const char kMetricCPUUsage[] = "cpu_usage"; Provide at least one comment for this group. https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.cc:130: database_->AddMetric(key, value); These two functions can be combined, since they don't have to do any special scoping operations like events do. Just bind the database task in AddMetric(). https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.cc:189: void PerformanceMonitor::GatherStatistics() { If the method is public and runs on the background thread, it should be called GatherStatisticsOnBackgroundThread(). https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.cc:190: CHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); Will gathering the stats on the background thread return the same as gathering stats on the UI thread, or will it affect the pids? (Please double check). https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.cc:212: AddMetric(performance_monitor::kMetricCPUUsage, If you're on the background thread, you don't need to use the wrapper function. https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.cc:233: std::sort(private_memory_vec.begin(), private_memory_vec.end()); Nit: Check the size before sorting (even though sort will check, it makes the code look cleaner). Perhaps just have if (!private_memory_vec.size() return; right after you gather? https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.cc:258: // Add new process handles add a . https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.cc:260: ChromeProcessList chrome_processes(GetRunningChromeProcesses(browser_pid)); If you are getting the list of running processes, should browser_pid be base::GetParentProcessId on POSIX? (I'm not sure, but I seem to recall that it might be.) https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... File chrome/browser/performance_monitor/performance_monitor.h (right): https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.h:58: // last call to GatherStatistics(). Make the helpers private? https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... File chrome/browser/performance_monitor/performance_monitor_browsertest.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:135: void GetStatsForActivityAndMetricOnBackgroundThread( Add function documentation. https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:367: LOG(INFO) << "Private memory usage median: " << stats[0].value; Remove debugging LOGs. https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:376: // TODO(mwrosen) something less barbaric? hmmm...check out base::? base::PlatformThread::Sleep may or might work (I'm not 100% sure you'll get CPU usage from it, but it could be worth a shot). Otherwise base::OneShotTimer might be worth a try. https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:381: ASSERT_EQ(1000000000, i); If this stays in here, you should define this to be a const at the top.
https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... File chrome/browser/performance_monitor/constants.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/constants.cc:9: const char kMetricCPUUsage[] = "cpu_usage"; On 2012/07/11 16:28:08, D Cronin wrote: > Provide at least one comment for this group. Done. https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.cc:130: database_->AddMetric(key, value); On 2012/07/11 16:28:08, D Cronin wrote: > These two functions can be combined, since they don't have to do any special > scoping operations like events do. Just bind the database task in AddMetric(). Done. https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.cc:189: void PerformanceMonitor::GatherStatistics() { On 2012/07/11 16:28:08, D Cronin wrote: > If the method is public and runs on the background thread, it should be called > GatherStatisticsOnBackgroundThread(). Done. https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.cc:212: AddMetric(performance_monitor::kMetricCPUUsage, On 2012/07/11 16:28:08, D Cronin wrote: > If you're on the background thread, you don't need to use the wrapper function. Done. https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.cc:233: std::sort(private_memory_vec.begin(), private_memory_vec.end()); On 2012/07/11 16:28:08, D Cronin wrote: > Nit: Check the size before sorting (even though sort will check, it makes the > code look cleaner). Perhaps just have > if (!private_memory_vec.size() > return; > right after you gather? Done. https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.cc:258: // Add new process handles On 2012/07/11 16:28:08, D Cronin wrote: > add a . Done. https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.cc:260: ChromeProcessList chrome_processes(GetRunningChromeProcesses(browser_pid)); On 2012/07/11 16:28:08, D Cronin wrote: > If you are getting the list of running processes, should browser_pid be > base::GetParentProcessId on POSIX? (I'm not sure, but I seem to recall that it > might be.) I couldn't find anything to suggest otherwise. It's proven quite hard to track down exactly how to get the browser process ID. https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.cc:265: linked_ptr<base::ProcessMetrics> process_metrics( On 2012/07/10 21:58:00, Yoyo Zhou wrote: > This should be moved to inside the if statement. Done. https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... File chrome/browser/performance_monitor/performance_monitor.h (right): https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor.h:58: // last call to GatherStatistics(). On 2012/07/11 16:28:08, D Cronin wrote: > Make the helpers private? Done. https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... File chrome/browser/performance_monitor/performance_monitor_browsertest.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:135: void GetStatsForActivityAndMetricOnBackgroundThread( On 2012/07/11 16:28:08, D Cronin wrote: > Add function documentation. Done. https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:367: LOG(INFO) << "Private memory usage median: " << stats[0].value; On 2012/07/11 16:28:08, D Cronin wrote: > Remove debugging LOGs. Done. https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:376: // TODO(mwrosen) something less barbaric? On 2012/07/11 16:28:08, D Cronin wrote: > hmmm...check out base::? base::PlatformThread::Sleep may or might work (I'm not > 100% sure you'll get CPU usage from it, but it could be worth a shot). Otherwise > base::OneShotTimer might be worth a try. Sleeping doesn't consume CPU. OneShotTimer is awkward to use because I would have to split my test in two (the second half would be the callback). Not sure if that's possible since an InProcBrowserTest just expands the test class and test name to a class delimited by underscores. https://chromiumcodereview.appspot.com/10656052/diff/9007/chrome/browser/perf... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:381: ASSERT_EQ(1000000000, i); On 2012/07/11 16:28:08, D Cronin wrote: > If this stays in here, you should define this to be a const at the top. Done.
Please also sync with latest master. https://chromiumcodereview.appspot.com/10656052/diff/14002/chrome/browser/per... File chrome/browser/performance_monitor/constants.h (right): https://chromiumcodereview.appspot.com/10656052/diff/14002/chrome/browser/per... chrome/browser/performance_monitor/constants.h:22: // TODO(mtytel): When you make real metrics, delete the sample metric. Isn't this what's added? https://chromiumcodereview.appspot.com/10656052/diff/14002/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/14002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:8: #include <map> map and string are included in the .h file; you can remove them here. https://chromiumcodereview.appspot.com/10656052/diff/14002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:15: #include "base/process.h" linked_ptr, process, process_util already included. https://chromiumcodereview.appspot.com/10656052/diff/14002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:190: for (; iter != metrics_map_.end(); ++iter) Why not declare iter here, as normal? https://chromiumcodereview.appspot.com/10656052/diff/14002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:229: // on the current element, and advance by a call to iter = iter_next++. style violation - always use ++i, not i++ https://chromiumcodereview.appspot.com/10656052/diff/14002/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor.h (right): https://chromiumcodereview.appspot.com/10656052/diff/14002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.h:106: extra newline; also a short comment may be in order. https://chromiumcodereview.appspot.com/10656052/diff/14002/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor_browsertest.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/14002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:382: // Spin for a while, so CPU usage isn't 0 add a period. https://chromiumcodereview.appspot.com/10656052/diff/14002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:386: ASSERT_EQ(kSpinCount, i); Is this just testing that a for-loop runs? If so, I think we can probably safely get rid of it...
http://codereview.chromium.org/10656052/diff/9007/chrome/browser/performance_... File chrome/browser/performance_monitor/performance_monitor.cc (right): http://codereview.chromium.org/10656052/diff/9007/chrome/browser/performance_... chrome/browser/performance_monitor/performance_monitor.cc:265: linked_ptr<base::ProcessMetrics> process_metrics( On 2012/07/11 18:30:18, mitchellwrosen wrote: > On 2012/07/10 21:58:00, Yoyo Zhou wrote: > > This should be moved to inside the if statement. > > Done. I did not mean the #if, I meant the if statement below it (at 261). But this is probably a good idea too (keeping a complete statement within each ifdef block). http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... File chrome/browser/performance_monitor/constants.cc (right): http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... chrome/browser/performance_monitor/constants.cc:9: // Metric keys for statistics gathering. Comments such as this belong in the header file. http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... File chrome/browser/performance_monitor/constants.h (right): http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... chrome/browser/performance_monitor/constants.h:22: // TODO(mtytel): When you make real metrics, delete the sample metric. On 2012/07/17 18:07:58, D Cronin wrote: > Isn't this what's added? And the added constants should probably go here, below the comment (minus the TODO). http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... File chrome/browser/performance_monitor/performance_monitor.cc (right): http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:228: // Remove old process handles. Use two iteraterors to safely call erase() typo: iterators http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:229: // on the current element, and advance by a call to iter = iter_next++. On 2012/07/17 18:07:58, D Cronin wrote: > style violation - always use ++i, not i++ It's fine if you're assigning the old value of i. http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:233: iter = (iter_next == iter_end) ? iter_next : iter_next++; Yikes, it's super unreadable to squeeze everything into the for loop statement like this. See alarm_manager.cc:291-293 for an example of how to do this more gracefully. (I can't be certain if this will work for you, though, because it's too hard to read.)
http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... File chrome/browser/performance_monitor/constants.cc (right): http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... chrome/browser/performance_monitor/constants.cc:9: // Metric keys for statistics gathering. On 2012/07/19 21:35:35, Yoyo Zhou wrote: > Comments such as this belong in the header file. Done. http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... File chrome/browser/performance_monitor/performance_monitor.cc (right): http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:8: #include <map> On 2012/07/17 18:07:58, D Cronin wrote: > map and string are included in the .h file; you can remove them here. Isn't there a style rule against relying on transitive includes? http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:190: for (; iter != metrics_map_.end(); ++iter) On 2012/07/17 18:07:58, D Cronin wrote: > Why not declare iter here, as normal? Done. http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:228: // Remove old process handles. Use two iteraterors to safely call erase() On 2012/07/19 21:35:35, Yoyo Zhou wrote: > typo: iterators Done. http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:233: iter = (iter_next == iter_end) ? iter_next : iter_next++; On 2012/07/19 21:35:35, Yoyo Zhou wrote: > Yikes, it's super unreadable to squeeze everything into the for loop statement > like this. > > See alarm_manager.cc:291-293 for an example of how to do this more gracefully. > (I can't be certain if this will work for you, though, because it's too hard to > read.) Does it look better now? I can make it cleaner by removing iter_end. It's only there to save a call to .end() every iteration http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... File chrome/browser/performance_monitor/performance_monitor.h (right): http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.h:106: On 2012/07/17 18:07:58, D Cronin wrote: > extra newline; also a short comment may be in order. Done. http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... File chrome/browser/performance_monitor/performance_monitor_browsertest.cc (right): http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:382: // Spin for a while, so CPU usage isn't 0 On 2012/07/17 18:07:58, D Cronin wrote: > add a period. Done. http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:386: ASSERT_EQ(kSpinCount, i); On 2012/07/17 18:07:58, D Cronin wrote: > Is this just testing that a for-loop runs? If so, I think we can probably safely > get rid of it... See yoz's previous comment
http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... File chrome/browser/performance_monitor/performance_monitor.cc (right): http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:233: iter = (iter_next == iter_end) ? iter_next : iter_next++; On 2012/07/20 19:42:36, mitchellwrosen wrote: > On 2012/07/19 21:35:35, Yoyo Zhou wrote: > > Yikes, it's super unreadable to squeeze everything into the for loop statement > > like this. > > > > See alarm_manager.cc:291-293 for an example of how to do this more gracefully. > > (I can't be certain if this will work for you, though, because it's too hard > to > > read.) > > Does it look better now? I can make it cleaner by removing iter_end. It's only > there to save a call to .end() every iteration It still looks scary. I have to think too much to understand what these 2 lines are doing. A for statement is a great way to compress 3 lines of code into 1, but you have 5 here, and they're nontrivial. I would: - get rid of iter_end. The savings isn't worth the cost to the reader. - declare a variable in the body of the loop to be the second iterator (in this case, iter). That way the for statement only has to concern itself with one iterator (in this case, iter_next). What you want is iter = iter_next++; but it's much clearer to put this in the body of the loop and leave the third part of the for statement empty. (By the way, the comment is out of sync with the code, and I'd say you could delete the rest of it after the word "element") http://codereview.chromium.org/10656052/diff/30001/chrome/browser/performance... File chrome/browser/performance_monitor/constants.h (right): http://codereview.chromium.org/10656052/diff/30001/chrome/browser/performance... chrome/browser/performance_monitor/constants.h:18: // When you add a metric type, make sure to edit CPMDatabase::InitMetricDetails nit: add a period. http://codereview.chromium.org/10656052/diff/30001/chrome/browser/performance... File chrome/browser/performance_monitor/database.cc (right): http://codereview.chromium.org/10656052/diff/30001/chrome/browser/performance... chrome/browser/performance_monitor/database.cc:485: metric_details_map_[kMetricCPUUsage]= nit: missing space before =
http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... File chrome/browser/performance_monitor/performance_monitor.cc (right): http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:8: #include <map> On 2012/07/20 19:42:36, mitchellwrosen wrote: > On 2012/07/17 18:07:58, D Cronin wrote: > > map and string are included in the .h file; you can remove them here. > > Isn't there a style rule against relying on transitive includes? The rule seems to be special cased to omit a .cc file's "own" .h file. :|
http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... File chrome/browser/performance_monitor/performance_monitor.cc (right): http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:233: iter = (iter_next == iter_end) ? iter_next : iter_next++; On 2012/07/20 20:47:30, Yoyo Zhou wrote: > On 2012/07/20 19:42:36, mitchellwrosen wrote: > > On 2012/07/19 21:35:35, Yoyo Zhou wrote: > > > Yikes, it's super unreadable to squeeze everything into the for loop > statement > > > like this. > > > > > > See alarm_manager.cc:291-293 for an example of how to do this more > gracefully. > > > (I can't be certain if this will work for you, though, because it's too hard > > to > > > read.) > > > > Does it look better now? I can make it cleaner by removing iter_end. It's only > > there to save a call to .end() every iteration > > It still looks scary. I have to think too much to understand what these 2 lines > are doing. A for statement is a great way to compress 3 lines of code into 1, > but you have 5 here, and they're nontrivial. > > I would: > - get rid of iter_end. The savings isn't worth the cost to the reader. > - declare a variable in the body of the loop to be the second iterator (in this > case, iter). That way the for statement only has to concern itself with one > iterator (in this case, iter_next). What you want is > iter = iter_next++; > but it's much clearer to put this in the body of the loop and leave the third > part of the for statement empty. > > (By the way, the comment is out of sync with the code, and I'd say you could > delete the rest of it after the word "element") I'm not sure I understand. If only iter_next is initialized in the for statement, then I only have it to compare against .end(), so I would miss the last element. Anyways, I took out iter_end. If it's not clean enough please let me know!
http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... File chrome/browser/performance_monitor/performance_monitor.cc (right): http://codereview.chromium.org/10656052/diff/14002/chrome/browser/performance... chrome/browser/performance_monitor/performance_monitor.cc:233: iter = (iter_next == iter_end) ? iter_next : iter_next++; On 2012/07/20 23:05:27, mitchellwrosen wrote: > On 2012/07/20 20:47:30, Yoyo Zhou wrote: > > On 2012/07/20 19:42:36, mitchellwrosen wrote: > > > On 2012/07/19 21:35:35, Yoyo Zhou wrote: > > > > Yikes, it's super unreadable to squeeze everything into the for loop > > statement > > > > like this. > > > > > > > > See alarm_manager.cc:291-293 for an example of how to do this more > > gracefully. > > > > (I can't be certain if this will work for you, though, because it's too > hard > > > to > > > > read.) > > > > > > Does it look better now? I can make it cleaner by removing iter_end. It's > only > > > there to save a call to .end() every iteration > > > > It still looks scary. I have to think too much to understand what these 2 > lines > > are doing. A for statement is a great way to compress 3 lines of code into 1, > > but you have 5 here, and they're nontrivial. > > > > I would: > > - get rid of iter_end. The savings isn't worth the cost to the reader. > > - declare a variable in the body of the loop to be the second iterator (in > this > > case, iter). That way the for statement only has to concern itself with one > > iterator (in this case, iter_next). What you want is > > iter = iter_next++; > > but it's much clearer to put this in the body of the loop and leave the third > > part of the for statement empty. > > > > (By the way, the comment is out of sync with the code, and I'd say you could > > delete the rest of it after the word "element") > > I'm not sure I understand. If only iter_next is initialized in the for > statement, then I only have it to compare against .end(), so I would miss the > last element. Anyways, I took out iter_end. If it's not clean enough please let > me know! As I mentioned, you can look at the example in alarm_manager.cc:291-293. At the beginning of the loop (rather than the end, as happens if the assignment is in the 3rd part of the for statement) you set iter = iter_next++; Then when iter_next hits the end, you have already seen the last element. (For the record, I noticed that example does set an iterator for end(). So I guess I don't feel as strongly about that. But the advancing of the iterator here is too complex to fit in the for statement, and as you have it here, it's split between the for statement and the first line of the body -- oof.)
Oops, ya, every element will be visited like you described. Sorry.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10656052/3...
On 2012/07/20 23:56:05, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/mitchellwrosen%40chromium.org/10656052... !!!!!!!! Sorry, I didn't see that aboodman was added as a reviewer.
On Fri, Jul 20, 2012 at 5:03 PM, <mitchellwrosen@chromium.org> wrote: > On 2012/07/20 23:56:05, I haz the power (commit-bot) wrote: >> >> CQ is trying da patch. Follow status at > > > https://chromium-status.appspot.com/cq/mitchellwrosen%40chromium.org/10656052... > > !!!!!!!! > > Sorry, I didn't see that aboodman was added as a reviewer. > > http://codereview.chromium.org/10656052/ Most likely what that means is that he replied to the email in the Rietveld web interface, which by default opts you in to "add me as a reviewer". Yoyo Zhou
https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... File chrome/browser/performance_monitor/constants.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... chrome/browser/performance_monitor/constants.cc:11: "Median CPU usage of all running Chrome processes."; See comment in performance_monitor.cc about not using medians here. https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:188: CHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); you can remove the preceding content::'s now. https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:208: base::DoubleToString(GetMedian(cpu_usage_vec))); I think these should be sums, not medians. Medians are good for averaging various readings (e.g. in aggregation over time), but these are essentially all one reading. If we 10 processes, each taking between 1% and 3% of the CPU, then this should give us 20% (the amount of the CPU that chrome is using), not 2%. Thoughts, Yoyo, Aaron, Clint? https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:235: base::Uint64ToString(GetMedian(private_memory_vec))); See above comment. https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:265: #if !defined(OS_MACOSX) nit: This might be clearer as #if defined(OS_MACOSX) <mac behavior> #else <everything else>. https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor.h (right): https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.h:92: // Gathers the CPU usage of every process that has been running since the nit: every Chrome process. https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor_browsertest.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:440: // CPU Usage -- no stats recorded the first time Comments should be grammatically correct (need periods, consider making things like "Private memory usage #2" more complete sentences).
https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:208: base::DoubleToString(GetMedian(cpu_usage_vec))); On 2012/07/23 21:10:33, D Cronin wrote: > I think these should be sums, not medians. Medians are good for averaging > various readings (e.g. in aggregation over time), but these are essentially all > one reading. If we 10 processes, each taking between 1% and 3% of the CPU, then > this should give us 20% (the amount of the CPU that chrome is using), not 2%. > > Thoughts, Yoyo, Aaron, Clint? Agree.
Fixed all the nits but currently the browser test is flaky on Windows. I've run win_rel three times now and it failed the first two, passed the third. Specifically, it fails because GetMemoryBytes returns false, which causes a memory usage of 0 to be recorded, which causes an EXPECT_GT to fail. GetMemoryBytes returns false because GetWorkingSetKBytes returns false at process_util_win.cc:793. Still looking into the problem. Anyone have any ideas? https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... File chrome/browser/performance_monitor/constants.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... chrome/browser/performance_monitor/constants.cc:11: "Median CPU usage of all running Chrome processes."; On 2012/07/23 21:10:33, D Cronin wrote: > See comment in performance_monitor.cc about not using medians here. Done. https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:188: CHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); On 2012/07/23 21:10:33, D Cronin wrote: > you can remove the preceding content::'s now. Done. https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:208: base::DoubleToString(GetMedian(cpu_usage_vec))); On 2012/07/23 21:10:33, D Cronin wrote: > I think these should be sums, not medians. Medians are good for averaging > various readings (e.g. in aggregation over time), but these are essentially all > one reading. If we 10 processes, each taking between 1% and 3% of the CPU, then > this should give us 20% (the amount of the CPU that chrome is using), not 2%. > > Thoughts, Yoyo, Aaron, Clint? Done. https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:235: base::Uint64ToString(GetMedian(private_memory_vec))); On 2012/07/23 21:10:33, D Cronin wrote: > See above comment. Done. https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:265: #if !defined(OS_MACOSX) On 2012/07/23 21:10:33, D Cronin wrote: > nit: This might be clearer as #if defined(OS_MACOSX) <mac behavior> #else > <everything else>. Tomato, tomato. See task_manager.cc:756. Anyways, done. https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor.h (right): https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.h:92: // Gathers the CPU usage of every process that has been running since the On 2012/07/23 21:10:33, D Cronin wrote: > nit: every Chrome process. Done. https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor_browsertest.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/36002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:440: // CPU Usage -- no stats recorded the first time On 2012/07/23 21:10:33, D Cronin wrote: > Comments should be grammatically correct (need periods, consider making things > like "Private memory usage #2" more complete sentences). Done.
https://chromiumcodereview.appspot.com/10656052/diff/39004/base/process_util_... File base/process_util_win.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/39004/base/process_util_... base/process_util_win.cc:735: LOG(ERROR) << "GetMemoryBytes return false"; Debug statements generally don't go in CL's. https://chromiumcodereview.appspot.com/10656052/diff/39004/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/39004/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:7: #include <algorithm> Is this still needed, now that you removed FindMedian()?
https://chromiumcodereview.appspot.com/10656052/diff/39004/base/process_util_... File base/process_util_win.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/39004/base/process_util_... base/process_util_win.cc:735: LOG(ERROR) << "GetMemoryBytes return false"; On 2012/07/24 15:30:48, D Cronin wrote: > Debug statements generally don't go in CL's. I know. It was an accident =P https://chromiumcodereview.appspot.com/10656052/diff/39004/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/39004/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:7: #include <algorithm> On 2012/07/24 15:30:48, D Cronin wrote: > Is this still needed, now that you removed FindMedian()? Nope!
https://chromiumcodereview.appspot.com/10656052/diff/41001/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/41001/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:35: const uint32 access_flags = base::kProcessAccessDuplicateHandle | const uint32 kAccessFlags https://chromiumcodereview.appspot.com/10656052/diff/41001/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:190: void PerformanceMonitor::GatherCPUUsage() { If these are done on the background thread, they should be labeled as such. https://chromiumcodereview.appspot.com/10656052/diff/41001/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor.h (right): https://chromiumcodereview.appspot.com/10656052/diff/41001/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.h:52: void CheckForVersionUpdate(); This doesn't belong here - the method has been replaced (git mistake, most likely?) https://chromiumcodereview.appspot.com/10656052/diff/41001/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.h:103: MetricsMap metrics_map_; Put this with the other data members. https://chromiumcodereview.appspot.com/10656052/diff/41001/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor_browsertest.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/41001/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:36: // Used in PerformanceMonitorBrowserTest.GatherStatistics to consume CPU cycles. Since it's only used in the one test, it can go at the top of the test. https://chromiumcodereview.appspot.com/10656052/diff/41001/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:180: void GetStatsOnBackgroundThread(Database::MetricInfoVector* metrics, Not sure if you noticed, but while doing the merge for you, I used my copy of GetStats/GetStatsOnBackgroundThread (since they were already updated to use the new metrics) - please double check my work and make sure it's all right.
https://chromiumcodereview.appspot.com/10656052/diff/41001/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/41001/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:35: const uint32 access_flags = base::kProcessAccessDuplicateHandle | On 2012/07/27 22:09:47, D Cronin wrote: > const uint32 kAccessFlags Done. https://chromiumcodereview.appspot.com/10656052/diff/41001/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:190: void PerformanceMonitor::GatherCPUUsage() { On 2012/07/27 22:09:47, D Cronin wrote: > If these are done on the background thread, they should be labeled as such. Done. https://chromiumcodereview.appspot.com/10656052/diff/41001/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor.h (right): https://chromiumcodereview.appspot.com/10656052/diff/41001/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.h:52: void CheckForVersionUpdate(); On 2012/07/27 22:09:47, D Cronin wrote: > This doesn't belong here - the method has been replaced (git mistake, most > likely?) Aye, a git mistake. Done. https://chromiumcodereview.appspot.com/10656052/diff/41001/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.h:103: MetricsMap metrics_map_; On 2012/07/27 22:09:47, D Cronin wrote: > Put this with the other data members. Done. https://chromiumcodereview.appspot.com/10656052/diff/41001/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor_browsertest.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/41001/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:36: // Used in PerformanceMonitorBrowserTest.GatherStatistics to consume CPU cycles. On 2012/07/27 22:09:47, D Cronin wrote: > Since it's only used in the one test, it can go at the top of the test. Done. https://chromiumcodereview.appspot.com/10656052/diff/41001/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:180: void GetStatsOnBackgroundThread(Database::MetricInfoVector* metrics, On 2012/07/27 22:09:47, D Cronin wrote: > Not sure if you noticed, but while doing the merge for you, I used my copy of > GetStats/GetStatsOnBackgroundThread (since they were already updated to use the > new metrics) - please double check my work and make sure it's all right. Looks good. Thanks.
https://chromiumcodereview.appspot.com/10656052/diff/48002/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/48002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:199: base::DoubleToString(cpu_usage)); nit: This fits on the previous line, doesn't it? https://chromiumcodereview.appspot.com/10656052/diff/48002/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor_browsertest.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/48002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:19: #include "chrome/browser/ui/tabs/tab_strip_model.h" Sort. https://chromiumcodereview.appspot.com/10656052/diff/48002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:146: void GatherStatistics() { Document. https://chromiumcodereview.appspot.com/10656052/diff/48002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:437: const int kSpinCount = 1000000000; This one's my fault - you're supposed to put this close to where it's used in the test (i.e. around line 456). Sorry about that (this is one of my stylistic problems).
https://chromiumcodereview.appspot.com/10656052/diff/48002/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/48002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:199: base::DoubleToString(cpu_usage)); On 2012/07/31 17:05:13, D Cronin wrote: > nit: This fits on the previous line, doesn't it? Yessir, I didn't notice the new metrics refactor shortened up that first line. Good catch. https://chromiumcodereview.appspot.com/10656052/diff/48002/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor_browsertest.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/48002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:19: #include "chrome/browser/ui/tabs/tab_strip_model.h" On 2012/07/31 17:05:13, D Cronin wrote: > Sort. Done. https://chromiumcodereview.appspot.com/10656052/diff/48002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:146: void GatherStatistics() { On 2012/07/31 17:05:13, D Cronin wrote: > Document. Done. https://chromiumcodereview.appspot.com/10656052/diff/48002/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor_browsertest.cc:437: const int kSpinCount = 1000000000; On 2012/07/31 17:05:13, D Cronin wrote: > This one's my fault - you're supposed to put this close to where it's used in > the test (i.e. around line 456). Sorry about that (this is one of my stylistic > problems). Done.
https://chromiumcodereview.appspot.com/10656052/diff/45003/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/45003/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:35: const uint32 kAccessFlags = base::kProcessAccessDuplicateHandle | Do we need all these accesses? I don't see why we would require permission to, for instance, terminate the process. In fact, that sounds like something we definitely do *not* want to do...Are you these are all necessary?
https://chromiumcodereview.appspot.com/10656052/diff/45003/chrome/browser/per... File chrome/browser/performance_monitor/performance_monitor.cc (right): https://chromiumcodereview.appspot.com/10656052/diff/45003/chrome/browser/per... chrome/browser/performance_monitor/performance_monitor.cc:35: const uint32 kAccessFlags = base::kProcessAccessDuplicateHandle | On 2012/07/31 18:43:20, D Cronin wrote: > Do we need all these accesses? I don't see why we would require permission to, > for instance, terminate the process. In fact, that sounds like something we > definitely do *not* want to do...Are you these are all necessary? Yessir, I ran the test with all possible subsets of the flags since I too thought they weren't all necessary. Started with these plus maybe one or two more. We're trying to appease Windows, here. She is a finicky beast.
LGTM. Might wanna get a quick check with Yoz, since his last was 10 days and 4 revisions ago, but up to you if you think there are significant enough changes to need it.
Still LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10656052/4...
Try job failure for 10656052-45003 (retry) on linux_chromeos for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10656052/5...
Change committed as 149766 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
