Embed a single sample in histogram metadata.

base/metrics/histogram_samples.h

Index: base/metrics/histogram_samples.h
diff --git a/base/metrics/histogram_samples.h b/base/metrics/histogram_samples.h
index 93f6d21c8aabd34eed61dd9c20d2e030cabada8f..9c9716f08c53ef212bfab7c7bdb5a18c4b586bff 100644
--- a/base/metrics/histogram_samples.h
+++ b/base/metrics/histogram_samples.h
@@ -24,8 +24,55 @@ class SampleCountIterator;
 // elements must be of a fixed width to ensure 32/64-bit interoperability.
 // If this structure changes, bump the version number for kTypeIdHistogram
 // in persistent_histogram_allocator.cc.
+//
+// Note that though these samples are individually consistent (through the use
+// of atomic operations on the counts), there is only "eventual consistency"
+// overall when multiple threads are accessing this data. That means that the
+// sum, redundant-count, etc. could be momentarily out-of-sync with the stored
+// counts but will settle to a consistent "steady state" once all threads have
+// exited this code.
 class BASE_EXPORT HistogramSamples {
  public:
+  // A single value and count. To fit within a single atomic on 32-bit build
+  // architectures, both |value| and |count| are reduced in size to 16 bits.
+  // This limits the functionality somewhat but if an entry can't fit then
+  // the full array of samples can be allocated and used.
+  struct SingleSample {
+    int16_t value;

[Alexei Svitkine (slow), 2017/04/18 20:52:20]

uint16_t for both?

Or is there a valid case for them to be negative?

[bcwhite, 2017/04/19 18:13:02]

It was significantly simpler this way because the full-size values are signed. 
Defining these as unsigned made for signed/unsigned additions and comparisons.

I can take another look at it now that the rest of the code is stable if you
think it's worth it.

[bcwhite, 2017/04/19 20:10:22]

Another option is to change it from "value" to "bucket" which would add
(actually restore) determining the bucket index for single samples.

I may have to do this anyway to deal with various add/subtract operations which
use iterators which in turn operate on bucket ranges rather than actual values.

Thinking on it...

+    int16_t count;
+  };
+
+  // A structure for managing an atomic single sample. Because this is generally
+  // used in association with other atomic values, the defined methods use
+  // acquire/release operations to guarantee ordering with outside values.
+  union AtomicSingleSample {
+    AtomicSingleSample() : as_atomic(0) {}
+    AtomicSingleSample(subtle::Atomic32 rhs) : as_atomic(rhs) {}
+
+    // Returns the single value in an atomic manner. This in an "acquire"
+    // load. The returned value isn't shared and thus its parts can be safely
+    // accessed.
+    SingleSample Load() const;
+
+    // Extracts the single value in an atomic manner. If |disable| is true then
+    // the single-sample will be set so it can never accumulate another value.
+    // This is "no barrier" so doesn't enforce ordering with other atomic ops.
+    SingleSample Extract(bool disable);
+
+    // Adds a given count to the held value. If not possible, it returns false
+    // and leaves the internal value unchanged. This in an "acquire/release"
+    // operation. Once extracted/disabled, this always returns false.
+    bool Accumulate(HistogramBase::Sample value, HistogramBase::Count count);
+
+   private:
+    // The actual sample value and count.
+    SingleSample as_parts;
+
+    // The sample as an atomic value. Atomic64 would provide more flexibility
+    // but isn't available on all builds.

[Alexei Svitkine (slow), 2017/04/18 20:52:20]

Please expand these field comments to mention that it's a union - since that's
easy to miss given it's way at the top. 

Also mention kDisabledSingleSample as the special value to mean "disabled".

[bcwhite, 2017/04/19 18:13:02]

Done.

+    subtle::Atomic32 as_atomic;
+  };
+
   struct Metadata {
     // Expected size for 32/64-bit check.
     static constexpr size_t kExpectedInstanceSize = 24;
@@ -58,21 +105,17 @@ class BASE_EXPORT HistogramSamples {
     // might mismatch even when no memory corruption has happened.
     HistogramBase::AtomicCount redundant_count;
 
-    // 4 bytes of padding to explicitly extend this structure to a multiple of
-    // 64-bits. This is required to ensure the structure is the same size on
-    // both 32-bit and 64-bit builds.
-    char padding[4];
+    // A single histogram value and associated count. This allows histograms
+    // that typically report only a single value to not require full storage
+    // to be allocated.
+    AtomicSingleSample single_sample;  // 32 bits
   };
 
   // Because sturctures held in persistent memory must be POD, there can be no

[Alexei Svitkine (slow), 2017/04/18 20:52:20]

Nit: structures is misspelled

[bcwhite, 2017/04/19 18:13:02]

Done.

   // default constructor to clear the fields. This derived class exists just
   // to clear them when being allocated on the heap.
-  struct LocalMetadata : Metadata {
-    LocalMetadata() {
-      id = 0;
-      sum = 0;
-      redundant_count = 0;
-    }
+  struct BASE_EXPORT LocalMetadata : Metadata {
+    LocalMetadata();
   };
 
   explicit HistogramSamples(uint64_t id);
@@ -112,8 +155,15 @@ class BASE_EXPORT HistogramSamples {
   enum Operator { ADD, SUBTRACT };
   virtual bool AddSubtractImpl(SampleCountIterator* iter, Operator op) = 0;
 
-  void IncreaseSum(int64_t diff);
-  void IncreaseRedundantCount(HistogramBase::Count diff);
+  bool AccumulateSingleSample(HistogramBase::Sample value,
+                              HistogramBase::Count count);
+
+  void IncreaseSumAndCount(int64_t sum, HistogramBase::Count count);
+
+  AtomicSingleSample& single_sample() { return meta_->single_sample; }
+  const AtomicSingleSample& single_sample() const {
+    return meta_->single_sample;
+  }
 
  private:
   // In order to support histograms shared through an external memory segment,
@@ -145,6 +195,35 @@ class BASE_EXPORT SampleCountIterator {
   virtual bool GetBucketIndex(size_t* index) const;
 };
 
+class BASE_EXPORT SingleSampleIterator : public SampleCountIterator {
+ public:
+  SingleSampleIterator(HistogramBase::Sample min,
+                       HistogramBase::Sample max,
+                       HistogramBase::Count count);
+  SingleSampleIterator(HistogramBase::Sample min,
+                       HistogramBase::Sample max,
+                       HistogramBase::Count count,
+                       size_t bucket_index);
+  ~SingleSampleIterator() override;
+
+  // SampleCountIterator implementation:

[Alexei Svitkine (slow), 2017/04/18 20:52:20]

Nit: Just // SampleCounterIterator:

(For new code).

[bcwhite, 2017/04/19 18:13:02]

Done.

+  bool Done() const override;
+  void Next() override;
+  void Get(HistogramBase::Sample* min,
+           HistogramBase::Sample* max,
+           HistogramBase::Count* count) const override;
+
+  // SampleVector uses predefined buckets, so iterator can return bucket index.
+  bool GetBucketIndex(size_t* index) const override;
+
+ private:
+  // Information about the single value to return.
+  const HistogramBase::Sample min_;
+  const HistogramBase::Sample max_;
+  const size_t bucket_index_;
+  HistogramBase::Count count_;
+};
+
 }  // namespace base
 
 #endif  // BASE_METRICS_HISTOGRAM_SAMPLES_H_