Display histograms from subprocesses in chrome://histograms

chrome/browser/metrics/subprocess_metrics_provider.h

 // Copyright 2016 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.
 
 #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"
 #include "content/public/browser/render_process_host_observer.h"
 
 namespace base {
 class PersistentHistogramAllocator;
 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;
 
  private:
   friend class SubprocessMetricsProviderTest;
 
   // Indicates subprocess to be monitored with unique id for later reference.
   // Metrics reporting will read histograms from it and upload them to UMA.
   void RegisterSubprocessAllocator(
       int id,
       std::unique_ptr<base::PersistentHistogramAllocator> allocator);
 
   // Indicates that a subprocess has exited and is thus finished with the
   // 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);
 
   // metrics::MetricsProvider:
   void OnDidCreateMetricsLog() override;
   void OnRecordingEnabled() override;
   void OnRecordingDisabled() override;
   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,
                const content::NotificationDetails& details) override;
 
   // content::RenderProcessHostObserver:
   void RenderProcessReady(content::RenderProcessHost* host) override;
   void RenderProcessExited(content::RenderProcessHost* host,
                            base::TerminationStatus status,
                            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
   // once the last data contained therein has been reported.
   std::vector<std::unique_ptr<base::PersistentHistogramAllocator>>
       allocators_for_exited_processes_;
   std::vector<std::unique_ptr<base::PersistentHistogramAllocator>>
       allocators_to_release_;
 
   // Track all observed render processes to un-observe them on exit.
   ScopedObserver<content::RenderProcessHost, SubprocessMetricsProvider>
       scoped_observer_;
 
   // Flag indicating if metrics recording is enabled. Allocators will not
   // live past the death of the subprocess if it is not.
   bool metrics_recording_enabled_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(SubprocessMetricsProvider);
 };
 
 #endif  // CHROME_BROWSER_METRICS_SUBPROCESS_METRICS_PROVIDER_H_