Display histograms from subprocesses in chrome://histograms

chrome/browser/metrics/subprocess_metrics_provider.cc

Index: chrome/browser/metrics/subprocess_metrics_provider.cc
diff --git a/chrome/browser/metrics/subprocess_metrics_provider.cc b/chrome/browser/metrics/subprocess_metrics_provider.cc
index ebefe4492c7403c85540eb610a943e021c330a14..f0c86604360e7e0b1d7d4fd7b871e8eaeb3c371a 100644
--- a/chrome/browser/metrics/subprocess_metrics_provider.cc
+++ b/chrome/browser/metrics/subprocess_metrics_provider.cc
@@ -7,78 +7,132 @@
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_base.h"
+#include "base/metrics/histogram_flattener.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/metrics/histogram_samples.h"
 #include "base/metrics/persistent_histogram_allocator.h"
 #include "base/metrics/persistent_memory_allocator.h"
+#include "base/metrics/statistics_recorder.h"

[Ilya Sherman, 2016/04/25 20:53:14]

nit: Already included in the header file.

[bcwhite, 2016/04/26 19:51:54]

Done.

+#include "base/stl_util.h"
+#include "base/strings/stringprintf.h"
 #include "components/metrics/metrics_service.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_process_host.h"
 
+namespace {
+
+class HistogramOutputFlattener : public base::HistogramFlattener {
+ public:
+  explicit HistogramOutputFlattener(
+      base::StatisticsRecorder::MetricsDisplayer::DisplayType format)
+      : format_(format) {}
+
+  void RecordDelta(const base::HistogramBase& histogram,
+                   const base::HistogramSamples& snapshot) override {
+    std::string& graph = graphs_[histogram.histogram_name()];

[Ilya Sherman, 2016/04/25 20:53:14]

Hmm.  Suppose that multiple subprocesses recorded histograms with the same name
(which is a fairly common case).  Do the histograms get aggregated together, or
duplicated for each renderer?  It seems like the latter, which is quite odd.  Or
are they aggregated together somewhere prior to reaching this code?

[bcwhite, 2016/04/26 19:51:54]

The HistogramSnapshotManager merges histograms with duplicate names.

[Ilya Sherman, 2016/04/26 20:21:58]

I see.  It's kind of wacky that |histogram| mostly describes the histogram, but
might have different samples in it than the |snapshot| does.  But, I guess this
is how the code has worked for ages.  Oh well.  Thanks for the explanation.

[bcwhite, 2016/04/26 21:03:29]

Actually, the merging in the HSM is new.  It used to be that everything was
merged by the StatisticsRecorder beacuse all remote histograms were imported
completely via RPC.  With the move to persistent storage, importing them becomes
unnecessary since the remote memory is directly accessible.  So, no merged into
the SR but merge in the HSM.

In hindsight, it may have been better to continue the import of everything into
the SR and just used the persistent memory as a replacement for the RPC.  There
are advantages and disadvantages with both methods, I think.

[Ilya Sherman, 2016/04/27 20:17:42]

Hmm.  Might be worth discussing when we meet tomorrow.

Assuming we keep the merging approach: Since the merging concept is new, I
wonder if it wouldn't make more sense to create a new histogram, which has the
correct snapshot within it, rather than tracking the snapshot externally.

[bcwhite, 2016/05/02 13:20:34]

There's no easy way to create a new histogram based on another which is
necessary in order to have the bucketing information.

You have to "pickle" then "unpickle" it to something new which is quite
expensive.  This would have to be done once per histogram name during every HSM
run (not just those for display).

[Ilya Sherman, 2016/05/04 06:00:26]

Sorry, I'm still confused here.  Are you saying that it would be hard to write a
direct copy constructor?  If so, how come?  The expensive part of the copy is
copying the snapshot... but that's the part that's being copied anyway.  (And,
we wouldn't need to copy most histograms -- just ones whose names appear
multiple times, i.e. those defined in renderer processes.)

[bcwhite, 2016/05/04 15:38:08]

It wouldn't be too difficult.  A virtual Duplicate() method that returns a new
(heap) HistogramBase object would sit beside Snapshot().

However, I think this is a bad idea as it would create multiple histogram
objects of the same name when care has been taken to ensure that there is only
ever one.

+    switch (format_) {
+      case base::StatisticsRecorder::MetricsDisplayer::DISPLAY_TEXT_PLAIN:
+        histogram.WriteAscii(&snapshot, &graph);
+        graph.append("\n");
+        break;
+      case base::StatisticsRecorder::MetricsDisplayer::DISPLAY_TEXT_HTML:
+        histogram.WriteHTMLGraph(&snapshot, &graph);

[Ilya Sherman, 2016/04/25 20:53:15]

So, why do you need to pass the snapshot here?  How does it differ from the
internal snapshot that the histogram has?

[bcwhite, 2016/04/26 19:51:54]

The snapshot passed in is merged from all those with the same name.

+        graph.append("<br><hr><br>");
+        break;
+    }
+  }
+
+  void InconsistencyDetected(
+      base::HistogramBase::Inconsistency problem) override {}
+

[Ilya Sherman, 2016/04/25 20:53:15]

nit: Please remove this blank line.

[bcwhite, 2016/04/26 19:51:54]

Done.

+  void UniqueInconsistencyDetected(
+      base::HistogramBase::Inconsistency problem) override {}
+

[Ilya Sherman, 2016/04/25 20:53:15]

nit: Please remove this blank line.

[bcwhite, 2016/04/26 19:51:54]

Done.

+  void InconsistencyDetectedInLoggedCount(int amount) override {}
+
+  void WriteOutput(std::string* output) {
+    for (auto& name_and_graph : graphs_) {

[Ilya Sherman, 2016/04/25 20:53:15]

nit: const auto&?

[bcwhite, 2016/04/26 19:51:53]

Done.

+      *output += name_and_graph.second;
+    }
+  }
+
+ private:
+  base::StatisticsRecorder::MetricsDisplayer::DisplayType format_;

[Ilya Sherman, 2016/04/25 20:53:15]

nit: const?

[bcwhite, 2016/04/26 19:51:54]

Done.

+
+  // Map of histogram names to histogram output. This is used to have the
+  // display appear in alphabetical order.
+  std::map<const std::string, std::string> graphs_;
+
+  DISALLOW_COPY_AND_ASSIGN(HistogramOutputFlattener);
+};
+
+}  // namespace
+
 SubprocessMetricsProvider::SubprocessMetricsProvider()
     : scoped_observer_(this) {
+  base::StatisticsRecorder::RegisterMetricsDisplayer(this);
   registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_CREATED,
                  content::NotificationService::AllBrowserContextsAndSources());
 }
 
-SubprocessMetricsProvider::~SubprocessMetricsProvider() {}
+SubprocessMetricsProvider::~SubprocessMetricsProvider() {
+  base::StatisticsRecorder::DeregisterMetricsDisplayer(this);
+}
 
 void SubprocessMetricsProvider::RegisterSubprocessAllocator(
     int id,
     std::unique_ptr<base::PersistentHistogramAllocator> allocator) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(!allocators_by_id_.Lookup(id));
+  DCHECK(!ContainsKey(allocators_by_id_, id));
 
-  // Map is "MapOwnPointer" so transfer ownership to it.
-  allocators_by_id_.AddWithID(allocator.release(), id);
+  base::AutoLock lock(lock_);
+
+  // Insert has to be done with a pair in order to support "move" semantics.
+  allocators_by_id_.insert(std::make_pair(id, std::move(allocator)));
 }
 
 void SubprocessMetricsProvider::DeregisterSubprocessAllocator(int id) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  base::AutoLock lock(lock_);
 
-  if (!allocators_by_id_.Lookup(id))
+  auto existing = allocators_by_id_.find(id);
+  if (existing == allocators_by_id_.end())
     return;
 
-  // Extract the matching allocator from the list of active ones.
-  std::unique_ptr<base::PersistentHistogramAllocator> allocator(
-      allocators_by_id_.Replace(id, nullptr));
-  allocators_by_id_.Remove(id);
-  DCHECK(allocator);
-
   // If metrics recording is enabled, transfer the allocator to the "release"
   // list. The allocator will continue to live (and keep the associated shared
   // memory alive) until the next upload after which it will be released.
   // Otherwise, the allocator and its memory will be released when the
   // unique_ptr goes out of scope at the end of this method.
   if (metrics_recording_enabled_)
-    allocators_for_exited_processes_.push_back(std::move(allocator));
+    allocators_for_exited_processes_.push_back(std::move(existing->second));
+
+  allocators_by_id_.erase(existing);
 }
 
 void SubprocessMetricsProvider::OnDidCreateMetricsLog() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
   // The previous reporting cycle is complete and the data used to create it
   // will never be needed again. Allocators for exited processes can finally
   // be released.
+  base::AutoLock lock(lock_);
   allocators_to_release_.clear();
 }
 
 void SubprocessMetricsProvider::OnRecordingEnabled() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
   metrics_recording_enabled_ = true;
 }
 
 void SubprocessMetricsProvider::OnRecordingDisabled() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
   metrics_recording_enabled_ = false;
+
+  base::AutoLock lock(lock_);
   allocators_for_exited_processes_.clear();
   allocators_to_release_.clear();
 }
 
 void SubprocessMetricsProvider::RecordHistogramSnapshotsFromAllocator(
     base::HistogramSnapshotManager* snapshot_manager,
+    const std::string& query,
+    bool absolute,

[Ilya Sherman, 2016/04/25 20:53:15]

nit: The semantics of |absolute| are pretty hard to follow at the callsite. 
Would be nice to use an enum instead... or better yet, IMO, to just implement a
separate method.  There's not a whole lot of shared code, and some of the code
needed for the delta case isn't really necessary for writing the HTML versions
(and vice-versa).

[bcwhite, 2016/04/26 19:51:53]

I didn't bother with an enum because it's all private code but Done.

I'd rather not duplicate the code.

[Ilya Sherman, 2016/04/26 20:21:58]

I think you're overestimating the amount of duplicated code, but I guess I'm
okay with either approach.

[bcwhite, 2016/04/26 21:03:29]

Acknowledged.

     int id,
     base::PersistentHistogramAllocator* allocator) {
   DCHECK(allocator);
@@ -89,7 +143,13 @@ void SubprocessMetricsProvider::RecordHistogramSnapshotsFromAllocator(
     std::unique_ptr<base::HistogramBase> histogram = hist_iter.GetNext();
     if (!histogram)
       break;
-    snapshot_manager->PrepareDeltaTakingOwnership(std::move(histogram));
+    if (histogram->histogram_name().find(query) == std::string::npos)
+      continue;
+
+    if (absolute)
+      snapshot_manager->PrepareAbsoluteTakingOwnership(std::move(histogram));
+    else
+      snapshot_manager->PrepareDeltaTakingOwnership(std::move(histogram));
     ++histogram_count;
   }
 
@@ -99,16 +159,18 @@ void SubprocessMetricsProvider::RecordHistogramSnapshotsFromAllocator(
 
 void SubprocessMetricsProvider::RecordHistogramSnapshots(
     base::HistogramSnapshotManager* snapshot_manager) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  base::AutoLock lock(lock_);
 
-  for (AllocatorByIdMap::iterator iter(&allocators_by_id_); !iter.IsAtEnd();
-       iter.Advance()) {
-    RecordHistogramSnapshotsFromAllocator(
-        snapshot_manager, iter.GetCurrentKey(), iter.GetCurrentValue());
+  for (auto& id_and_allocator : allocators_by_id_) {
+    RecordHistogramSnapshotsFromAllocator(snapshot_manager, std::string(),
+                                          false, id_and_allocator.first,
+                                          id_and_allocator.second.get());
   }
 
-  for (auto& allocator : allocators_for_exited_processes_)
-    RecordHistogramSnapshotsFromAllocator(snapshot_manager, 0, allocator.get());
+  for (auto& allocator : allocators_for_exited_processes_) {
+    RecordHistogramSnapshotsFromAllocator(snapshot_manager, std::string(),
+                                          false, 0, allocator.get());
+  }
 
   UMA_HISTOGRAM_COUNTS_100(
       "UMA.SubprocessMetricsProvider.SubprocessCount",
@@ -122,13 +184,57 @@ void SubprocessMetricsProvider::RecordHistogramSnapshots(
   allocators_to_release_.swap(allocators_for_exited_processes_);
 }
 
+void SubprocessMetricsProvider::WriteTitleString(std::string* output) {
+  base::AutoLock lock(lock_);
+  base::StringAppendF(
+      output,
+      "Histograms belonging to %d currently active and %d recently exited "
+      "sub-processes (merged). Data from older processes are not included.",
+      static_cast<int>(allocators_by_id_.size()),
+      static_cast<int>(allocators_to_release_.size()));
+  if (!metrics_recording_enabled_) {
+    output->append(" UMA reporting is not currently enabled so data from"
+                   " processes that have exited are not kept at all.");
+  }
+}
+
+void SubprocessMetricsProvider::WriteGraphs(const std::string& query,
+                                            DisplayType format,
+                                            std::string* output) {
+  HistogramOutputFlattener flattener(format);
+  base::HistogramSnapshotManager snapshot_manager(&flattener);
+  snapshot_manager.StartDeltas();
+
+  // Only the access to the allocators needs to be locked. Once their data
+  // has been recorded, the lock can be released for the processing of the
+  // merged data.
+  {
+    base::AutoLock lock(lock_);
+
+    for (auto& id_and_allocator : allocators_by_id_) {

[Ilya Sherman, 2016/04/25 20:53:15]

nit: const auto?

[bcwhite, 2016/04/26 19:51:54]

Done.

+      RecordHistogramSnapshotsFromAllocator(&snapshot_manager, query, true,
+                                            id_and_allocator.first,
+                                            id_and_allocator.second.get());
+    }
+
+    for (auto& allocator : allocators_to_release_) {

[Ilya Sherman, 2016/04/25 20:53:15]

nit: const auto?

[bcwhite, 2016/04/26 19:51:54]

Done.

+      RecordHistogramSnapshotsFromAllocator(&snapshot_manager, query, true,
+                                            0, allocator.get());
+    }
+  }
+
+  snapshot_manager.FinishDeltas();
+  flattener.WriteOutput(output);
+}
+
 void SubprocessMetricsProvider::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
-  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_EQ(content::NOTIFICATION_RENDERER_PROCESS_CREATED, type);
 
+  base::AutoLock lock(lock_);
+
   content::RenderProcessHost* host =
       content::Source<content::RenderProcessHost>(source).ptr();
 
@@ -140,8 +246,6 @@ void SubprocessMetricsProvider::Observe(
 
 void SubprocessMetricsProvider::RenderProcessReady(
     content::RenderProcessHost* host) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
   // If the render-process-host passed a persistent-memory-allocator to the
   // renderer process, extract it and register it here.
   std::unique_ptr<base::SharedPersistentMemoryAllocator> allocator =
@@ -158,19 +262,16 @@ void SubprocessMetricsProvider::RenderProcessExited(
     content::RenderProcessHost* host,
     base::TerminationStatus status,
     int exit_code) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
   DeregisterSubprocessAllocator(host->GetID());
 }
 
 void SubprocessMetricsProvider::RenderProcessHostDestroyed(
     content::RenderProcessHost* host) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
   // It's possible for a Renderer to terminate without RenderProcessExited
   // (above) being called so it's necessary to de-register also upon the
   // destruction of the host. If both get called, no harm is done.
-
   DeregisterSubprocessAllocator(host->GetID());
+
+  base::AutoLock lock(lock_);
   scoped_observer_.Remove(host);
 }