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

components/metrics/file_metrics_provider.cc

Index: components/metrics/file_metrics_provider.cc
diff --git a/components/metrics/file_metrics_provider.cc b/components/metrics/file_metrics_provider.cc
index 44df25c5fd67a62230ded244d2844314996e24ad..02c7a7fd730bbebdceb042fd4d2d0fce606e0437 100644
--- a/components/metrics/file_metrics_provider.cc
+++ b/components/metrics/file_metrics_provider.cc
@@ -69,6 +69,7 @@ FileMetricsProvider::~FileMetricsProvider() {}
 
 void FileMetricsProvider::RegisterFile(const base::FilePath& path,
                                        FileType type,
+                                       FileAssociation file_association,
                                        const base::StringPiece prefs_key) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
@@ -84,7 +85,15 @@ void FileMetricsProvider::RegisterFile(const base::FilePath& path,
                                 file->prefs_key));
   }
 
-  files_to_check_.push_back(std::move(file));
+  switch (file_association) {
+    case ASSOCIATE_CURRENT_RUN:
+      files_to_check_.push_back(std::move(file));
+      break;
+    case ASSOCIATE_PREVIOUS_RUN:
+      DCHECK_EQ(FILE_HISTOGRAMS_ATOMIC, file->type);
+      files_for_previous_.push_back(std::move(file));
+      break;
+  }
 }
 
 // static
@@ -187,7 +196,7 @@ void FileMetricsProvider::RecordHistogramSnapshotsFromFile(
     if (!histogram)
       break;
     if (file->type == FILE_HISTOGRAMS_ATOMIC)
-      snapshot_manager->PrepareAbsoluteTakingOwnership(std::move(histogram));
+      snapshot_manager->PrepareFinalDeltaTakingOwnership(std::move(histogram));
     else
       snapshot_manager->PrepareDeltaTakingOwnership(std::move(histogram));
     ++histogram_count;
@@ -268,6 +277,44 @@ void FileMetricsProvider::OnDidCreateMetricsLog() {
   // check is run off of the worker-pool so as to not cause delays on the
   // main UI thread (which is currently where metric collection is done).
   ScheduleFilesCheck();
+
+  // Clear any data for stability metrics since they're always reported
+  // before the first call to this method.

[Ilya Sherman, 2016/05/04 06:48:07]

Hrm, why not clear them when they're reported?

[bcwhite, 2016/05/04 14:12:51]

They have to live until the reporting is complete which goes beyond when the
method that loads them to the HSM returns.  Ownership of the (heap) histogram
objects was passed to the HSM but they still point to memory managed by an
Allocator and so that can't be released until some time later.

[Ilya Sherman, 2016/05/05 07:22:37]

FileInfo just provides file metadata, right?  How does that relate to the
Allocator?

[bcwhite, 2016/05/05 16:01:17]

It has all the information related to the file, including the MemoryMappedFile
object, the file contents, and the PersistentHistogramAllocator object using
those (one or the other, actually).

+#if DCHECK_IS_ON()
+  for (std::unique_ptr<FileInfo>& file : files_for_previous_)

[Ilya Sherman, 2016/05/04 06:48:07]

nit: "const auto&"?

[bcwhite, 2016/05/04 14:12:51]

Done.

+    DCHECK(file->read_complete);
+#endif
+  files_for_previous_.clear();
+}
+
+bool FileMetricsProvider::HasInitialStabilityMetrics() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  // Check all files for previous run to see if they need to be read.
+  for (auto iter = files_for_previous_.begin();
+       iter != files_for_previous_.end();) {
+    auto temp = iter++;

[Ilya Sherman, 2016/05/04 06:48:07]

"iter++" is strongly discouraged, because it can be significantly less efficient
than "++iter".  Please instead either "++iter" or "iter = 
files_for_previous_.erase(temp)" at the end of the loop, depending on the if
stmt. (For this particular loop, the efficiency difference is probably not
significant, but IIRC it's recommended by the style guide to always use
preincrement, so that you can't accidentally mess up when it does matter.)

[bcwhite, 2016/05/04 14:12:51]

Post-increment is definitely more efficient but ++iter won't work as it would
leave temp pointing to the wrong place.  I could do "temp=iter; ++iter" but
that's the same as "temp=iter++".

I can assign it from the erase() call except that's conditional so would require
an additional "else ++iter".  Maintenance wise, I think that would be a
detriment.  The increment should be at the top where the loop is defined for
clarity and less chance of errors in the future.

[Ilya Sherman, 2016/05/05 07:22:37]

I disagree pretty strongly on this point.  For a loop that has an empty
increment statement, I expect to see the increment happen at the end of the
loop, rather than at the beginning.  (Also, unpacking the line "auto temp =
iter++" involves a lot of work, as I have to figure out what the semantics of
"temp" are, and I have to think about the semantics of pre- vs. post-increment. 
That's a lot of mental overhead when reading this line of code.)

[bcwhite, 2016/05/05 16:01:17]

My expectations differ from yours, then, because if the increment could be done
at the end of the loop then it would have been defined in the if() itself.  It
can't be done at the end because the iterator may be invalidated in the body.
The mental exercise will be greater parsing an if/else condition with one clause
doing an assignment from the return of erase() and the other doing a
pre-increment.

At least pre/post increment has consistent and long-standing known behavior
(back to the very first days of C).  I had to look up the return-code of
erase()... it changes depending on the parameter types.

+    FileInfo* file = temp->get();
+
+    // This would normally be done on a background I/O thread but there
+    // hasn't been a chance to run any at the time this method is called.
+    // Do the check in-line.

[Ilya Sherman, 2016/05/04 06:48:07]

Hmm, this sounds a bit disconcerting.  Are we doing file I/O on the main thread?
 If so, let's ponder a way to structure this code so that we don't need to do
so.

[bcwhite, 2016/05/04 14:12:51]

This method is called during startup and must return before the browser can
show.  Even if it were possible to move the checks to a background thread, the
main thread would still have to block until the background thread completed its
work.

[Ilya Sherman, 2016/05/05 07:22:37]

Erm, why does this block startup?  If that's true, that sounds like a (pretty
egregious) bug.

[bcwhite, 2016/05/05 16:01:17]

All metrics work is done on the main UI thread.  A once talked with Alexei about
moving it to a background thread but he felt is would be difficult and not worth
it.

The check for and reporting of InitialStabilityMetrics is done only once at
startup.  Even if the I/O could be moved to a background thread, there's no
guarantee it would be complete before this call is received and thus it would
still have to block.

The dumping of initial metrics is called in
ChromeBrowserMainParts::PreCreateThreadsImpl().  I don't see any way to avoid
delaying startup for reporting this.

I agree, though, that blocking startup could be a problem but should be
manageable if it is limited to only crashes and hangs.  We're not concerned with
saving metrics during a normal shutdown since metrics are gathered immediately
before a clean exit anyway.  We just make sure the metrics file is either never
written or deleted on exit.  Startup after a clean shutdown will do a stat()
looking for it but that is all.  If there was a crash/hang, then we pay the
penalty of cleaning up after the previous run.

I've added some timing histograms so we can at least have real information about
the severity of this problem before we decide exactly what to do about it.

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

Hmm, I see that you're right.  I still find this pretty surprising, but agree
that it's out of scope for this CL.  Thanks for adding the timing histograms.

+    AccessResult result = CheckAndMapNewMetrics(file);
+    UMA_HISTOGRAM_ENUMERATION("UMA.FileMetricsProvider.StabilityAccessResult",
+                              result, ACCESS_RESULT_MAX);
+
+    // If it couldn't be accessed, remove it from the list. There is only ever
+    // one chance to record it no point keeping it around for later. Also mark
+    // it as having been read since uploading it with a future browser run
+    // would associate it the previous run which would no longer be the run
+    // from which it came.
+    if (result != ACCESS_RESULT_SUCCESS) {
+      RecordFileAsSeen(file);
+      files_for_previous_.erase(temp);
+    }
+  }
+
+  return !files_for_previous_.empty();
 }
 
 void FileMetricsProvider::RecordHistogramSnapshots(
@@ -300,4 +347,23 @@ void FileMetricsProvider::RecordHistogramSnapshots(
   }
 }
 
+void FileMetricsProvider::RecordStabilityHistogramSnapshots(
+    base::HistogramSnapshotManager* snapshot_manager) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  for (std::unique_ptr<FileInfo>& file : files_for_previous_) {

[Ilya Sherman, 2016/05/04 06:48:07]

nit: "const auto&"?

[bcwhite, 2016/05/04 14:12:51]

Done.

+    // The file needs to have an allocator attached to it in order to read
+    // histograms out of it.
+    DCHECK(file->mapped || !file->data.empty());
+    CreateAllocatorForFile(file.get());

[Ilya Sherman, 2016/05/04 06:48:07]

Is this doing file I/O on the main thread?

[bcwhite, 2016/05/04 14:12:51]

Yes, for the same reason as HasInitialStabilityMetrics().  It's not actually the
UI thread because at this point there is no UI.

+    DCHECK(file->allocator);
+
+    // Dump all histograms contained within the file to the snapshot-manager.
+    RecordHistogramSnapshotsFromFile(snapshot_manager, file.get());
+
+    // Update the last-seen time so it isn't read again unless it changes.
+    RecordFileAsSeen(file.get());
+  }
+}
+
 }  // namespace metrics