Remove std::string histogram name from HistogramBase object.

base/metrics/histogram_base.cc

 // 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.
 
 #include "base/metrics/histogram_base.h"
 
 #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 {
 
 std::string HistogramTypeToString(HistogramType type) {
   switch (type) {
     case HISTOGRAM:
       return "HISTOGRAM";
     case LINEAR_HISTOGRAM:
       return "LINEAR_HISTOGRAM";
@@ skipping 25 matching lines @@
     case SPARSE_HISTOGRAM:
       return SparseHistogram::DeserializeInfoImpl(iter);
     default:
       return NULL;
   }
 }
 
 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) {
   HistogramBase::Count old_flags = subtle::NoBarrier_Load(&flags_);
   subtle::NoBarrier_Store(&flags_, old_flags | flags);
 }
 
 void HistogramBase::ClearFlags(int32_t flags) {
   HistogramBase::Count old_flags = subtle::NoBarrier_Load(&flags_);
   subtle::NoBarrier_Store(&flags_, old_flags & ~flags);
@@ skipping 94 matching lines @@
   return StringPrintf("%d", sample);
 }
 
 void HistogramBase::WriteAsciiBucketValue(Count current,
                                           double scaled_sum,
                                           std::string* output) const {
   StringAppendF(output, " (%d = %3.1f%%)", current, current/scaled_sum);
 }
 
 // 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_)
     return;
 
   const int32_t flags = histogram.flags_;
   HistogramReport report_type = HISTOGRAM_REPORT_MAX;
   switch (activity) {
     case HISTOGRAM_CREATED:
       report_histogram_->Add(HISTOGRAM_REPORT_HISTOGRAM_CREATED);
@@ skipping 23 matching lines @@
         report_histogram_->Add(HISTOGRAM_REPORT_FLAG_UMA_TARGETED);
       break;
 
     case HISTOGRAM_LOOKUP:
       report_histogram_->Add(HISTOGRAM_REPORT_HISTOGRAM_LOOKUP);
       break;
   }
 }
 
 }  // namespace base