Avoid computing md5 hash multiple times in Sparse Histogram.

This change is a continuation on reusing the already computed hash for
the metric name instead of recomputing it each time.

Create an extra parameter in
PersistentHistogramAllocator::AllocateHistogram so we can pass name_hash
as a parameter.

Change-Id: Iae7964c58e09cd3945483cba6e05da89b652db00
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6469736
Reviewed-by: Robert Kaplow <[email protected]>
Commit-Queue: Ramon Cano Aparicio <[email protected]>
Reviewed-by: Luc Nguyen <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1458097}
NOKEYCHECK=True
GitOrigin-RevId: eb4e315ee8e550eaf1a0e5d5a844ef80129df9fd

Author: Ramon Cano Aparicio

metrics/histogram.cc

@@ -198,8 +198,8 @@ HistogramBase* Histogram::Factory::Build() {
       // CreateHistogram, which calls HashMetricName. We already have the hash,
       // so we could pass it in.
       tentative_histogram = allocator->AllocateHistogram(
-          histogram_type_, name_, minimum_, maximum_, registered_ranges, flags_,
-          &histogram_ref);
+          histogram_type_, name_, name_hash, minimum_, maximum_,
+          registered_ranges, flags_, &histogram_ref);
     }
 
     // Handle the case where no persistent allocator is present or the

metrics/persistent_histogram_allocator.cc

@@ -310,6 +310,7 @@ std::unique_ptr<HistogramBase> PersistentHistogramAllocator::GetHistogram(
   // Check that metadata is reasonable: metric_name is non-empty,
   // ID fields have been loaded with a hash of the name (0 is considered
   // unset/invalid).
+  uint64_t name_hash = HashMetricName(*durable_metric_name);
   if (durable_metric_name->empty() ||
       UNSAFE_TODO(reinterpret_cast<const char*>(data)[alloc_size - 1]) !=
           '\0' ||
@@ -321,15 +322,16 @@ std::unique_ptr<HistogramBase> PersistentHistogramAllocator::GetHistogram(
       // could just verify the name length based on the overall alloc length,
       // but that doesn't work because the allocated block may have been
       // aligned to the next boundary value.
-      HashMetricName(*durable_metric_name) != data->samples_metadata.id) {
+      name_hash != data->samples_metadata.id) {
     return nullptr;
   }
-  return CreateHistogram(data, durable_metric_name);
+  return CreateHistogram(data, durable_metric_name, name_hash);
 }
 
 std::unique_ptr<HistogramBase> PersistentHistogramAllocator::AllocateHistogram(
     HistogramType histogram_type,
     std::string_view name,
+    uint64_t name_hash,
     int minimum,
     int maximum,
     const BucketRanges* bucket_ranges,
@@ -438,7 +440,7 @@ std::unique_ptr<HistogramBase> PersistentHistogramAllocator::AllocateHistogram(
     DurableStringView durable_name(
         std::string_view(histogram_data->name, name.size()));
     std::unique_ptr<HistogramBase> histogram =
-        CreateHistogram(histogram_data, durable_name);
+        CreateHistogram(histogram_data, durable_name, name_hash);
     DCHECK(histogram);
     DCHECK_NE(0U, histogram_data->samples_metadata.id);
     DCHECK_NE(0U, histogram_data->logged_metadata.id);
@@ -549,7 +551,8 @@ void PersistentHistogramAllocator::ClearLastCreatedReferenceForTesting() {
 
 std::unique_ptr<HistogramBase> PersistentHistogramAllocator::CreateHistogram(
     PersistentHistogramData* histogram_data_ptr,
-    DurableStringView durable_name) {
+    DurableStringView durable_name,
+    uint64_t name_hash) {
   if (!histogram_data_ptr) {
     return nullptr;
   }
@@ -561,7 +564,7 @@ std::unique_ptr<HistogramBase> PersistentHistogramAllocator::CreateHistogram(
   // Sparse histograms are quite different so handle them as a special case.
   if (histogram_data_ptr->histogram_type == SPARSE_HISTOGRAM) {
     std::unique_ptr<HistogramBase> histogram =
-        SparseHistogram::PersistentCreate(this, durable_name,
+        SparseHistogram::PersistentCreate(this, durable_name, name_hash,
                                           &histogram_data_ptr->samples_metadata,
                                           &histogram_data_ptr->logged_metadata);
     DCHECK(histogram);
@@ -654,6 +657,8 @@ std::unique_ptr<HistogramBase> PersistentHistogramAllocator::CreateHistogram(
 
   // Create the right type of histogram.
   std::unique_ptr<HistogramBase> histogram;
+  // TODO(crbug.com/394149163): We can pass the name_hash to the histogram
+  // constructors, and then use it here.
   switch (histogram_type) {
     case HISTOGRAM:
       histogram = Histogram::PersistentCreate(

metrics/persistent_histogram_allocator.h

@@ -254,6 +254,7 @@ class BASE_EXPORT PersistentHistogramAllocator {
   std::unique_ptr<HistogramBase> AllocateHistogram(
       HistogramType histogram_type,
       std::string_view name,
+      uint64_t name_hash,
       int minimum,
       int maximum,
       const BucketRanges* bucket_ranges,
@@ -340,7 +341,8 @@ class BASE_EXPORT PersistentHistogramAllocator {
   // `alloc_size` returned from the `memory_allocator`.
   std::unique_ptr<HistogramBase> CreateHistogram(
       PersistentHistogramData* histogram_data_ptr,
-      DurableStringView durable_name);
+      DurableStringView durable_name,
+      uint64_t name_hash);
 
   // Gets or creates an object in the global StatisticsRecorder matching
   // the `histogram` passed. Null is returned if one was not found and

metrics/sparse_histogram.cc

@@ -43,20 +43,18 @@ HistogramBase* SparseHistogram::FactoryGet(std::string_view name,
     std::unique_ptr<HistogramBase> tentative_histogram;
     PersistentHistogramAllocator* allocator = GlobalHistogramAllocator::Get();
     if (allocator) {
-      // TODO(crbug.com/394149163): AllocateHistogram ends up calling
-      // HashMetricName. We already have the hash, so we could pass it in.
       tentative_histogram = allocator->AllocateHistogram(
-          SPARSE_HISTOGRAM, name, 0, 0, nullptr, flags, &histogram_ref);
+          SPARSE_HISTOGRAM, name, name_hash, /*minimum=*/0, /*maximum=*/0,
+          /*bucket_ranges=*/nullptr, flags, &histogram_ref);
     }
 
     // Handle the case where no persistent allocator is present or the
     // persistent allocation fails (perhaps because it is full).
     if (!tentative_histogram) {
       DCHECK(!histogram_ref);  // Should never have been set.
       flags &= ~HistogramBase::kIsPersistent;
-      // TODO(crbug.com/394149163): SparseHistogram constructor ends up calling
-      // HashMetricName. We already have the hash, so we could pass it in.
-      tentative_histogram.reset(new SparseHistogram(GetPermanentName(name)));
+      tentative_histogram.reset(
+          new SparseHistogram(GetPermanentName(name), name_hash));
       tentative_histogram->SetFlags(flags);
     }
 
@@ -95,10 +93,11 @@ HistogramBase* SparseHistogram::FactoryGet(std::string_view name,
 std::unique_ptr<HistogramBase> SparseHistogram::PersistentCreate(
     PersistentHistogramAllocator* allocator,
     DurableStringView durable_name,
+    uint64_t name_hash,
     HistogramSamples::Metadata* meta,
     HistogramSamples::Metadata* logged_meta) {
-  return WrapUnique(
-      new SparseHistogram(allocator, durable_name, meta, logged_meta));
+  return WrapUnique(new SparseHistogram(allocator, durable_name, name_hash,
+                                        meta, logged_meta));
 }
 
 SparseHistogram::~SparseHistogram() = default;
@@ -207,12 +206,19 @@ void SparseHistogram::SerializeInfoImpl(Pickle* pickle) const {
 }
 
 SparseHistogram::SparseHistogram(DurableStringView durable_name)
+    : SparseHistogram(durable_name, HashMetricName(*durable_name)) {}
+
+SparseHistogram::SparseHistogram(DurableStringView durable_name,
+                                 uint64_t name_hash)
     : HistogramBase(durable_name),
-      unlogged_samples_(new SampleMap(HashMetricName(*durable_name))),
-      logged_samples_(new SampleMap(unlogged_samples_->id())) {}
+      unlogged_samples_(new SampleMap(name_hash)),
+      logged_samples_(new SampleMap(unlogged_samples_->id())) {
+  DCHECK_EQ(name_hash, HashMetricName(*durable_name)) << "Name hash mismatch";
+}
 
 SparseHistogram::SparseHistogram(PersistentHistogramAllocator* allocator,
                                  DurableStringView durable_name,
+                                 uint64_t name_hash,
                                  HistogramSamples::Metadata* meta,
                                  HistogramSamples::Metadata* logged_meta)
     : HistogramBase(durable_name),
@@ -226,12 +232,12 @@ SparseHistogram::SparseHistogram(PersistentHistogramAllocator* allocator,
       // "active" samples use, for convenience purposes, an ID matching
       // that of the histogram while the "logged" samples use that number
       // plus 1.
-      unlogged_samples_(new PersistentSampleMap(HashMetricName(*durable_name),
-                                                allocator,
-                                                meta)),
+      unlogged_samples_(new PersistentSampleMap(name_hash, allocator, meta)),
       logged_samples_(new PersistentSampleMap(unlogged_samples_->id() + 1,
                                               allocator,
-                                              logged_meta)) {}
+                                              logged_meta)) {
+  DCHECK_EQ(name_hash, HashMetricName(*durable_name)) << "Name hash mismatch";
+}
 
 HistogramBase* SparseHistogram::DeserializeInfoImpl(PickleIterator* iter) {
   std::string histogram_name;

metrics/sparse_histogram.h

@@ -36,6 +36,7 @@ class BASE_EXPORT SparseHistogram : public HistogramBase {
   static std::unique_ptr<HistogramBase> PersistentCreate(
       PersistentHistogramAllocator* allocator,
       DurableStringView name,
+      uint64_t name_hash,
       HistogramSamples::Metadata* meta,
       HistogramSamples::Metadata* logged_meta);
 
@@ -69,8 +70,14 @@ class BASE_EXPORT SparseHistogram : public HistogramBase {
   // Clients should always use FactoryGet to create SparseHistogram.
   explicit SparseHistogram(DurableStringView name);
 
+  // Same as above, but takes a pre-computed `name_hash`. This function is more
+  // efficient as it avoids recomputing the hash if it's already known. The
+  // `name_hash` must be the hash of `name`, this is enforced with a DCHECK.
+  SparseHistogram(DurableStringView name, uint64_t name_hash);
+
   SparseHistogram(PersistentHistogramAllocator* allocator,
                   DurableStringView name,
+                  uint64_t name_hash,
                   HistogramSamples::Metadata* meta,
                   HistogramSamples::Metadata* logged_meta);