Remove std::string histogram name from HistogramBase object.

base/metrics/histogram_base.cc

Index: base/metrics/histogram_base.cc
diff --git a/base/metrics/histogram_base.cc b/base/metrics/histogram_base.cc
index 671cad24290980512c8d284875915b8443e6deb9..d974e8d9b01584151254165c7644f87ae0616bff 100644
--- a/base/metrics/histogram_base.cc
+++ b/base/metrics/histogram_base.cc
@@ -7,17 +7,21 @@
 #include <limits.h>
 
 #include <memory>
+#include <set>
 #include <utility>
 
 #include "base/json/json_string_value_serializer.h"
+#include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/metrics/histogram_samples.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/metrics/statistics_recorder.h"
 #include "base/pickle.h"
 #include "base/process/process_handle.h"
 #include "base/strings/stringprintf.h"
+#include "base/synchronization/lock.h"
 #include "base/values.h"
 
 namespace base {
@@ -63,14 +67,13 @@ HistogramBase* DeserializeHistogramInfo(PickleIterator* iter) {
 const HistogramBase::Sample HistogramBase::kSampleType_MAX = INT_MAX;
 HistogramBase* HistogramBase::report_histogram_ = nullptr;
 
-HistogramBase::HistogramBase(const std::string& name)
-    : histogram_name_(name),
-      flags_(kNoFlags) {}
+HistogramBase::HistogramBase(const char* name)
+    : histogram_name_(name), flags_(kNoFlags) {}
 
 HistogramBase::~HistogramBase() {}
 
 void HistogramBase::CheckName(const StringPiece& name) const {
-  DCHECK_EQ(histogram_name(), name);
+  DCHECK_EQ(StringPiece(histogram_name()), name);
 }
 
 void HistogramBase::SetFlags(int32_t flags) {
@@ -185,6 +188,19 @@ void HistogramBase::WriteAsciiBucketValue(Count current,
 }
 
 // static
+char const* HistogramBase::GetPermanentName(const std::string& name) {

[Alexei Svitkine (slow), 2017/07/17 21:34:43]

Can we avoid having an extra temporary string constructed for when the original
input is a literal?

I believe with your CL, it currently works thus:

HISTOGRAM_*() macro calls const char* version of FactoryGet(). Which then
creates a std::string and calls regular version of FactoryGet(), which then
calls Factory() which stores a reference to that. Which then calls
GetPermanentName() with that entry. Now, since name is const here, the insert()
call on line 199 will make a copy of the name and add it to the set.

So there's a temporary intermediate copy.

I think we can avoid this by plumbing const char* version through and then
having the std::string be created just here on line 199 and so no extra
temporary needed.

[bcwhite, 2017/07/18 01:07:16]

We might also be able to make the name parameter be a "moveable" string to avoid
the copy.  That would probably need a lot of fixes throughout the code, though,
so probably not worth the effort.

And of course, std::string is free to do internal optimizations, too, though I
don't know if it does any.

I'd need two GetPermanentName() methods: one that takes std::string (for when
it's a constructed name) and one that takes const char*... though taking
std::string&& (i.e. movable) might be even better as it would allow the former
to call the latter with a copy rather than duplicate the logic.

Is it worth it considering that persistence is now the 100% default and so this
extra-copy path only gets called when persistence hasn't yet been enabled or
when the memory buffer is full?

+  // A set of histogram names that provides the "permanent" lifetime required
+  // by histogram objects for those strings that are not already code constants
+  // or held in persistent memory.
+  static LazyInstance<std::set<std::string>>::Leaky permanent_names;
+  static LazyInstance<Lock>::Leaky permanent_names_lock;
+
+  AutoLock lock(permanent_names_lock.Get());
+  auto result = permanent_names.Get().insert(name);
+  return result.first->c_str();
+}
+
+// static
 void HistogramBase::ReportHistogramActivity(const HistogramBase& histogram,
                                             ReportActivity activity) {
   if (!report_histogram_)