Add support for persistent sparse histograms.

base/metrics/histogram_persistence.cc

 // Copyright (c) 2015 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_persistence.h"
 
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/histogram_base.h"
 #include "base/metrics/histogram_samples.h"
+#include "base/metrics/sparse_histogram.h"
 #include "base/metrics/statistics_recorder.h"
 #include "base/synchronization/lock.h"
 
 namespace base {
 
 namespace {
 
 // Enumerate possible creation results for reporting.
 enum CreateHistogramResultType {
   // Everything was fine.
@@ skipping 91 matching lines @@
     return nullptr;
 
   return ranges.release();
 }
 
 // Calculate the number of bytes required to store all of a histogram's
 // "counts". This will return zero (0) if |bucket_count| is not valid.
 size_t CalculateRequiredCountsBytes(size_t bucket_count) {
   // 2 because each "sample count" also requires a backup "logged count"
   // used for calculating the delta during snapshot operations.
   const unsigned kBytesPerBucket = 2 * sizeof(HistogramBase::AtomicCount);

[grt (UTC plus 2), 2016/03/02 20:00:42]

sizeof() returns a size_t. please change the type of this to any of: size_t,
int, or a fixed-size int type.

[bcwhite, 2016/03/02 22:32:43]

Done.

 
   // If the |bucket_count| is such that it would overflow the return type,
   // perhaps as the result of a malicious actor, then return zero to
   // indicate the problem to the caller.
   if (bucket_count > std::numeric_limits<uint32_t>::max() / kBytesPerBucket)

[grt (UTC plus 2), 2016/03/02 20:00:42]

why is this comparing with uint32_t's max when the return type is size_t? this
is inconsistent with the comment, which says that the check is to avoid
overflowing the return type.

[bcwhite, 2016/03/02 22:32:43]

Done.

     return 0;

[grt (UTC plus 2), 2016/03/02 20:00:42]

if this should never happen, and always means that something has gone horribly
wrong (or is being tampered with), then CHECK(false); seems like the right thing
to do. this will crash the process immediately, and send us a dump telling us
why.

[bcwhite, 2016/03/02 22:32:43]

This is a tough balance.  On one hand, it should never happen in a
well-functioning system so a crash seems reasonable.  On the other hand, it's
dependent on data outside of the control of this module and thus end up crashing
for things it should not.

For example, a corrupted file from setup.exe could cause a debug build to crash
after one minute of operation as the file gets read.

I used to have more such DCHECK() for things that simply shouldn't happen but
I've been moving away from them on anything that could be "external" input.

 
   return bucket_count * kBytesPerBucket;
 }
 
 }  // namespace
 
 const Feature kPersistentHistogramsFeature{
   "PersistentHistograms", FEATURE_DISABLED_BY_DEFAULT
 };
 
@@ skipping 90 matching lines @@
 
 HistogramBase* CreatePersistentHistogram(
     PersistentMemoryAllocator* allocator,
     PersistentHistogramData* histogram_data_ptr) {
   if (!histogram_data_ptr) {
     RecordCreateHistogramResult(CREATE_HISTOGRAM_INVALID_METADATA_POINTER);
     NOTREACHED();
     return nullptr;
   }
 
+  // Sparse histograms are quite different so handle them as a special case.
+  if (histogram_data_ptr->histogram_type == SPARSE_HISTOGRAM) {
+    HistogramBase* histogram = SparseHistogram::PersistentGet(
+        allocator,
+        histogram_data_ptr->name,
+        &histogram_data_ptr->samples_metadata,
+        &histogram_data_ptr->logged_metadata);
+    DCHECK(histogram);
+    return histogram;
+  }
+
   // Copy the histogram_data to local storage because anything in persistent
   // memory cannot be trusted as it could be changed at any moment by a
   // malicious actor that shares access. The contents of histogram_data are
   // validated below; the local copy is to ensure that the contents cannot
   // be externally changed between validation and use.
   PersistentHistogramData histogram_data = *histogram_data_ptr;
 
   HistogramBase::Sample* ranges_data =
       allocator->GetAsObject<HistogramBase::Sample>(histogram_data.ranges_ref,
                                                     kTypeIdRangesArray);
@@ skipping 161 matching lines @@
 
   // If the allocator is corrupt, don't waste time trying anything else.
   // This also allows differentiating on the dashboard between allocations
   // failed due to a corrupt allocator and the number of process instances
   // with one, the latter being idicated by "newly corrupt", below.
   if (allocator->IsCorrupt()) {
     RecordCreateHistogramResult(CREATE_HISTOGRAM_ALLOCATOR_CORRUPT);
     return nullptr;
   }
 
-  // If CalculateRequiredCountsBytes() returns zero then the bucket_count
-  // was not valid.
-  size_t bucket_count = bucket_ranges->bucket_count();
-  size_t counts_bytes = CalculateRequiredCountsBytes(bucket_count);
-  if (!counts_bytes) {
-    NOTREACHED();
-    return nullptr;
+  // Create all the metadata necessary for a persistent histogram. Sparse
+  // histograms are quite different so handle them as a special case.
+  PersistentMemoryAllocator::Reference histogram_ref = 0;
+  PersistentHistogramData* histogram_data = nullptr;
+  if (histogram_type == SPARSE_HISTOGRAM) {
+    histogram_ref = allocator->Allocate(
+        offsetof(PersistentHistogramData, name) + name.length() + 1,
+        kTypeIdHistogram);
+    histogram_data = allocator->GetAsObject<PersistentHistogramData>(
+        histogram_ref, kTypeIdHistogram);
+    if (histogram_data) {
+      strcpy(histogram_data->name, name.c_str());
+      histogram_data->histogram_type = histogram_type;
+      histogram_data->flags = flags;
+    }
+  } else {
+    // If CalculateRequiredCountsBytes() returns zero then the bucket_count

[grt (UTC plus 2), 2016/03/02 20:00:42]

nit: move this comment into the "!counts_bytes" block as something like:
      // |bucket_count| is out of range.

[bcwhite, 2016/03/02 22:32:43]

Done.

+    // was not valid.
+    size_t bucket_count = bucket_ranges->bucket_count();
+    size_t counts_bytes = CalculateRequiredCountsBytes(bucket_count);
+    if (!counts_bytes) {
+      NOTREACHED();
+      return nullptr;
+    }
+
+    size_t ranges_bytes = (bucket_count + 1) * sizeof(HistogramBase::Sample);
+    PersistentMemoryAllocator::Reference counts_ref =
+        allocator->Allocate(counts_bytes, kTypeIdCountsArray);
+    PersistentMemoryAllocator::Reference ranges_ref =
+        allocator->Allocate(ranges_bytes, kTypeIdRangesArray);
+    HistogramBase::Sample* ranges_data =
+        allocator->GetAsObject<HistogramBase::Sample>(ranges_ref,
+                                                      kTypeIdRangesArray);
+    histogram_ref = allocator->Allocate(
+        offsetof(PersistentHistogramData, name) + name.length() + 1,
+        kTypeIdHistogram);
+    histogram_data = allocator->GetAsObject<PersistentHistogramData>(
+        histogram_ref, kTypeIdHistogram);
+
+    // Only continue here if all allocations were successful. If they weren't
+    // there is no way to free the space but that's not really a problem since
+    // the allocations only fail because the space is full and so any future
+    // attempts will also fail.
+    if (counts_ref && ranges_data && histogram_data) {
+      strcpy(histogram_data->name, name.c_str());

[grt (UTC plus 2), 2016/03/02 20:00:42]

nit: use memcpy since you know the size

[bcwhite, 2016/03/02 22:32:43]

Done.

+      for (size_t i = 0; i < bucket_ranges->size(); ++i)
+        ranges_data[i] = bucket_ranges->range(i);
+
+      histogram_data->histogram_type = histogram_type;
+      histogram_data->flags = flags;
+      histogram_data->minimum = minimum;
+      histogram_data->maximum = maximum;
+      histogram_data->bucket_count = static_cast<uint32_t>(bucket_count);

[grt (UTC plus 2), 2016/03/02 20:00:42]

i think this is safe on account of CalculateRequiredCountsBytes, but it would be
nice to have an explicit range check for bucket_count up on line 452. why does
BucketRanges use size_t?

[bcwhite, 2016/03/02 22:32:43]

It's actually limited because any allocation that exceeds 32-bits will fail and
thus this smaller value must be less than 32-bits in size.  I'll add a comment.

I don't know why BucketRanges uses size_t.  Probably seemed the right thing at
the time.

+      histogram_data->ranges_ref = ranges_ref;
+      histogram_data->ranges_checksum = bucket_ranges->checksum();
+      histogram_data->counts_ref = counts_ref;
+    } else {
+      histogram_data = nullptr;  // Clear this for proper handling below.
+    }
   }
 
-  size_t ranges_bytes = (bucket_count + 1) * sizeof(HistogramBase::Sample);
-  PersistentMemoryAllocator::Reference ranges_ref =
-      allocator->Allocate(ranges_bytes, kTypeIdRangesArray);
-  PersistentMemoryAllocator::Reference counts_ref =
-      allocator->Allocate(counts_bytes, kTypeIdCountsArray);
-  PersistentMemoryAllocator::Reference histogram_ref =
-      allocator->Allocate(offsetof(PersistentHistogramData, name) +
-                          name.length() + 1, kTypeIdHistogram);
-  HistogramBase::Sample* ranges_data =
-      allocator->GetAsObject<HistogramBase::Sample>(ranges_ref,
-                                                    kTypeIdRangesArray);
-  PersistentHistogramData* histogram_data =
-      allocator->GetAsObject<PersistentHistogramData>(histogram_ref,
-                                                      kTypeIdHistogram);
-
-  // Only continue here if all allocations were successful. If they weren't
-  // there is no way to free the space but that's not really a problem since
-  // the allocations only fail because the space is full and so any future
-  // attempts will also fail.
-  if (counts_ref && ranges_data && histogram_data) {
-    strcpy(histogram_data->name, name.c_str());
-    for (size_t i = 0; i < bucket_ranges->size(); ++i)
-      ranges_data[i] = bucket_ranges->range(i);
-
-    histogram_data->histogram_type = histogram_type;
-    histogram_data->flags = flags;
-    histogram_data->minimum = minimum;
-    histogram_data->maximum = maximum;
-    histogram_data->bucket_count = static_cast<uint32_t>(bucket_count);
-    histogram_data->ranges_ref = ranges_ref;
-    histogram_data->ranges_checksum = bucket_ranges->checksum();
-    histogram_data->counts_ref = counts_ref;
-
+  if (histogram_data) {
     // Create the histogram using resources in persistent memory. This ends up
     // resolving the "ref" values stored in histogram_data instad of just
     // using what is already known above but avoids duplicating the switch
     // statement here and serves as a double-check that everything is
     // correct before commiting the new histogram to persistent space.
     HistogramBase* histogram =
         CreatePersistentHistogram(allocator, histogram_data);
     DCHECK(histogram);
     if (ref_ptr != nullptr)
       *ref_ptr = histogram_ref;
@@ skipping 33 matching lines @@
     while (true) {
       HistogramBase* histogram = GetNextPersistentHistogram(g_allocator, &iter);
       if (!histogram)
         break;
       StatisticsRecorder::RegisterOrDeleteDuplicate(histogram);
     }
   }
 }
 
 }  // namespace base