Merge multiple histogram snapshots into single one for reporting.

base/metrics/histogram_snapshot_manager.h

Index: base/metrics/histogram_snapshot_manager.h
diff --git a/base/metrics/histogram_snapshot_manager.h b/base/metrics/histogram_snapshot_manager.h
index d1ba2a5002a88273c7e88b449f0f7f4a1be07cd3..97bd693966fd6b1ed8ef2c84bcaaafee9eb8a0bc 100644
--- a/base/metrics/histogram_snapshot_manager.h
+++ b/base/metrics/histogram_snapshot_manager.h
@@ -10,6 +10,7 @@
 #include <map>
 #include <string>
 
+#include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/metrics/histogram_base.h"
 
@@ -26,6 +27,28 @@ class HistogramFlattener;
 // calling for the marginal change (a.k.a., delta) in a histogram to be
 // recorded.
 class BASE_EXPORT HistogramSnapshotManager {
+  struct SampleInfo {

[Alexei Svitkine (slow), 2016/02/09 19:46:51]

Add a comment about this struct overall - what's it for / what's it storing?

[bcwhite, 2016/02/11 16:42:39]

Done.

+    // A histogram associated with this sample; it may be one of many if
+    // several have been aggregated into the same "accumulated" sample set.
+    // Ownership of the histogram remains elsewhere and this pointer is
+    // cleared by FinishDeltas().
+    const HistogramBase* histogram;
+
+    // The current snapshot-delta values being accumulated. There is no need
+    // for this to be a scoped_ptr<> because it is only used temporarily

[Alexei Svitkine (slow), 2016/02/09 19:46:51]

I don't think it's any better to avoid the scoped_ptr<> just to have to explain
it in a comment and have a delete instead of .release(). Just use a scoped_ptr.

[bcwhite, 2016/02/11 16:42:39]

Done.

[bcwhite, 2016/02/12 02:03:07]

It turns out that using a scoped_ptr here is a real PITB due to the struct being
used as the value of a std::map.  Some architectures apparently don't yet use
std::move of the value, instead trying to call the copy-ctor which breaks
because you can't do that with a scoped_ptr.  <sigh>

Though I haven't tried it, I assume using a map value of scoped_ptr<SampleInfo>
would fail the same way (in addition to being simply wasteful).

So...  While I agree with you that the scoped_ptr is actually the better way,
I'd like to switch this back to being a raw pointer managed in code with a TODO
to fix it once all architectures support it.

Thoughts?

+    // during a loop after which it is deleted.
+    HistogramSamples* accumulated;

[Alexei Svitkine (slow), 2016/02/09 19:46:51]

Nit: accumulated_samples

[bcwhite, 2016/02/11 16:42:39]

Done.

+
+    // The set of inconsistencies (flags) already seen for the histogram.

[Alexei Svitkine (slow), 2016/02/09 19:46:51]

Mention what flags? Where are they defined?

[bcwhite, 2016/02/11 16:42:39]

Done.

+    int inconsistencies;
+
+    SampleInfo() : histogram(nullptr),
+                   accumulated(nullptr),
+                   inconsistencies(0) {}
+  };
+
+  typedef std::map<uint64_t, SampleInfo> HashInfoMap;

[Alexei Svitkine (slow), 2016/02/09 19:46:51]

You only use this typedef once, so I'd just not have the typedef and have the
struct be above the map declaration.

[bcwhite, 2016/02/11 16:42:39]

I guess all other uses became "auto".  Handy, that.  Done.

+

[Alexei Svitkine (slow), 2016/02/09 19:46:51]

You keep placing this above the public: section - that's not the right place for
them per the style guide. If they need to be referenced from the public section,
put them in the public section. Otherwise, in the private: section.

[bcwhite, 2016/02/11 16:42:39]

There is no longer any access to it from public so I've moved it down below.

  public:
   explicit HistogramSnapshotManager(HistogramFlattener* histogram_flattener);
   virtual ~HistogramSnapshotManager();
@@ -35,36 +58,55 @@ class BASE_EXPORT HistogramSnapshotManager {
   // |required_flags| is used to select histograms to be recorded.
   // Only histograms that have all the flags specified by the argument will be
   // chosen. If all histograms should be recorded, set it to
-  // |Histogram::kNoFlags|.
+  // |Histogram::kNoFlags|. Though any "forward" iterator will work, the
+  // histograms over which it iterates *must* remain valid until this method
+  // returns; the iterator cannot deallocate histograms once it iterates past
+  // them.
   template <class ForwardHistogramIterator>
   void PrepareDeltas(ForwardHistogramIterator begin,
                      ForwardHistogramIterator end,
                      HistogramBase::Flags flags_to_set,
                      HistogramBase::Flags required_flags) {
+    StartDeltas();
     for (ForwardHistogramIterator it = begin; it != end; ++it) {
       (*it)->SetFlags(flags_to_set);
       if (((*it)->flags() & required_flags) == required_flags)
-        PrepareDelta(**it);
+        PrepareDelta(*it);
     }
+    FinishDeltas();
   }
 
+  // When the collection is not so simple as can be done using a single
+  // iterator, the steps can be performed separately. Call PerpareDelta()
+  // as many times as necessary with a single StartDeltas() before and
+  // a single FinishDeltas() after. All passed histograms must live
+  // until FinishDeltas() completes. PrepareOnce() works the same

[Alexei Svitkine (slow), 2016/02/09 19:46:51]

Nit: I find PrepareOnce() not very clear - how about PrepareWithoutDeltas()?

[bcwhite, 2016/02/11 16:42:39]

PrepareAbsolute?

+  // but assumes there were no previous logged values and no future deltas
+  // will be created (and thus can work on read-only histograms).
+  void StartDeltas();
+  void PrepareDelta(HistogramBase* histogram);
+  void PrepareOnce(const HistogramBase* histogram);

[Alexei Svitkine (slow), 2016/02/09 19:46:51]

Nit: Make the param a const ref, which is best practice for const params unless
they're allowed to be null.

[bcwhite, 2016/02/11 16:42:39]

Since it's an either/or call to these two methods, wouldn't it be better to keep
the parameters the same style?

[Alexei Svitkine (slow), 2016/02/16 16:46:05]

But it would be different callers to them, right? Not like an if (foo) callA();
else callB(). If it was like that then I agree it would be bad to be
inconsistent in this case - but if it's completely separate callers, I don't see
it being an issue.

[bcwhite, 2016/02/16 18:06:10]

Same caller.  FileMetricsProvider calls PrepareDelta() if the file is read/write
and PrepareOnce (PrepareAbsolute?) if it's read-only.

+  void FinishDeltas();
+
  private:
-  // Snapshot this histogram, and record the delta.
-  void PrepareDelta(const HistogramBase& histogram);
+  FRIEND_TEST_ALL_PREFIXES(HistogramSnapshotManagerTest, CheckMerge);
+
+  // Capture and hold samples from a histogram. This does all the heavy
+  // lifting for PrepareDelta() and PrepareOnce().
+  void PrepareSamples(const HistogramBase* histogram,
+                      scoped_ptr<HistogramSamples> samples);
 
   // Try to detect and fix count inconsistency of logged samples.
   void InspectLoggedSamplesInconsistency(
       const HistogramSamples& new_snapshot,
       HistogramSamples* logged_samples);
 
-  // For histograms, track what we've already recorded (as a sample for
-  // each histogram) so that we can record only the delta with the next log.
-  // The information is indexed by the hash of the histogram name.
-  std::map<uint64_t, HistogramSamples*> logged_samples_;
-
-  // Set of histograms found to be corrupt and their problems, indexed
+  // For histograms, track what has been previously seen, indexed
   // by the hash of the histogram name.
-  std::map<uint64_t, int> inconsistencies_;
+  HashInfoMap known_histograms_;
+
+  // Indicates if deltas are currently being prepared.
+  bool preparing_deltas_;
 
   // |histogram_flattener_| handles the logistics of recording the histogram
   // deltas.