Use strings for keys in the histogram map.

base/metrics/statistics_recorder.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // StatisticsRecorder holds all Histograms and BucketRanges that are used by
 // Histograms in the system. It provides a general place for
 // Histograms/BucketRanges to register, and supports a global API for accessing
 // (i.e., dumping, or graphing) the data.
 
 #ifndef BASE_METRICS_STATISTICS_RECORDER_H_
@@ skipping 13 matching lines @@
 #include "base/macros.h"
 #include "base/metrics/histogram_base.h"
 #include "base/strings/string_piece.h"
 
 namespace base {
 
 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.
   class BASE_EXPORT HistogramIterator {
    public:
     HistogramIterator(const HistogramMap::iterator& iter,
                       bool include_persistent);
     HistogramIterator(const HistogramIterator& rhs);  // Must be copyable.
     ~HistogramIterator();
 
@@ skipping 133 matching lines @@
 
   // Lock protects access to above maps.
   static base::Lock* lock_;
 
   DISALLOW_COPY_AND_ASSIGN(StatisticsRecorder);
 };
 
 }  // namespace base
 
 #endif  // BASE_METRICS_STATISTICS_RECORDER_H_