Change some operations touching counts_[] to be atomic.

base/metrics/histogram_samples.cc

Index: base/metrics/histogram_samples.cc
===================================================================
--- base/metrics/histogram_samples.cc	(revision 242425)
+++ base/metrics/histogram_samples.cc	(working copy)
@@ -64,7 +64,10 @@
 
 void HistogramSamples::Add(const HistogramSamples& other) {
   sum_ += other.sum();
-  redundant_count_ += other.redundant_count();
+  HistogramBase::Count old_redundant_count =

[jar (doing other things), 2013/12/24 18:00:55]

<sigh>... this is also seemingly atomic... which is supposed to mean you access
it consistently via the atomic methods... which then makes little sense for
automatic (stack) variable.

You probably need a typedef for the array elements, and similar non-atomic for
temporaries, etc.

Maybe something like:
typedef subtle::Atomic32 AtomicCount;     // Used to count samples.
typedef int32 Count;  // Used to manipulate counts in temporaries.

[Alexander Potapenko, 2013/12/25 09:08:31]

I think making accesses to old_redundant_count atomic is an overkill. This is
the case where it's ok to access atomic vars using non-atomic methods, because
they do not escape this thread (even this function) and are very likely to be
placed on a register.
There probably are no strict guidelines for using atomic operations from
base/atomicops.h, but the common sense suggests treating the result of an atomic
load as a basic type, like this is for C++11's std::atomic (see
http://en.cppreference.com/w/cpp/atomic/atomic_load)
I'd actually vote for changing the NoBarrier_Load (and others') return value
type to a non-atomic one, but that's hard because of its extensive use inside
and outside Google. Let us leave it as is.

[jar (doing other things), 2013/12/25 20:12:01]

Quoting from line 19 of atomics.h documentation about atomic ops:

"It is incorrect to make direct assignments to/from an atomic variable.  You
should use one of the Load or Store routines."

If you didn't want to use a typedef, you'd at least change line 67 to:

int32 old_redundant_count = ...

You'd need to do the same on lines 82 and 93.  

You should also stop passing arguments with this atomic type, such as in line
125 and many other places :-(.

Bottom line: You need to distinguish the generic type, from the atomic type,
which is only used in the class to hold samples and a redundant count (and is
always accessed via atomic ops).

IMO, you should use one typedef for the samples that you pass around as
arguments, and a special atomic typedef for the types of the array elements and
redundant count.

[Alexander Potapenko, 2013/12/26 09:26:10]

Correct, I've missed that. 
I've renamed Count to AtomicCount, introduced a new Count and fixed the data
members to be AtomicCount (the local vars became Count automatically).
To avoid intermixing Count and AtomicCount they could also be declared as
one-field structs.

+      subtle::NoBarrier_Load(&redundant_count_);
+  subtle::NoBarrier_Store(&redundant_count_,
+      old_redundant_count + other.redundant_count());
   bool success = AddSubtractImpl(other.Iterator().get(), ADD);
   DCHECK(success);
 }
@@ -76,7 +79,10 @@
   if (!iter->ReadInt64(&sum) || !iter->ReadInt(&redundant_count))
     return false;
   sum_ += sum;
-  redundant_count_ += redundant_count;
+  HistogramBase::Count old_redundant_count =
+      subtle::NoBarrier_Load(&redundant_count_);
+  subtle::NoBarrier_Store(&redundant_count_,
+                          old_redundant_count + redundant_count);
 
   SampleCountPickleIterator pickle_iter(iter);
   return AddSubtractImpl(&pickle_iter, ADD);
@@ -84,13 +90,17 @@
 
 void HistogramSamples::Subtract(const HistogramSamples& other) {
   sum_ -= other.sum();
-  redundant_count_ -= other.redundant_count();
+  HistogramBase::Count old_redundant_count =
+      subtle::NoBarrier_Load(&redundant_count_);
+  subtle::NoBarrier_Store(&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(redundant_count_))
+  if (!pickle->WriteInt64(sum_) ||
+      !pickle->WriteInt(subtle::NoBarrier_Load(&redundant_count_)))
     return false;
 
   HistogramBase::Sample min;
@@ -113,8 +123,8 @@
 }
 
 void HistogramSamples::IncreaseRedundantCount(HistogramBase::Count diff) {
-  base::subtle::NoBarrier_Store(&redundant_count_,
-      base::subtle::NoBarrier_Load(&redundant_count_) + diff);
+  subtle::NoBarrier_Store(&redundant_count_,
+      subtle::NoBarrier_Load(&redundant_count_) + diff);
 }
 
 SampleCountIterator::~SampleCountIterator() {}