Use strings for keys in the histogram map.

base/metrics/statistics_recorder.h

Index: base/metrics/statistics_recorder.h
diff --git a/base/metrics/statistics_recorder.h b/base/metrics/statistics_recorder.h
index 6eaf07959f127f9821d3f077360bf22db20bcec0..6c4815e5186d04b37325a34984902ffe9b58cf54 100644
--- a/base/metrics/statistics_recorder.h
+++ b/base/metrics/statistics_recorder.h
@@ -31,8 +31,35 @@ class BucketRanges;
 class Lock;
 
 class BASE_EXPORT StatisticsRecorder {
+  // A class used as a key for the histogram map below. It always references

[Alexei Svitkine (slow), 2016/04/07 20:21:10]

Nit: Move this within the public section below.

[bcwhite, 2016/04/07 20:24:33]

Oh?  It's not something that should be accessed outside of this class.

[Alexei Svitkine (slow), 2016/04/07 20:49:08]

From the style guide:

"Use the specified order of declarations within a class: public: before
private:, methods before data members (variables), etc.

Your class definition should start with its public: section, followed by its
protected: section and then its private: section. If any of these sections are
empty, omit them."

Your code as-is isn't following this. So, either follow my suggestion or find a
way to put this in a private section while following the style guide practice.

Another option is to move this outside the StatisticsRecorder class into a
separate "namespace internal" inside the base namespace.

[bcwhite, 2016/04/07 21:09:16]

It does say "should".  I think that would allow for variations if language
limitations (such as the order of definitions of types) made it impractical.

But whatever.  It's not important so I'll just move it back into "public".

Done.

+  // a string owned outside of this class, likely in the value of the map.
+  class StringKey : public StringPiece {
+   public:
+    // Constructs the StringKey using various sources. The source must live
+    // at least as long as the created object.
+    StringKey(const std::string& str) : StringPiece(str) {}
+    StringKey(StringPiece str) : StringPiece(str) {}
+
+    // Though StringPiece is better passed by value than by reference, in
+    // this case it's being passed many times and likely already been stored
+    // in memory (not just registers) so the benefit of pass-by-value is
+    // negated.
+    bool operator<(const StringKey& rhs) const {
+      // Since order is unimportant in the map and string comparisons can be
+      // slow, use the length as the primary sort value.
+      if (length() < rhs.length())
+        return true;
+      if (length() > rhs.length())
+        return false;
+
+      // Fall back to an actual string comparison. The lengths are the same
+      // so a simple memory-compare is sufficient.
+      return wordmemcmp(data(), rhs.data(), length()) < 0;

[Alexei Svitkine (slow), 2016/04/07 20:21:10]

Nit: How about "return StringPiece::operator<(rhs);" so that it doesn't require
having knowledge of wordmemcmp internal StringPiece impl.

[bcwhite, 2016/04/07 20:24:33]

I had something like that previously but removed it so that it wouldn't do yet
another set of length checks.  Perhaps the compiler can optimize that out.

[Alexei Svitkine (slow), 2016/04/07 20:49:08]

I see - I guess since we don't if the compiler will optimize those out, I'm fine
to keep as-is, just mention explicitly that this doesn't call the base operator
to avoid re-doing those length checks.

[bcwhite, 2016/04/07 21:09:16]

Done.

+    }
+  };
+
  public:
-  typedef std::map<uint64_t, HistogramBase*> HistogramMap;  // Key is name-hash.
+  typedef std::map<StringKey, HistogramBase*> HistogramMap;
   typedef std::vector<HistogramBase*> Histograms;
 
   // A class for iterating over the histograms held within this global resource.