Display histograms from subprocesses in chrome://histograms

chrome/browser/metrics/subprocess_metrics_provider.h

Index: chrome/browser/metrics/subprocess_metrics_provider.h
diff --git a/chrome/browser/metrics/subprocess_metrics_provider.h b/chrome/browser/metrics/subprocess_metrics_provider.h
index d47f5846a3d7438a6aebfbbd1ea8c3d9db98788e..2690aca2766bbbd8e96bee13f72089a72b87461f 100644
--- a/chrome/browser/metrics/subprocess_metrics_provider.h
+++ b/chrome/browser/metrics/subprocess_metrics_provider.h
@@ -5,13 +5,14 @@
 #ifndef CHROME_BROWSER_METRICS_SUBPROCESS_METRICS_PROVIDER_H_
 #define CHROME_BROWSER_METRICS_SUBPROCESS_METRICS_PROVIDER_H_
 
+#include <map>
 #include <memory>
-#include <set>
+#include <vector>
 
 #include "base/gtest_prod_util.h"
-#include "base/id_map.h"
+#include "base/metrics/statistics_recorder.h"
 #include "base/scoped_observer.h"
-#include "base/threading/thread_checker.h"
+#include "base/synchronization/lock.h"

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

On the subject of locks: I've now researched how the HTML display code is run,
and I understand why it's happening on a background thread.  I don't think the
performance benefit is very important, honestly -- this is code that's only run
by developers, and even then only rarely, and I doubt it's actually *super*
expensive -- the chrome://histograms page renders within O(100ms) for me.  I
think the primary reason this all runs on a background thread is just the
convenience of architecting the code that way -- we use a //net class that just
assigns tasks to worker pool threads.

I don't think it's worth introducing a lock to support this functionality. 
Locks make the code more complicated, even for the much more common case of of
actually uploading metrics data.  I'd much rather push that complexity down into
just the HTML displaying code.  Specifically, I'd suggest either moving the work
to happen on the main thread, or writing the code to run asynchronously,
probably whichever is simpler to implement.

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

I'll look into it.  There are two calls, one from Desktop and one from iOS.  I
don't think I can post a callback to the UI thread from base/ because, correct
me if I'm wrong, it doesn't know about threads that way.

May need some help on that front.

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

Which base/ code are you thinking of, specifically?

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

base/metrics/statistics_recorder, which is the common entry point.  Ideally, it
would switch to the UI thread, blocking any background thread until done, so
that callers wouldn't need to know about it.

With multiple callers, all (okay, both) of the will have to do the thread-jump
to call SR::WriteHTMLGraphs().  I've seen a send-and-wait-reply in previous
work; I'll investigate.

 #include "components/metrics/metrics_provider.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
@@ -24,9 +25,11 @@ class SharedPersistentMemoryAllocator;
 
 // SubprocessMetricsProvider gathers and logs histograms stored in shared
 // memory segments between processes.
-class SubprocessMetricsProvider : public metrics::MetricsProvider,
-                                  public content::NotificationObserver,
-                                  public content::RenderProcessHostObserver {
+class SubprocessMetricsProvider
+    : public metrics::MetricsProvider,
+      public base::StatisticsRecorder::MetricsDisplayer,
+      public content::NotificationObserver,
+      public content::RenderProcessHostObserver {
  public:
   SubprocessMetricsProvider();
   ~SubprocessMetricsProvider() override;
@@ -44,9 +47,15 @@ class SubprocessMetricsProvider : public metrics::MetricsProvider,
   // allocator it was using.
   void DeregisterSubprocessAllocator(int id);
 
-  // Report all histograms of a given allocator to the snapshot-manager.
+  // Report histograms of a given allocator to the snapshot-manager. If
+  // |query| is not empty, only histograms that contain that string will
+  // be recorded. If |absolute| is true, no delta from the previous snapshot
+  // will be done. Only one caller, the caller that uploads to UMA, is
+  // allowed to calcuate deltas because they are not independent.
   void RecordHistogramSnapshotsFromAllocator(
       base::HistogramSnapshotManager* snapshot_manager,
+      const std::string& query,
+      bool absolute,
       int id,
       base::PersistentHistogramAllocator* allocator);
 
@@ -57,6 +66,12 @@ class SubprocessMetricsProvider : public metrics::MetricsProvider,
   void RecordHistogramSnapshots(
       base::HistogramSnapshotManager* snapshot_manager) override;
 
+  // base::StatisticsRecorder::MetricsDisplay:

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

nit: displayer

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

Done.

+  void WriteTitleString(std::string* output) override;
+  void WriteGraphs(const std::string& query,
+                   DisplayType format,
+                   std::string* output) override;
+
   // content::NotificationObserver:
   void Observe(int type,
                const content::NotificationSource& source,
@@ -69,14 +84,16 @@ class SubprocessMetricsProvider : public metrics::MetricsProvider,
                            int exit_code) override;
   void RenderProcessHostDestroyed(content::RenderProcessHost* host) override;
 
-  base::ThreadChecker thread_checker_;
+  // Lock for accessing objects. This is required because MetricsDisplay is

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

nit: displayer

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

Done.

+  // typically called by a background thread.
+  base::Lock lock_;
 
   // Object for registing notification requests.
   content::NotificationRegistrar registrar_;
 
   // All of the shared-persistent-allocators for known sub-processes.
   using AllocatorByIdMap =
-      IDMap<base::PersistentHistogramAllocator, IDMapOwnPointer, int>;
+      std::map<int, std::unique_ptr<base::PersistentHistogramAllocator>>;

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

This change seems unrelated to the rest of the CL.  Or am I overlooking the
connection?

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

IDmap allows access only from a single thread.  I used it originally because
maps of std::scoped_ptr weren't supported.

   AllocatorByIdMap allocators_by_id_;
 
   // Allocators that are no longer attached to a subprocess, to be released