Performance monitor stats gathering.

chrome/browser/performance_monitor/performance_monitor.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/performance_monitor/performance_monitor.h"
 
+#include <algorithm>
+#include <map>

[Devlin, 2012/07/17 18:07:58]

map and string are included in the .h file; you can remove them here.

[mitchellwrosen, 2012/07/20 19:42:36]

Isn't there a style rule against relying on transitive includes?

[Aaron Boodman, 2012/07/20 21:07:05]

The rule seems to be special cased to omit a .cc file's "own" .h file. :|

+#include <string>
+#include <vector>
+
 #include "base/bind.h"
 #include "base/logging.h"
+#include "base/memory/linked_ptr.h"
+#include "base/process.h"

[Devlin, 2012/07/17 18:07:58]

linked_ptr, process, process_util already included.

+#include "base/process_util.h"
+#include "base/stl_util.h"
+#include "base/string_number_conversions.h"
 #include "base/threading/worker_pool.h"
 #include "base/time.h"
 #include "chrome/browser/performance_monitor/constants.h"
 #include "chrome/browser/performance_monitor/database.h"
 #include "chrome/browser/performance_monitor/performance_monitor_util.h"
 #include "chrome/browser/extensions/crx_installer.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_constants.h"
+#include "chrome/test/base/chrome_process_util.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
 
 using extensions::Extension;
 
+namespace {
+
+template <class T>
+T GetMedian(std::vector<T> vec) {
+  typedef typename std::vector<T>::size_type vec_size;
+
+  vec_size size = vec.size();
+  DCHECK(size != 0);
+  std::sort(vec.begin(), vec.end());
+  vec_size mid = size / 2;
+
+  return size % 2 == 0 ? (vec[mid] + vec[mid - 1]) / 2 : vec[mid];
+}
+
+}  // namespace
+
 namespace performance_monitor {
 
 PerformanceMonitor::PerformanceMonitor() : database_(NULL) {
 }
 
 PerformanceMonitor::~PerformanceMonitor() {
 }
 
 void PerformanceMonitor::Start() {
   content::BrowserThread::PostBlockingPoolTaskAndReply(
@@ skipping 103 matching lines @@
                    current_version));
     if (!previous_version.empty()) {
       AddEvent(util::CreateChromeUpdateEvent(
           base::Time::Now(),
           previous_version,
           current_version));
     }
   }
 }
 
+void PerformanceMonitor::GatherStatisticsOnBackgroundThread() {
+  CHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  // Because the CPU usage is gathered as an average since the last time the
+  // function was called, while the memory usage is gathered as an instantaneous
+  // usage, the CPU usage is gathered before the metrics map is wiped.
+  GatherCPUUsage();
+  UpdateMetricsMap();
+  GatherMemoryUsage();
+}
+
+void PerformanceMonitor::GatherCPUUsage() {
+  std::vector<double> cpu_usage_vec;
+
+  if (metrics_map_.size()) {
+    MetricsMap::const_iterator iter = metrics_map_.begin();
+    for (; iter != metrics_map_.end(); ++iter)

[Devlin, 2012/07/17 18:07:58]

Why not declare iter here, as normal?

[mitchellwrosen, 2012/07/20 19:42:36]

Done.

+      cpu_usage_vec.push_back(iter->second->GetCPUUsage());
+
+    std::sort(cpu_usage_vec.begin(), cpu_usage_vec.end());
+    database_->AddMetric(performance_monitor::kMetricCPUUsage,
+                         base::DoubleToString(GetMedian(cpu_usage_vec)));
+  }
+}
+
+void PerformanceMonitor::GatherMemoryUsage() {
+  std::vector<size_t> private_memory_vec;
+  std::vector<size_t> shared_memory_vec;
+
+  size_t private_memory = 0;
+  size_t shared_memory = 0;
+  for (MetricsMap::const_iterator iter = metrics_map_.begin();
+       iter != metrics_map_.end(); ++iter) {
+    if (iter->second->GetMemoryBytes(&private_memory, &shared_memory)) {
+      private_memory_vec.push_back(private_memory);
+      shared_memory_vec.push_back(shared_memory);
+    } else {
+      LOG(ERROR) << "GetMemoryBytes returned NULL (platform-specific error)";
+    }
+  }
+
+  if (!private_memory_vec.size())
+    return;
+
+  std::sort(private_memory_vec.begin(), private_memory_vec.end());
+  std::sort(shared_memory_vec.begin(), shared_memory_vec.end());
+
+  database_->AddMetric(performance_monitor::kMetricPrivateMemoryUsage,
+                       base::Uint64ToString(GetMedian(private_memory_vec)));
+  database_->AddMetric(performance_monitor::kMetricSharedMemoryUsage,
+                       base::Uint64ToString(GetMedian(shared_memory_vec)));
+}
+
+void PerformanceMonitor::UpdateMetricsMap() {
+  // Remove old process handles. Use two iteraterors to safely call erase()

[Yoyo Zhou, 2012/07/19 21:35:35]

typo: iterators

[mitchellwrosen, 2012/07/20 19:42:36]

Done.

+  // on the current element, and advance by a call to iter = iter_next++.

[Devlin, 2012/07/17 18:07:58]

style violation - always use ++i, not i++

[Yoyo Zhou, 2012/07/19 21:35:35]

It's fine if you're assigning the old value of i.

+  // The third iterator saves us from calling end() every loop.
+  for (MetricsMap::iterator iter_next = metrics_map_.begin(),
+           iter_end = metrics_map_.end(),
+           iter = (iter_next == iter_end) ? iter_next : iter_next++;

[Yoyo Zhou, 2012/07/19 21:35:35]

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.)

[mitchellwrosen, 2012/07/20 19:42:36]

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

[Yoyo Zhou, 2012/07/20 20:47:30]

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")

[mitchellwrosen, 2012/07/20 23:05:27]

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!

[Yoyo Zhou, 2012/07/20 23:14:26]

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.)

+       iter != iter_next;
+       iter = (iter_next == iter_end) ? iter_next : iter_next++) {
+    if (base::GetTerminationStatus(iter->first, NULL) !=
+        base::TERMINATION_STATUS_STILL_RUNNING) {
+      base::CloseProcessHandle(iter->first);
+      metrics_map_.erase(iter);
+    } else {
+      // Prime the CPUUsage to be gathered next time.
+      iter->second->GetCPUUsage();
+    }
+  }
+
+  // Add new process handles.
+  base::ProcessId browser_pid = base::GetCurrentProcId();
+  ChromeProcessList chrome_processes(GetRunningChromeProcesses(browser_pid));
+  for (ChromeProcessList::const_iterator pid_iter = chrome_processes.begin();
+       pid_iter != chrome_processes.end(); ++pid_iter) {
+    base::ProcessHandle process_handle;
+    if (base::OpenProcessHandle(*pid_iter, &process_handle)) {
+#if !defined(OS_MACOSX)
+      linked_ptr<base::ProcessMetrics> process_metrics(
+          base::ProcessMetrics::CreateProcessMetrics(process_handle));
+#else
+      linked_ptr<base::ProcessMetrics> process_metrics(
+          base::ProcessMetrics::CreateProcessMetrics(process_handle,
+              content::BrowserChildProcessHost::GetPortProvider()));
+#endif
+      if (!ContainsKey(metrics_map_, process_handle)) {
+        // Prime the CPUUsage to be gathered next time.
+        process_metrics->GetCPUUsage();
+
+        metrics_map_[process_handle] = process_metrics;
+      }
+    }
+  }
+}
+
 void PerformanceMonitor::Observe(int type,
                                  const content::NotificationSource& source,
                                  const content::NotificationDetails& details) {
   switch (type) {
     case chrome::NOTIFICATION_EXTENSION_INSTALLED: {
       const Extension* extension = content::Details<Extension>(details).ptr();
       AddEvent(util::CreateExtensionInstallEvent(base::Time::Now(),
                                                  extension->id(),
                                                  extension->name(),
                                                  extension->url().spec(),
@@ skipping 46 matching lines @@
       break;
     }
     default: {
       NOTREACHED();
       break;
     }
   }
 }
 
 }  // namespace performance_monitor