Implement write methods for SparseHistogram

base/metrics/sparse_histogram.cc

Index: base/metrics/sparse_histogram.cc
diff --git a/base/metrics/sparse_histogram.cc b/base/metrics/sparse_histogram.cc
index 295267335a4efd6fb595c895ffdc93706a06500f..2cee63422838d71967cf78f99ef353bff3f0e3da 100644
--- a/base/metrics/sparse_histogram.cc
+++ b/base/metrics/sparse_histogram.cc
@@ -7,6 +7,7 @@
 #include "base/metrics/sample_map.h"
 #include "base/metrics/statistics_recorder.h"
 #include "base/pickle.h"
+#include "base/stringprintf.h"
 #include "base/synchronization/lock.h"
 
 using std::map;
@@ -69,11 +70,13 @@ bool SparseHistogram::AddSamplesFromPickle(PickleIterator* iter) {
 }
 
 void SparseHistogram::WriteHTMLGraph(string* output) const {
-  // TODO(kaiwang): Implement.
+  output->append("<PRE>");
+  WriteAsciiImpl(true, "<br>", output);
+  output->append("</PRE>");
 }
 
 void SparseHistogram::WriteAscii(string* output) const {
-  // TODO(kaiwang): Implement.
+  WriteAsciiImpl(true, "\n", output);
 }
 
 bool SparseHistogram::SerializeInfoImpl(Pickle* pickle) const {
@@ -106,4 +109,97 @@ void SparseHistogram::GetCountAndBucketData(Count* count,
   // TODO(kaiwang): Implement. (See HistogramBase::WriteJSON.)
 }
 
+void SparseHistogram::WriteAsciiImpl(bool graph_it,
+                                     const std::string& newline,
+                                     std::string* output) const {
+  // Get a local copy of the data so we are consistent

[jar (doing other things), 2013/04/04 00:45:09]

nit: Period at end of line.

[jrummell, 2013/04/04 22:38:59]

Done.

+  scoped_ptr<HistogramSamples> snapshot = SnapshotSamples();
+  Count sample_count = snapshot->TotalCount();
+
+  WriteAsciiHeader(sample_count, output);
+  output->append(newline);
+
+  // determine largest value (needed to determine how much space to print)
+  // and largest count in a bucket

[jar (doing other things), 2013/04/04 00:45:09]

nit: Upper case start of sentence, and add a period.

Same thing for comments below.

The comment also had me a bit confused.  Please consider instead:

Determine how wide the largest bucket range is (how many digits to print), so
that we'll be able to right-align starts for the graphical bars.
Determine which bucket has the largest sample count so that we can normalize the
graphical bar-width relative to that sample count.

[jrummell, 2013/04/04 22:38:59]

Done.

+  Count max_size = 0;
+  Sample largest = 0;

[jar (doing other things), 2013/04/04 00:45:09]

nit: suggest largest_count as a better name.  I was getting confused relative to
max_size when reading.

[jrummell, 2013/04/04 22:38:59]

Done.

+  scoped_ptr<SampleCountIterator> it = snapshot->Iterator();
+  while (!it->Done())
+  {
+    Sample min;
+    Sample max;
+    Count count;
+    it->Get(&min, &max, &count);
+    if (min > largest)
+      largest = min;
+    if (count > max_size)
+      max_size = count;
+    it->Next();
+  }
+  size_t print_width = GetAsciiBucketRange(largest).size() + 1;
+
+  // iterate over each item and display them
+  it = snapshot->Iterator();
+  while (!it->Done())
+  {
+    Sample min;
+    Sample max;
+    Count count;
+    it->Get(&min, &max, &count);
+
+    // value is min, so display it
+    string range = GetAsciiBucketRange(min);
+    output->append(range);
+    for (size_t j = 0; range.size() + j < print_width + 1; ++j)
+      output->push_back(' ');
+
+    if (graph_it)
+      WriteAsciiBucketGraph(count, max_size, output);
+    WriteAsciiBucketValue(count, sample_count, output);
+    output->append(newline);
+    it->Next();
+  }
+}
+
+void SparseHistogram::WriteAsciiHeader(const Count sample_count,
+                                       std::string* output) const {
+  StringAppendF(output,
+                "Histogram: %s recorded %d samples",
+                histogram_name().c_str(),
+                sample_count);
+  if (flags() & ~kHexRangePrintingFlag)
+    StringAppendF(output, " (flags = 0x%x)", flags() & ~kHexRangePrintingFlag);
+}
+
+void SparseHistogram::WriteAsciiBucketValue(const Count current,
+                                            const Count total,
+                                            std::string* output) const {
+  float percentage = (float)current / (float)total * 100.0;
+  StringAppendF(output, " (%d = %3.1f%%)", current, percentage);
+}
+
+void SparseHistogram::WriteAsciiBucketGraph(double current_size,
+                                            double max_size,
+                                            std::string* output) const {
+  const int k_line_length = 72;  // Maximal horizontal width of graph.

[jar (doing other things), 2013/04/04 00:45:09]

nit: This method is also repeated in histogram.cc.

Can we put a method in base, as suggested in line 196?

[jrummell, 2013/04/04 22:38:59]

Done.

+  int x_count = static_cast<int>(k_line_length * (current_size / max_size)
+                                 + 0.5);
+  int x_remainder = k_line_length - x_count;
+
+  while (0 < x_count--)
+    output->append("-");
+  output->append("O");
+  while (0 < x_remainder--)
+    output->append(" ");
+}
+
+std::string SparseHistogram::GetAsciiBucketRange(const Sample sample) const {

[jar (doing other things), 2013/04/04 00:45:09]

nit: This code is repeated in histogram.cc.

Could we add a method in HistogramBase to do this work? Maybe the base method
should be something like GetSimpleAsciiBucketRange(), and we'd forward to it
here?

Same thing fro line 181 and line 174.

[jrummell, 2013/04/04 22:38:59]

Done.

+  std::string result;
+  if (kHexRangePrintingFlag & flags())
+    StringAppendF(&result, "%#x", sample);
+  else
+    StringAppendF(&result, "%d", sample);
+  return result;
+}
+
 }  // namespace base