Performance monitor stats gathering.

chrome/browser/performance_monitor/performance_monitor.cc

Index: chrome/browser/performance_monitor/performance_monitor.cc
diff --git a/chrome/browser/performance_monitor/performance_monitor.cc b/chrome/browser/performance_monitor/performance_monitor.cc
index 9bacb1e5180b1c581019fccc0dc31c3323f4209e..c89190f4b453dc38c5da8b90a2f7d8777cbe5b85 100644
--- a/chrome/browser/performance_monitor/performance_monitor.cc
+++ b/chrome/browser/performance_monitor/performance_monitor.cc
@@ -4,8 +4,16 @@
 
 #include "chrome/browser/performance_monitor/performance_monitor.h"
 
+#include <algorithm>
+#include <map>
+#include <string>
+#include <vector>
+
 #include "base/bind.h"
 #include "base/logging.h"
+#include "base/process.h"
+#include "base/process_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"
@@ -16,11 +24,27 @@
 #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) {

[Yoyo Zhou, 2012/07/09 21:33:36]

Since you never expect this to be called with size 0, DCHECKing the size is a
good idea.

[mitchellwrosen, 2012/07/09 22:45:23]

Done.

+  typedef typename std::vector<T>::size_type vec_size;
+
+  vec_size size = vec.size();
+  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) {
@@ -88,6 +112,23 @@ void PerformanceMonitor::AddEventOnBackgroundThread(scoped_ptr<Event> event) {
   database_->AddEvent(*event.get());
 }
 
+void PerformanceMonitor::AddMetric(const std::string& key,
+                                   const std::string& value) {
+  content::BrowserThread::PostBlockingPoolSequencedTask(
+      Database::kDatabaseSequenceToken,
+      FROM_HERE,
+      base::Bind(&PerformanceMonitor::AddMetricOnBackgroundThread,
+                 base::Unretained(this),
+                 key,
+                 value));
+}
+
+void PerformanceMonitor::AddMetricOnBackgroundThread(const std::string& key,
+                                                     const std::string& value) {
+  database_->AddMetric(key, value);
+}
+
+
 void PerformanceMonitor::GetStateValueOnBackgroundThread(
     std::string key, base::Callback<void(std::string)> callback) {
   CHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
@@ -139,6 +180,96 @@ void PerformanceMonitor::CheckForVersionUpdateHelper(
   }
 }
 
+void PerformanceMonitor::GatherStatistics() {
+  CHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  GatherCPUUsage();
+  UpdateMetricsMap();

[Yoyo Zhou, 2012/07/09 21:33:36]

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?

[mitchellwrosen, 2012/07/09 22:45:23]

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.

+  GatherMemoryUsage();
+}
+
+void PerformanceMonitor::GatherCPUUsage() {
+  std::vector<double> cpu_usage_vec;
+
+  if (metrics_map_.size() != 0) {

[Yoyo Zhou, 2012/07/09 21:33:36]

nit: you use size() at 227 but size() != 0 here. I don't have a strong
preference, but be consistent.

[mitchellwrosen, 2012/07/09 22:45:23]

Done.

+    for (MetricsMap::const_iterator iter = metrics_map_.begin();
+         iter != metrics_map_.end(); ++iter) {
+      double cpu_usage = iter->second->GetCPUUsage();
+      DCHECK_NE(cpu_usage, 0) << "Gathered CPU usage of 0";
+      cpu_usage_vec.push_back(cpu_usage);
+    }
+
+    std::sort(cpu_usage_vec.begin(), cpu_usage_vec.end());
+    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)";
+    }
+  }
+
+  std::sort(private_memory_vec.begin(), private_memory_vec.end());
+  std::sort(shared_memory_vec.begin(), shared_memory_vec.end());
+
+  if (private_memory_vec.size()) {
+    AddMetric(performance_monitor::kMetricPrivateMemoryUsage,
+              base::Uint64ToString(GetMedian(private_memory_vec)));
+
+    AddMetric(performance_monitor::kMetricSharedMemoryUsage,
+              base::Uint64ToString(GetMedian(shared_memory_vec)));
+  }
+}
+
+void PerformanceMonitor::UpdateMetricsMap() {
+  // Remove old process handles

[Yoyo Zhou, 2012/07/09 21:33:36]

nit: comments should end in .

[mitchellwrosen, 2012/07/09 22:45:23]

Done.

+  for (MetricsMap::iterator iter = metrics_map_.begin();
+       iter != metrics_map_.end(); ++iter) {
+    if (iter->first == base::kNullProcessHandle) {

[Yoyo Zhou, 2012/07/09 21:33:36]

I don't understand how the key of the metrics_map_ is going to change.

[mitchellwrosen, 2012/07/09 22:45:23]

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

[Yoyo Zhou, 2012/07/10 21:58:00]

Seems like you should be checking GetTerminationStatus to see if the process has
exited.

+      // base::CloseProcessHandle(iter->first); // Necessary?

[Yoyo Zhou, 2012/07/09 21:33:36]

This is commented out.

[mitchellwrosen, 2012/07/09 22:45:23]

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

[Yoyo Zhou, 2012/07/10 21:58:00]

I would say "yes, you need to close it" based on the comment on
process_util.h:OpenProcessHandle.

+      metrics_map_.erase(iter);
+    } else {
+      // Prime the CPUUsage to be gathered next time.
+      iter->second->GetCPUUsage();

[Yoyo Zhou, 2012/07/09 21:33:36]

Do you need this? Seems like it should have been primed when you added it to
metrics_map_ the first time.

[mitchellwrosen, 2012/07/09 22:45:23]

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?

[Yoyo Zhou, 2012/07/10 21:58:00]

Either seems fine.

+    }
+  }
+
+  // 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)) {
+      linked_ptr<base::ProcessMetrics> process_metrics(
+#if !defined(OS_MACOSX)
+          base::ProcessMetrics::CreateProcessMetrics(process_handle));
+#else
+          base::ProcessMetrics::CreateProcessMetrics(process_handle,
+              content::BrowserChildProcessHost::GetPortProvider()));
+#endif
+      if (metrics_map_.find(process_handle) == metrics_map_.end()) {

[Yoyo Zhou, 2012/07/09 21:33:36]

You can use ContainsKey from stl_util.h.

[mitchellwrosen, 2012/07/09 22:45:23]

Done.

+        // 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) {