Change some operations touching counts_[] to be atomic.

Change the operations touching counts_[] and redundant_count_ to be atomic.
This will make it explicit that the counters are accessed without synchronization and will also suppress TSan v2 warnings.

BUG=46840
R=jar@chromium.org,kaiwang@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245997

[Alexander Potapenko, 2013-12-18 10:56:33]

Please take a look.

[jar (doing other things), 2013-12-18 17:19:07]

Q: I thought you needed to declare things atomic in order to use these atomic
ops?  ... and then I thought you had to use them consistently, including storing
values.

Are you really just using this to signal the tsan?... and do no
platform/compiles complain?

[Alexander Potapenko, 2013-12-18 19:04:10]

You're right, I should have ensured all the accesses to atomic vars are
indeed atomic. I'll do.
This isn't enforced by the compiler since Atomic64 is just unsigned long. I
wonder if LLVM can check the consistency of atomic types for us.
On Dec 18, 2013 9:19 PM, <jar@chromium.org> wrote:

> Q: I thought you needed to declare things atomic in order to use these
> atomic
> ops?  ... and then I thought you had to use them consistently, including
> storing
> values.
>
> Are you really just using this to signal the tsan?... and do no
> platform/compiles complain?
>
> https://codereview.chromium.org/116983004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexander Potapenko, 2013-12-24 11:04:20]

Sorry for not getting back to this for a while.

On 2013/12/18 17:19:07, jar wrote:
> Q: I thought you needed to declare things atomic in order to use these atomic
> ops?  ... and then I thought you had to use them consistently, including
storing
> values.
> 
> Are you really just using this to signal the tsan?... and do no
> platform/compiles complain?

I've fixed other accesses to counts_[] and redundant_count_ to be atomic as
well. Please take another look.

No platforms/compiles complain about this, because:
 a) Atomic32 and Atomic64 are typedefs
 b) it's ok to use non-atomic loads of atomic vars in some cases (e.g. if
there's only one thread)

I'm adding NoBarrier_ atomics for two reasons, readability and signaling TSan.

[jar (doing other things), 2013-12-24 18:00:54]

https://codereview.chromium.org/116983004/diff/40001/base/metrics/histogram_s...
File base/metrics/histogram_samples.cc (right):

https://codereview.chromium.org/116983004/diff/40001/base/metrics/histogram_s...
base/metrics/histogram_samples.cc:67: HistogramBase::Count old_redundant_count =
<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:30]

https://codereview.chromium.org/116983004/diff/40001/base/metrics/histogram_s...
File base/metrics/histogram_samples.cc (right):

https://codereview.chromium.org/116983004/diff/40001/base/metrics/histogram_s...
base/metrics/histogram_samples.cc:67: HistogramBase::Count old_redundant_count =
On 2013/12/24 18:00:55, jar (OOO till 1-2-2014) wrote:
> <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.

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]

https://codereview.chromium.org/116983004/diff/40001/base/metrics/histogram_s...
File base/metrics/histogram_samples.cc (right):

https://codereview.chromium.org/116983004/diff/40001/base/metrics/histogram_s...
base/metrics/histogram_samples.cc:67: HistogramBase::Count old_redundant_count =
On 2013/12/25 09:08:31, Alexander Potapenko wrote:
> On 2013/12/24 18:00:55, jar (OOO till 1-2-2014) wrote:
> > <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.
> 
> 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.

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]

Fixed.
This CL isn't really urgent, so please enjoy your vacation.

https://codereview.chromium.org/116983004/diff/40001/base/metrics/histogram_s...
File base/metrics/histogram_samples.cc (right):

https://codereview.chromium.org/116983004/diff/40001/base/metrics/histogram_s...
base/metrics/histogram_samples.cc:67: HistogramBase::Count old_redundant_count =
> 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."
Correct, I've missed that. 
> 
> 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.

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.

[Alexander Potapenko, 2014-01-13 10:37:11]

Jim, can you please take another look? Thanks.

On 2013/12/26 09:26:10, Alexander Potapenko wrote:
> Fixed.
> This CL isn't really urgent, so please enjoy your vacation.
> 
>
https://codereview.chromium.org/116983004/diff/40001/base/metrics/histogram_s...
> File base/metrics/histogram_samples.cc (right):
> 
>
https://codereview.chromium.org/116983004/diff/40001/base/metrics/histogram_s...
> base/metrics/histogram_samples.cc:67: HistogramBase::Count old_redundant_count
=
> > 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."
> Correct, I've missed that. 
> > 
> > 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.
> 
> 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.

[Alexander Potapenko, 2014-01-20 07:25:26]

Ping?

[jar (doing other things), 2014-01-20 18:33:08]

LGTM

Hopefully there are enough unit tests that there are no subtle typos that were
missed.

Thanks!

[commit-bot: I haz the power, 2014-01-21 06:31:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/glider@chromium.org/116983004/330001

[commit-bot: I haz the power, 2014-01-21 07:59:31]

Change committed as 245997