Support saving browser metrics to disk and reading them during next run.

base/metrics/persistent_histogram_allocator.cc

Index: base/metrics/persistent_histogram_allocator.cc
diff --git a/base/metrics/persistent_histogram_allocator.cc b/base/metrics/persistent_histogram_allocator.cc
index 62e8cd6d68baa4ca567e97f3c095ce093969eb1c..fe3a2059ba21d96a9716915d9eac70cf3c276864 100644
--- a/base/metrics/persistent_histogram_allocator.cc
+++ b/base/metrics/persistent_histogram_allocator.cc
@@ -6,6 +6,7 @@
 
 #include <memory>
 
+#include "base/files/important_file_writer.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
@@ -46,6 +47,7 @@ enum : uint32_t {
 // anything essential at exit anyway due to the fact that they depend on data
 // managed elsewhere and which could be destructed first.
 GlobalHistogramAllocator* g_allocator;
+bool g_allocator_enabled;

[Ilya Sherman, 2016/04/27 20:31:33]

Sorry, I'm going to go back to my previous question about this: Why wouldn't it
make sense to track internally to the GlobalHistogramAllocator object?  You
mentioned that you'd need to plumb it through to the base class, but I don't
actually follow why that would be necessary.

[bcwhite, 2016/05/02 14:46:47]

It would have to be tested by CreateHistogram() which is in the base class and
have it fail if not enabled.  Or the method would have to be overridden here,
which would also mean it should probably be virtual.

Code calling this, however, doesn't expect CreateHistogram to fail unless there
is a problem, such as corrupted or full.  Calling code does, however, expect
Get() to return null if it's not enabled so by testing g_enabled in Get(), all
the operational semantics remain the same.

[Ilya Sherman, 2016/05/05 22:22:52]

g_allocator_enabled is only accessed internally to this file, where every access
could be replaced by an access to g_allocator->enabled_.  What am I missing? 
(And, sorry, I thought I'd replied to this previously, but I guess I missed it.)

[bcwhite, 2016/05/06 16:59:27]

That would work just fine.  The difference would be that the static
Enable/Disable methods would now have to go inside the global g_allocator
singleton to adjust the flag.  The separation has been that static methods
operate on the existence of the singleton but don't go inside it.

Or, those methods would have to become non-static and manipulating them would
then become GetEvenIfDisabled()->Enable()...  which just looks bad to me.

 
 // Take an array of range boundaries and create a proper BucketRanges object
 // which is returned to the caller. A return of nullptr indicates that the
@@ -651,6 +653,18 @@ void GlobalHistogramAllocator::CreateWithSharedMemoryHandle(
 }
 
 // static
+void GlobalHistogramAllocator::Enable() {
+  DCHECK(g_allocator);
+  g_allocator_enabled = true;
+}
+
+// static
+void GlobalHistogramAllocator::Disable() {
+  DCHECK(g_allocator);
+  g_allocator_enabled = false;
+}
+
+// static
 void GlobalHistogramAllocator::Set(
     std::unique_ptr<GlobalHistogramAllocator> allocator) {
   // Releasing or changing an allocator is extremely dangerous because it
@@ -658,6 +672,7 @@ void GlobalHistogramAllocator::Set(
   // also released, future accesses to those histograms will seg-fault.
   CHECK(!g_allocator);
   g_allocator = allocator.release();
+  g_allocator_enabled = true;
   size_t existing = StatisticsRecorder::GetHistogramCount();
 
   DLOG_IF(WARNING, existing)
@@ -666,6 +681,11 @@ void GlobalHistogramAllocator::Set(
 
 // static
 GlobalHistogramAllocator* GlobalHistogramAllocator::Get() {
+  return g_allocator_enabled ? g_allocator : nullptr;
+}
+
+// static
+GlobalHistogramAllocator* GlobalHistogramAllocator::GetEvenIfDisabled() {
   return g_allocator;
 }
 
@@ -704,6 +724,35 @@ GlobalHistogramAllocator::ReleaseForTesting() {
   return WrapUnique(histogram_allocator);
 };
 
+void GlobalHistogramAllocator::SetPersistentLocation(const FilePath& location) {
+  persistent_location_ = location;
+}
+
+bool GlobalHistogramAllocator::WriteToPersistentLocation() {
+#if defined(OS_NACL)
+  // NACL doesn't support file operations, including ImportantFileWriter.
+  NOTREACHED();
+  return false;
+#else

[Ilya Sherman, 2016/04/27 20:31:33]

Please DCHECK(g_allocator_enabled) in this code.

[bcwhite, 2016/05/02 14:46:47]

Done.

+  // Stop if no destination is set, perhaps because it has not been enabled.
+  if (persistent_location_.empty()) {
+    NOTREACHED() << "Could not write \"" << Name() << "\" persistent histograms"
+                 << " to file because no location was set.";
+    return false;
+  }
+
+  StringPiece contents(static_cast<const char*>(data()), used());
+  if (!ImportantFileWriter::WriteFileAtomically(persistent_location_,
+                                                contents)) {
+    LOG(ERROR) << "Could not write \"" << Name() << "\" persistent histograms"
+               << " to file: " << persistent_location_.value();
+    return false;
+  }
+
+  return true;
+#endif
+}
+
 GlobalHistogramAllocator::GlobalHistogramAllocator(
     std::unique_ptr<PersistentMemoryAllocator> memory)
     : PersistentHistogramAllocator(std::move(memory)),