Embed a single sample in histogram metadata.

base/metrics/histogram_samples.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_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);
 
   bool Done() const override;
   void Next() override;
   void Get(HistogramBase::Sample* min,
            HistogramBase::Sample* max,
            HistogramBase::Count* count) const override;
 
@@ skipping 28 matching lines @@
                                     HistogramBase::Sample* max,
                                     HistogramBase::Count* count) const {
   DCHECK(!Done());
   *min = min_;
   *max = max_;
   *count = count_;
 }
 
 }  // 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
 // class itself is in the middle of being constructed. Using it to
 // initialize meta_ is okay because the object now exists and local_meta_
 // is before meta_ in the construction order.
 HistogramSamples::HistogramSamples(uint64_t id)
     : meta_(&local_meta_) {
   meta_->id = id;
 }
 
 HistogramSamples::HistogramSamples(uint64_t id, Metadata* meta)
     : meta_(meta) {
   DCHECK(meta_->id == 0 || meta_->id == id);
 
   // It's possible that |meta| is contained in initialized, read-only memory
   // so it's essential that no write be done in that case.
   if (!meta_->id)
     meta_->id = id;
 }
 
 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);
 }
 
 bool HistogramSamples::AddFromPickle(PickleIterator* iter) {
   int64_t sum;
   HistogramBase::Count redundant_count;
 
   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);
 }
 
 bool HistogramSamples::Serialize(Pickle* pickle) const {
   if (!pickle->WriteInt64(sum()))
     return false;
   if (!pickle->WriteInt(redundant_count()))
     return false;
 
   HistogramBase::Sample min;
   HistogramBase::Sample max;
   HistogramBase::Count count;
   for (std::unique_ptr<SampleCountIterator> it = Iterator(); !it->Done();
        it->Next()) {
     it->Get(&min, &max, &count);
     if (!pickle->WriteInt(min) ||
         !pickle->WriteInt(max) ||
         !pickle->WriteInt(count))
       return false;
   }
   return true;
 }
 
-void HistogramSamples::IncreaseSum(int64_t diff) {
-#ifdef ARCH_CPU_64_BITS
-  subtle::NoBarrier_AtomicIncrement(&meta_->sum, diff);
-#else
-  meta_->sum += diff;
-#endif
+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::IncreaseRedundantCount(HistogramBase::Count diff) {
-  subtle::NoBarrier_AtomicIncrement(&meta_->redundant_count, diff);
+void HistogramSamples::IncreaseSumAndCount(int64_t sum,
+                                           HistogramBase::Count count) {
+#ifdef ARCH_CPU_64_BITS
+  subtle::NoBarrier_AtomicIncrement(&meta_->sum, sum);
+#else
+  meta_->sum += sum;
+#endif
+  subtle::NoBarrier_AtomicIncrement(&meta_->redundant_count, count);
 }
 
 SampleCountIterator::~SampleCountIterator() {}
 
 bool SampleCountIterator::GetBucketIndex(size_t* index) const {
   DCHECK(!Done());
   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