Use subtle::NoBarrier* statements for Histogram flags_ field.

Use subtle::NoBarrier* statements for Histogram flags_ field.

This is similar to this CL which provided the same treatment to
some of the other histogram internal fields:
https://codereview.chromium.org/116983004

This field can be accessed from multiple threads which causes
TSAN to report data races. Using NoBarrier instructions makes
this intention explicit and silences the TSAN errors.

Here's some context for the data race and why it's harmless:
https://code.google.com/p/chromium/issues/detail?id=535359#c14

BUG=535359
Committed: https://crrev.com/a74e3d57c0c1feebb1f0bf28a4dfaea03b3734d2
Cr-Commit-Position: refs/heads/master@{#360723}

[Alexei Svitkine (slow), 2015-11-17 22:25:38]

asvitkine@chromium.org changed reviewers:
+ glider@chromium.org, isherman@chromium.org

[Ilya Sherman, 2015-11-17 22:28:29]

I'm not sure that we actually want to use NoBarrier -- is it prohibitively
expensive somehow to use memory barriers?  I agree that the race is typically
benign, but having read things like [1] I suspect that there might corner cases
that we're not considering.

[1]
https://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-coul...

[Alexei Svitkine (slow), 2015-11-17 22:34:14]

Let's see what glider@ says - who works on TSAN and I've also asked to
review this CL (and who authored the similar change to the other parts of
histograms code).

One thing to note is we now check flags() on every histogram Add() call, so
it seems whatever we decided to do for other fields that are accessed this
often (e.g. sums) probably makes sense to do here.

On Tue, Nov 17, 2015 at 5:28 PM, <isherman@chromium.org> wrote:

> I'm not sure that we actually want to use NoBarrier -- is it prohibitively
> expensive somehow to use memory barriers?  I agree that the race is
> typically
> benign, but having read things like [1] I suspect that there might corner
> cases
> that we're not considering.
>
> [1]
>
>
https://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-coul...
>
> https://codereview.chromium.org/1445283004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexander Potapenko, 2015-11-18 11:23:28]

https://codereview.chromium.org/1445283004/diff/1/base/metrics/histogram_base.cc
File base/metrics/histogram_base.cc (right):

https://codereview.chromium.org/1445283004/diff/1/base/metrics/histogram_base...
base/metrics/histogram_base.cc:76: subtle::NoBarrier_Store(&flags_, old_flags |
flags);
The question is whether there is any data that is being concurrently read or
written together with the flags.
For example, if there's a data pointer that's written by one thread before
SetFlags(kSomeFlag) and dereferenced by another thread after checking flags() &
kSomeFlag, there must be a barrier between the accesses to flags_ and to the
pointer (i.e. accesses to flags_ must have acquire/release semantics).
If your flags won't associated data it's fine to keep the accesses nobarrier.

[Alexei Svitkine (slow), 2015-11-18 16:37:12]

https://codereview.chromium.org/1445283004/diff/1/base/metrics/histogram_base.cc
File base/metrics/histogram_base.cc (right):

https://codereview.chromium.org/1445283004/diff/1/base/metrics/histogram_base...
base/metrics/histogram_base.cc:76: subtle::NoBarrier_Store(&flags_, old_flags |
flags);
On 2015/11/18 11:23:28, Alexander Potapenko wrote:
> The question is whether there is any data that is being concurrently read or
> written together with the flags.
> For example, if there's a data pointer that's written by one thread before
> SetFlags(kSomeFlag) and dereferenced by another thread after checking flags()
&
> kSomeFlag, there must be a barrier between the accesses to flags_ and to the
> pointer (i.e. accesses to flags_ must have acquire/release semantics).
> If your flags won't associated data it's fine to keep the accesses nobarrier.

I don't think there's a case where there is associated data that's not already
guarded by a lock.

The only case where there is an associate data is the callback flag, but the
callback itself is accessed after grabbing a lock on StatisticsRecorder. The
flag is just used to ensure that 99.999% cases that don't have a callback set
don't need to do any synchronization. (And inconsistencies between the flag and
the callback are safe - i.e. it's not critical if one doesn't match the other
due to caching for some period of time until a barrier happens elsewhere.)

[Alexei Svitkine (slow), 2015-11-19 16:45:31]

friendly ping

Alexander, WDYT given my explanation? Is this OK?

[Alexander Potapenko, 2015-11-19 16:48:36]

On 2015/11/19 16:45:31, Alexei Svitkine (slow) wrote:
> friendly ping
> 
> Alexander, WDYT given my explanation? Is this OK?

Um, sorry, I forgot to respond.
LGTM.

[Alexei Svitkine (slow), 2015-11-19 22:09:02]

isherman: friendly ping

[Ilya Sherman, 2015-11-19 23:06:50]

On 2015/11/19 22:09:02, Alexei Svitkine (slow) wrote:
> isherman: friendly ping

LGTM given glider's approval.  Thanks!

[Alexei Svitkine (slow), 2015-11-19 23:08:05]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2015-11-19 23:12:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1445283004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1445283004/1

[commit-bot: I haz the power, 2015-11-20 01:18:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-20 01:18:54]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)

[Alexei Svitkine (slow), 2015-11-20 02:09:04]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2015-11-20 02:11:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1445283004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1445283004/1

[commit-bot: I haz the power, 2015-11-20 02:38:04]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-11-20 02:39:13]

Patchset 1 (id:??) landed as
https://crrev.com/a74e3d57c0c1feebb1f0bf28a4dfaea03b3734d2
Cr-Commit-Position: refs/heads/master@{#360723}