Reorganize histograms for persistence.

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 "base/compiler_specific.h"
 #include "base/pickle.h"
 
 namespace base {
@@ skipping 41 matching lines @@
                                     HistogramBase::Sample* max,
                                     HistogramBase::Count* count) const {
   DCHECK(!Done());
   *min = min_;
   *max = max_;
   *count = count_;
 }
 
 }  // namespace
 
-HistogramSamples::HistogramSamples() : sum_(0), redundant_count_(0) {}
+HistogramSamples::HistogramSamples(uint64_t id)
+    : meta_(&local_meta_) {
+  SetMetadataId(id);
+}
+
+HistogramSamples::HistogramSamples(uint64_t id, Metadata* meta)
+    : meta_(meta) {
+  SetMetadataId(id);
+}
 
 HistogramSamples::~HistogramSamples() {}
 
+void HistogramSamples::SetMetadataId(uint64_t id) {
+  CHECK(meta_->id == 0 || meta_->id == id);

[Alexei Svitkine (slow), 2015/12/09 23:03:40]

Make this a DCHECK(). CHECKs() are generally reserved for security issues or
debugging production crashes.

Alternatively, if you still would like the safety of a CHECK during development,
you can keep it as-is but add a TODO to convert to a DCHECK that you can address
once the implementation is finalized and there are no issues.

[bcwhite, 2015/12/10 14:51:33]

Done.

+  *const_cast<uint64_t*>(&meta_->id) = id;

[Alexei Svitkine (slow), 2015/12/09 23:03:39]

Nit: Remove the const qualifier on the field and make this a regular assignment.

Alternatively, you can keep the field const and initialize local_meta_ with id
in its constructor. Then, in the case where a Metadata* is passed, you just do
the CHECK and not the assignment, since at that point it would already have been
assigned.

[bcwhite, 2015/12/10 14:51:33]

Given that the assignment constraints have been shuffled externally, there's
really no necessity in making the id unchangeable.  I'll remove the const.

+}
+
+// Despite using atomic operations, the increment/add actions below are *not*
+// atomic! Race conditions may cause loss of samples or even completely corrupt
+// the 64-bit sum on 32-bit machines. This is done intentionally to reduce the
+// cost of these operations that could be executed in performance-significant
+//  points of the code.
+//
+// TODO(bcwhite): Gather quantitative information as to the cost of using
+// proper atomic increments and improve either globally or for those histograms
+// that really need it.
+
 void HistogramSamples::Add(const HistogramSamples& other) {
-  sum_ += other.sum();
+  meta_->sum += other.sum();
+
   HistogramBase::Count old_redundant_count =
-      subtle::NoBarrier_Load(&redundant_count_);
-  subtle::NoBarrier_Store(&redundant_count_,
+      subtle::NoBarrier_Load(&meta_->redundant_count);
+  subtle::NoBarrier_Store(&meta_->redundant_count,
       old_redundant_count + other.redundant_count());
   bool success = AddSubtractImpl(other.Iterator().get(), ADD);
   DCHECK(success);
 }
 
 bool HistogramSamples::AddFromPickle(PickleIterator* iter) {
-  int64 sum;
+  int64_t sum;
   HistogramBase::Count redundant_count;
 
   if (!iter->ReadInt64(&sum) || !iter->ReadInt(&redundant_count))
     return false;
-  sum_ += sum;
+
+  meta_->sum += sum;
+
   HistogramBase::Count old_redundant_count =
-      subtle::NoBarrier_Load(&redundant_count_);
-  subtle::NoBarrier_Store(&redundant_count_,
+      subtle::NoBarrier_Load(&meta_->redundant_count);
+  subtle::NoBarrier_Store(&meta_->redundant_count,
                           old_redundant_count + redundant_count);
 
   SampleCountPickleIterator pickle_iter(iter);
   return AddSubtractImpl(&pickle_iter, ADD);
 }
 
 void HistogramSamples::Subtract(const HistogramSamples& other) {
-  sum_ -= other.sum();
+  meta_->sum -= other.sum();
+
   HistogramBase::Count old_redundant_count =
-      subtle::NoBarrier_Load(&redundant_count_);
-  subtle::NoBarrier_Store(&redundant_count_,
+      subtle::NoBarrier_Load(&meta_->redundant_count);
+  subtle::NoBarrier_Store(&meta_->redundant_count,
                           old_redundant_count - other.redundant_count());
   bool success = AddSubtractImpl(other.Iterator().get(), SUBTRACT);
   DCHECK(success);
 }
 
 bool HistogramSamples::Serialize(Pickle* pickle) const {
-  if (!pickle->WriteInt64(sum_) ||
-      !pickle->WriteInt(subtle::NoBarrier_Load(&redundant_count_)))
+  if (!pickle->WriteInt64(meta_->sum))
+    return false;
+  if (!pickle->WriteInt(subtle::NoBarrier_Load(&meta_->redundant_count)))
     return false;
 
   HistogramBase::Sample min;
   HistogramBase::Sample max;
   HistogramBase::Count count;
   for (scoped_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 diff) {
-  sum_ += diff;
+void HistogramSamples::IncreaseSum(int64_t diff) {
+  meta_->sum += diff;
 }
 
 void HistogramSamples::IncreaseRedundantCount(HistogramBase::Count diff) {
-  subtle::NoBarrier_Store(&redundant_count_,
-      subtle::NoBarrier_Load(&redundant_count_) + diff);
+  subtle::NoBarrier_Store(&meta_->redundant_count,
+      subtle::NoBarrier_Load(&meta_->redundant_count) + diff);
 }
 
 SampleCountIterator::~SampleCountIterator() {}
 
 bool SampleCountIterator::GetBucketIndex(size_t* index) const {
   DCHECK(!Done());
   return false;
 }
 
 }  // namespace base