Use nobarrier atomics for lossy counters in base::HistogramSamples and base::SampleVector

Use nobarrier atomics for platform-size lossy counters in base::HistogramSamples and base::SampleVector
This should not affect the performance, but will make it explicit that the counter values are expected to be read and written atomically.
This will also suppress the ThreadSanitizer v2 reports on these counters.

BUG=46840
R=jar@chromium.org

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

[jar (doing other things), 2013-08-29 03:16:54]

I've looked at this for a long time... and discussed it... I don't think using
atomics is a good direction (for the int32 items).  It diminishes performance,
but offers no real gain in precision when the "noise" of users turning on/off
machines etc. is factored into the stats.  If we ran on a platform that did not
have 32 bit integers, I could be convinced otherwise.

With regard to the int64 IncreaseSum(), I was not able to locate a users of this
function, and it appears to be dead code.  In general, if such was used, I'd
expect its use to be restricted to a single thread.  I went looking... and
couldn't find the caller to validate :-/.

[Alexander Potapenko, 2013-08-29 09:17:59]

+dvyukov

[Alexander Potapenko, 2013-08-29 09:32:08]

On 2013/08/29 03:16:54, jar wrote:
> I've looked at this for a long time... and discussed it... I don't think using
> atomics is a good direction (for the int32 items).  It diminishes performance,
> but offers no real gain in precision when the "noise" of users turning on/off
> machines etc. is factored into the stats.  If we ran on a platform that did
not
> have 32 bit integers, I could be convinced otherwise.
IIUC nobarrier atomics should not affect the performance here. They do not imply
memory barriers and should be compiled into the same code as "sum += diff".
There are two benefits for using NoBarrier_{Load,Store} instead:
 - they're understood by TSan (it won't report a race between two atomic
accesses; however races between an atomic and non-atomic access will still be
reported)
 - if you choose to change the counters to be 64-bit, the code will stay
correct.
Am I missing something?

> With regard to the int64 IncreaseSum(), I was not able to locate a users of
this
> function, and it appears to be dead code.  In general, if such was used, I'd
> expect its use to be restricted to a single thread.  I went looking... and
> couldn't find the caller to validate :-/.

There are races between IncreaseSum() and IncreaseSum() in base_unittests, see
https://code.google.com/p/chromium/issues/detail?id=46840#c23

[Alexander Potapenko, 2013-08-29 09:40:00]

Another good point from kcc: a lossy counter could be a performance problem
itself, since updating the same variable from two threads causes a cache
ping-pong.

[Alexander Potapenko, 2013-08-29 12:35:05]

Jim, I've changed the patch to touch only the platform-size counters
(HistogramSamples::IncreaseRedundantCount() and SampleVector::Accumulate())

Here's the disassembly for HistogramSamples::IncreaseRedundantCount() with my
patch:

00000000009fef70 <_ZN4base16HistogramSamples22IncreaseRedundantCountEi>:
  9fef70:       8b 47 10                mov    0x10(%rdi),%eax
  9fef73:       01 c6                   add    %eax,%esi
  9fef75:       89 77 10                mov    %esi,0x10(%rdi)
  9fef78:       c3                      retq   

and without it:

00000000009fef70 <_ZN4base16HistogramSamples22IncreaseRedundantCountEi>:
  9fef70:       01 77 10                add    %esi,0x10(%rdi)
  9fef73:       c3                      retq   


Same applies to SampleVector::Accumulate: the difference between the two
versions is that the compiler generates MOV-ADD-MOV for the addition operation
instead of a single ADD.

I don't think there should be a notable performance impact from my change. WDYT?

[jar (doing other things), 2013-08-30 03:17:36]

On 2013/08/29 12:35:05, glider (vacation till Aug 29) wrote:
> Jim, I've changed the patch to touch only the platform-size counters
> (HistogramSamples::IncreaseRedundantCount() and SampleVector::Accumulate())
> 
> Here's the disassembly for HistogramSamples::IncreaseRedundantCount() with my
> patch:
> 
> 00000000009fef70 <_ZN4base16HistogramSamples22IncreaseRedundantCountEi>:
>   9fef70:       8b 47 10                mov    0x10(%rdi),%eax
>   9fef73:       01 c6                   add    %eax,%esi
>   9fef75:       89 77 10                mov    %esi,0x10(%rdi)
>   9fef78:       c3                      retq   
> 
> and without it:
> 
> 00000000009fef70 <_ZN4base16HistogramSamples22IncreaseRedundantCountEi>:
>   9fef70:       01 77 10                add    %esi,0x10(%rdi)
>   9fef73:       c3                      retq   
> 
> 
> Same applies to SampleVector::Accumulate: the difference between the two
> versions is that the compiler generates MOV-ADD-MOV for the addition operation
> instead of a single ADD.
> 
> I don't think there should be a notable performance impact from my change.
WDYT?

Do you have insight into what this does on ARM etc.?

[Alexander Potapenko, 2013-08-30 08:24:06]

> Do you have insight into what this does on ARM etc.?

The implementation of 32-bit NoBarrier_Load and NoBarrier_Store is similar on
all the platforms supported by Chrome: these are just "return *ptr" and "*ptr =
value".
So the only problem is that the compiler may fail to generate the same code for
the following sequence (after inlining):
  temp = *sum_;
  temp += diff;
  *sum_ = temp;

as efficient as:
  *sum_ += diff;

I've checked that Clang generates similar code for both cases on ARM and x86.
The code generated by GCC on x86 differs slightly (see above), but shouldn't
affect the performance much.
I don't have a cross-compiling GCC at hand, so I haven't looked at ARM code
generated by GCC. But I doubt it will differ much.

[Alexander Potapenko, 2013-09-03 10:58:04]

Ping.
Jim, do you still have concerns about the possible performance impact of this
change?

[jar (doing other things), 2013-09-04 00:45:57]

lgtm

[Alexander Potapenko, 2013-09-04 10:33:56]

Committed patchset #2 manually as r221168 (presubmit successful).