Embed a single sample in histogram metadata.

base/metrics/histogram_samples.cc

Index: base/metrics/histogram_samples.cc
diff --git a/base/metrics/histogram_samples.cc b/base/metrics/histogram_samples.cc
index 3475cd59f71df439ebd7cd124d4614ccd588b98a..c3b20896ac01a446d6c21ea671a24b8e2d208354 100644
--- a/base/metrics/histogram_samples.cc
+++ b/base/metrics/histogram_samples.cc
@@ -4,13 +4,26 @@
 
 #include "base/metrics/histogram_samples.h"
 
+#include <limits>
+
 #include "base/compiler_specific.h"
+#include "base/numerics/safe_math.h"
 #include "base/pickle.h"
 
 namespace base {
 
 namespace {
 
+// A shorthand constant for the max value of size_t.
+constexpr size_t kSizeMax = std::numeric_limits<size_t>::max();
+
+// A constant stored in an AtomicSingleSample (as_atomic) to indicate that the
+// sample is "disabled" and no further accumulation should be done with it. The
+// value is chosen such that it will be MAX_UINT16 for both |bucket| & |count|,
+// and thus less likely to conflict with real use. Conflicts are explicitly
+// handled in the code but it's worth making them as unlikely as possible.
+constexpr int32_t kDisabledSingleSample = -1;
+
 class SampleCountPickleIterator : public SampleCountIterator {
  public:
   explicit SampleCountPickleIterator(PickleIterator* iter);
@@ -59,6 +72,93 @@ void SampleCountPickleIterator::Get(HistogramBase::Sample* min,
 
 }  // namespace
 
+static_assert(sizeof(HistogramSamples::AtomicSingleSample) ==
+                  sizeof(subtle::Atomic32),
+              "AtomicSingleSample isn't 32 bits");
+
+HistogramSamples::SingleSample HistogramSamples::AtomicSingleSample::Load()
+    const {
+  AtomicSingleSample single_sample = subtle::Acquire_Load(&as_atomic);
+
+  // If the sample was extracted/disabled, it's still zero to the outside.
+  if (single_sample.as_atomic == kDisabledSingleSample)
+    single_sample.as_atomic = 0;
+
+  return single_sample.as_parts;
+}
+
+HistogramSamples::SingleSample HistogramSamples::AtomicSingleSample::Extract(
+    bool disable) {
+  AtomicSingleSample single_sample = subtle::NoBarrier_AtomicExchange(
+      &as_atomic, disable ? kDisabledSingleSample : 0);
+  if (single_sample.as_atomic == kDisabledSingleSample)
+    single_sample.as_atomic = 0;
+  return single_sample.as_parts;
+}
+
+bool HistogramSamples::AtomicSingleSample::Accumulate(
+    size_t bucket,
+    HistogramBase::Count count) {
+  if (count == 0)
+    return true;
+
+  // Convert the parameters to 16-bit variables because it's all 16-bit below.
+  if (count < std::numeric_limits<uint16_t>::min() ||
+      count > std::numeric_limits<uint16_t>::max() ||
+      bucket > std::numeric_limits<uint16_t>::max()) {
+    return false;
+  }
+  uint16_t bucket16 = static_cast<uint16_t>(bucket);
+  uint16_t count16 = static_cast<uint16_t>(count);
+
+  // A local, unshared copy of the single-sample is necessary so the parts
+  // can be manipulated without worrying about atomicity.
+  AtomicSingleSample single_sample;
+
+  subtle::Atomic32 existing;
+  subtle::Atomic32 found;

[Alexei Svitkine (slow), 2017/04/20 19:56:10]

Nit: How about renaming found to updated_value - so it's clearer what it's
referring to (when you use it later).

[bcwhite, 2017/04/20 21:18:07]

I think "updated_value" could be construed as the value _after_ the
CompareAndSwap when in fact it is always the value found during the compare but
before the swap.

[Alexei Svitkine (slow), 2017/04/20 22:11:34]

True, updated_value isn't a great name. How about to make the code clear - you
can declare |found| where you call CompareAndSwap() and add a "bool
value_updated" at the top that you set to "value_updated = (found != existing);"
and loop on while (!value_updated). This makes the code a bit easier to follow.

[bcwhite, 2017/04/21 13:25:31]

Done.  I've also changed the names from existing/found to original/existing with
a comment of how it works.

+  do {
+    existing = subtle::Acquire_Load(&as_atomic);
+    if (existing == kDisabledSingleSample)
+      return false;
+    single_sample.as_atomic = existing;
+    if (single_sample.as_atomic != 0) {
+      // Only the same bucket (parameter and stored) can be counted multiple
+      // times.
+      if (single_sample.as_parts.bucket != bucket16)
+        return false;
+    } else {
+      // The single sample was zero so becomes the |bucket| parameter, the

[Alexei Svitkine (slow), 2017/04/20 19:56:10]

Nit: "single sample" -> "|single_sample|" to make it clear it's referring to the
old value - and not the new single sample.

[bcwhite, 2017/04/20 21:18:07]

Done.

+      // contents of which where checked above to fit in 16 bits.

[Alexei Svitkine (slow), 2017/04/20 19:56:10]

Nit: where -> were

[bcwhite, 2017/04/20 21:18:07]

Done.

+      single_sample.as_parts.bucket = bucket16;
+    }
+
+    // Update count, making sure that it doesn't overflow.
+    CheckedNumeric<uint16_t> new_count(single_sample.as_parts.count);
+    new_count += count16;
+    if (!new_count.AssignIfValid(&single_sample.as_parts.count))
+      return false;
+
+    // Store the updated single-sample back into memory.
+    if (single_sample.as_atomic == kDisabledSingleSample)
+      return false;
+    found = subtle::Release_CompareAndSwap(&as_atomic, existing,
+                                           single_sample.as_atomic);
+  } while (found != existing);
+
+  return true;
+}
+
+bool HistogramSamples::AtomicSingleSample::IsDisabled() const {
+  return subtle::Acquire_Load(&as_atomic) == kDisabledSingleSample;
+}
+
+HistogramSamples::LocalMetadata::LocalMetadata() {
+  // This is the same way it's done for persistent metadata since no ctor
+  // is called for the data members in that case.
+  memset(this, 0, sizeof(*this));
+}
+
 // Don't try to delegate behavior to the constructor below that accepts a
 // Matadata pointer by passing &local_meta_. Such cannot be reliably passed
 // because it has not yet been constructed -- no member variables have; the
@@ -83,9 +183,7 @@ HistogramSamples::HistogramSamples(uint64_t id, Metadata* meta)
 HistogramSamples::~HistogramSamples() {}
 
 void HistogramSamples::Add(const HistogramSamples& other) {
-  IncreaseSum(other.sum());
-  subtle::NoBarrier_AtomicIncrement(&meta_->redundant_count,
-                                    other.redundant_count());
+  IncreaseSumAndCount(other.sum(), other.redundant_count());
   bool success = AddSubtractImpl(other.Iterator().get(), ADD);
   DCHECK(success);
 }
@@ -97,18 +195,14 @@ bool HistogramSamples::AddFromPickle(PickleIterator* iter) {
   if (!iter->ReadInt64(&sum) || !iter->ReadInt(&redundant_count))
     return false;
 
-  IncreaseSum(sum);
-  subtle::NoBarrier_AtomicIncrement(&meta_->redundant_count,
-                                    redundant_count);
+  IncreaseSumAndCount(sum, redundant_count);
 
   SampleCountPickleIterator pickle_iter(iter);
   return AddSubtractImpl(&pickle_iter, ADD);
 }
 
 void HistogramSamples::Subtract(const HistogramSamples& other) {
-  IncreaseSum(-other.sum());
-  subtle::NoBarrier_AtomicIncrement(&meta_->redundant_count,
-                                    -other.redundant_count());
+  IncreaseSumAndCount(-other.sum(), -other.redundant_count());
   bool success = AddSubtractImpl(other.Iterator().get(), SUBTRACT);
   DCHECK(success);
 }
@@ -133,16 +227,25 @@ bool HistogramSamples::Serialize(Pickle* pickle) const {
   return true;
 }
 
-void HistogramSamples::IncreaseSum(int64_t diff) {
+bool HistogramSamples::AccumulateSingleSample(HistogramBase::Sample value,
+                                              HistogramBase::Count count,
+                                              size_t bucket) {
+  if (single_sample().Accumulate(bucket, count)) {
+    // Success. Update the (separate) sum and redundant-count.
+    IncreaseSumAndCount(static_cast<int64_t>(value) * count, count);
+    return true;
+  }
+  return false;
+}
+
+void HistogramSamples::IncreaseSumAndCount(int64_t sum,
+                                           HistogramBase::Count count) {
 #ifdef ARCH_CPU_64_BITS
-  subtle::NoBarrier_AtomicIncrement(&meta_->sum, diff);
+  subtle::NoBarrier_AtomicIncrement(&meta_->sum, sum);
 #else
-  meta_->sum += diff;
+  meta_->sum += sum;
 #endif
-}
-
-void HistogramSamples::IncreaseRedundantCount(HistogramBase::Count diff) {
-  subtle::NoBarrier_AtomicIncrement(&meta_->redundant_count, diff);
+  subtle::NoBarrier_AtomicIncrement(&meta_->redundant_count, count);
 }
 
 SampleCountIterator::~SampleCountIterator() {}
@@ -152,4 +255,46 @@ bool SampleCountIterator::GetBucketIndex(size_t* index) const {
   return false;
 }
 
+SingleSampleIterator::SingleSampleIterator(HistogramBase::Sample min,
+                                           HistogramBase::Sample max,
+                                           HistogramBase::Count count)
+    : SingleSampleIterator(min, max, count, kSizeMax) {}
+
+SingleSampleIterator::SingleSampleIterator(HistogramBase::Sample min,
+                                           HistogramBase::Sample max,
+                                           HistogramBase::Count count,
+                                           size_t bucket_index)
+    : min_(min), max_(max), bucket_index_(bucket_index), count_(count) {}
+
+SingleSampleIterator::~SingleSampleIterator() {}
+
+bool SingleSampleIterator::Done() const {
+  return count_ == 0;
+}
+
+void SingleSampleIterator::Next() {
+  DCHECK(!Done());
+  count_ = 0;
+}
+
+void SingleSampleIterator::Get(HistogramBase::Sample* min,
+                               HistogramBase::Sample* max,
+                               HistogramBase::Count* count) const {
+  DCHECK(!Done());
+  if (min != nullptr)
+    *min = min_;
+  if (max != nullptr)
+    *max = max_;
+  if (count != nullptr)
+    *count = count_;
+}
+
+bool SingleSampleIterator::GetBucketIndex(size_t* index) const {
+  DCHECK(!Done());
+  if (bucket_index_ == kSizeMax)
+    return false;
+  *index = bucket_index_;
+  return true;
+}
+
 }  // namespace base