Bug 747379 - Override Histogram::AddSampleSet for FlagHistograms. r=nfroyd
authorGraeme McCutcheon <graememcc_firefox@graeme-online.co.uk>
Fri, 27 Apr 2012 11:14:03 +0100
changeset 94888 05422247ed550b5aafffc2f82e06772dc56b3682
parent 94887 917f69eb26ef02e3b3f2123cecc23a3ec5616c65
child 94889 f769cc9bd71ca6a9b33ccd0439b60a00c29cdf85
push idunknown
push userunknown
push dateunknown
reviewersnfroyd
bugs747379
milestone15.0a1
Bug 747379 - Override Histogram::AddSampleSet for FlagHistograms. r=nfroyd
ipc/chromium/src/base/histogram.cc
ipc/chromium/src/base/histogram.h
toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
--- a/ipc/chromium/src/base/histogram.cc
+++ b/ipc/chromium/src/base/histogram.cc
@@ -957,16 +957,41 @@ FlagHistogram::Accumulate(Sample value, 
 
   mSwitched = true;
   DCHECK_EQ(value, 1);
   Histogram::Accumulate(value, 1, index);
   size_t zero_index = BucketIndex(0);
   Histogram::Accumulate(1, -1, zero_index);
 }
 
+void
+FlagHistogram::AddSampleSet(const SampleSet& sample) {
+  DCHECK_EQ(bucket_count(), sample.size());
+  // We can't be sure the SampleSet provided came from another FlagHistogram,
+  // so we take the following steps:
+  //  - If our flag has already been set do nothing.
+  //  - Set our flag if the following hold:
+  //      - The sum of the counts in the provided SampleSet is 1.
+  //      - The bucket index for that single value is the same as the index where we
+  //        would place our set flag.
+  //  - Otherwise, take no action.
+
+  if (mSwitched) {
+    return;
+  }
+
+  if (sample.sum() != 1) {
+    return;
+  }
+
+  size_t one_index = BucketIndex(1);
+  if (sample.counts(one_index) == 1) {
+    Accumulate(1, 1, one_index);
+  }
+}
 //------------------------------------------------------------------------------
 // CustomHistogram:
 //------------------------------------------------------------------------------
 
 Histogram* CustomHistogram::FactoryGet(const std::string& name,
                                        const std::vector<Sample>& custom_ranges,
                                        Flags flags) {
   Histogram* histogram(NULL);
--- a/ipc/chromium/src/base/histogram.h
+++ b/ipc/chromium/src/base/histogram.h
@@ -331,16 +331,17 @@ class Histogram {
     // Accessor for histogram to make routine additions.
     void Accumulate(Sample value, Count count, size_t index);
 
     // Accessor methods.
     Count counts(size_t i) const { return counts_[i]; }
     Count TotalCount() const;
     int64 sum() const { return sum_; }
     int64 redundant_count() const { return redundant_count_; }
+    size_t size() const { return counts_.size(); }
 
     // Arithmetic manipulation of corresponding elements of the set.
     void Add(const SampleSet& other);
     void Subtract(const SampleSet& other);
 
     bool Serialize(Pickle* pickle) const;
     bool Deserialize(void** iter, const Pickle& pickle);
 
@@ -387,17 +388,17 @@ class Histogram {
   // This method is an interface, used only by BooleanHistogram.
   virtual void AddBoolean(bool value);
 
   // Accept a TimeDelta to increment.
   void AddTime(TimeDelta time) {
     Add(static_cast<int>(time.InMilliseconds()));
   }
 
-  void AddSampleSet(const SampleSet& sample);
+  virtual void AddSampleSet(const SampleSet& sample);
 
   // This method is an interface, used only by LinearHistogram.
   virtual void SetRangeDescriptions(const DescriptionPair descriptions[]);
 
   // The following methods provide graphical histogram displays.
   void WriteHTMLGraph(std::string* output) const;
   void WriteAscii(bool graph_it, const std::string& newline,
                   std::string* output) const;
@@ -656,16 +657,18 @@ class FlagHistogram : public BooleanHist
 {
 public:
   static Histogram *FactoryGet(const std::string &name, Flags flags);
 
   virtual ClassType histogram_type() const;
 
   virtual void Accumulate(Sample value, Count count, size_t index);
 
+  virtual void AddSampleSet(const SampleSet& sample);
+
 private:
   explicit FlagHistogram(const std::string &name);
   bool mSwitched;
 
   DISALLOW_COPY_AND_ASSIGN(FlagHistogram);
 };
 
 //------------------------------------------------------------------------------
--- a/toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
+++ b/toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
@@ -127,41 +127,43 @@ function test_getHistogramById() {
 
 function compareHistograms(h1, h2) {
   let s1 = h1.snapshot();
   let s2 = h2.snapshot();
 
   do_check_eq(s1.histogram_type, s2.histogram_type);
   do_check_eq(s1.min, s2.min);
   do_check_eq(s1.max, s2.max);
-
-  // XXX Don't compare flag sums until bug 747379 is fixed
-  if (s1.histogram_type != Telemetry.HISTOGRAM_FLAG) {
-    do_check_eq(s1.sum, s2.sum);
+  do_check_eq(s1.sum, s2.sum);
 
-    do_check_eq(s1.counts.length, s2.counts.length);
-    for (let i = 0; i < s1.counts.length; i++)
-      do_check_eq(s1.counts[i], s2.counts[i]);
-  }
+  do_check_eq(s1.counts.length, s2.counts.length);
+  for (let i = 0; i < s1.counts.length; i++)
+    do_check_eq(s1.counts[i], s2.counts[i]);
 
   do_check_eq(s1.ranges.length, s2.ranges.length);
   for (let i = 0; i < s1.ranges.length; i++)
     do_check_eq(s1.ranges[i], s2.ranges[i]);
 }
 
 function test_histogramFrom() {
-  // One histogram of each type
+  // Test one histogram of each type.
   let names = ["CYCLE_COLLECTOR", "GC_REASON", "GC_RESET", "TELEMETRY_TEST_FLAG"];
 
   for each (let name in names) {
     let [min, max, bucket_count] = [1, INT_MAX - 1, 10]
     let original = Telemetry.getHistogramById(name);
     let clone = Telemetry.histogramFrom("clone" + name, name);
     compareHistograms(original, clone);
   }
+
+  // Additionally, set the flag on TELEMETRY_TEST_FLAG, and check it gets set on the clone.
+  let testFlag = Telemetry.getHistogramById("TELEMETRY_TEST_FLAG");
+  testFlag.add(1);
+  let clone = Telemetry.histogramFrom("FlagClone", "TELEMETRY_TEST_FLAG");
+  compareHistograms(testFlag, clone);
 }
 
 function test_getSlowSQL() {
   var slow = Telemetry.slowSQL;
   do_check_true(("mainThread" in slow) && ("otherThreads" in slow));
 }
 
 function test_addons() {