Use Atomic in IsProfilerTimingEnabled

Use Atomic in IsProfilerTimingEnabled

Using atomics with NoBarrier doesn't change the performance, and prevent
tsan warnings.

R=glider@chromium.org,mark@chromium.org
BUG=327722
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240385

[Alexander Potapenko, 2013-12-12 09:43:14]

LGTM with a nit.

https://codereview.chromium.org/111873003/diff/1/base/tracked_objects.cc
File base/tracked_objects.cc (right):

https://codereview.chromium.org/111873003/diff/1/base/tracked_objects.cc#newc...
base/tracked_objects.cc:75: base::subtle::Release_Store(&timing_enabled,
current_timing_enabled);
This can be a NoBarrier_Store, since you're not acquiring anything.

[qsr, 2013-12-12 09:58:11]

+kcc

[qsr, 2013-12-12 10:06:20]

https://codereview.chromium.org/111873003/diff/1/base/tracked_objects.cc
File base/tracked_objects.cc (right):

https://codereview.chromium.org/111873003/diff/1/base/tracked_objects.cc#newc...
base/tracked_objects.cc:75: base::subtle::Release_Store(&timing_enabled,
current_timing_enabled);
On 2013/12/12 09:43:14, Alexander Potapenko wrote:
> This can be a NoBarrier_Store, since you're not acquiring anything.

Done.

[qsr, 2013-12-12 10:27:01]

-kcc (OoO), +dvyukov@

Dmitriy, would you mind taking a look and check that this is safe with respect
to undefined behavior, knowing that multiple initialization of the variable is
not an issue.

 Thanks.

[Dmitry Vyukov, 2013-12-12 10:59:16]

LGTM

[Mark Mentovai, 2013-12-12 13:51:05]

LGTM.

To clarify: the real difference here is that the static variable is manipulated
through a volatile pointer, and tsan is happy with that?

[qsr, 2013-12-12 13:54:37]

On 2013/12/12 13:51:05, Mark Mentovai wrote:
> LGTM.
> 
> To clarify: the real difference here is that the static variable is
manipulated
> through a volatile pointer, and tsan is happy with that?

 Not exactly from what I understood. Using an atomic will prevent the compiler
to do optimization that might break the code, in particular, it prevents code
reordering as well as other transformation that would only be valid in a non
concurrent environment.

[commit-bot: I haz the power, 2013-12-12 13:55:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qsr@chromium.org/111873003/20001

[Mark Mentovai, 2013-12-12 14:04:27]

qsr wrote:
>  Not exactly from what I understood. Using an atomic will prevent the compiler
> to do optimization that might break the code, in particular, it prevents code
> reordering as well as other transformation that would only be valid in a non
> concurrent environment.

But by what mechanism? There are no special annotations around the atomic type
or functions. The only difference I see is the use of the “volatile” qualifier.
Otherwise the atomicops implementation is perfectly equivalent to what you had
before:

typedef int32 Atomic32;
inline Atomic32 NoBarrier_Load(volatile const Atomic32* ptr) {
  return *ptr;
}
inline void NoBarrier_Store(volatile Atomic32* ptr, Atomic32 value) {
  *ptr = value;
}

I’m perfectly happy if the reason this is considered safe by tsan is that the
variable is only ever accessed as volatile, I just want to know what the
conditions for being considered safe are.

[qsr, 2013-12-12 14:09:15]

On 2013/12/12 14:04:27, Mark Mentovai wrote:
> qsr wrote:
> >  Not exactly from what I understood. Using an atomic will prevent the
compiler
> > to do optimization that might break the code, in particular, it prevents
code
> > reordering as well as other transformation that would only be valid in a non
> > concurrent environment.
> 
> But by what mechanism? There are no special annotations around the atomic type
> or functions. The only difference I see is the use of the “volatile”
qualifier.
> Otherwise the atomicops implementation is perfectly equivalent to what you had
> before:
> 
> typedef int32 Atomic32;
> inline Atomic32 NoBarrier_Load(volatile const Atomic32* ptr) {
>   return *ptr;
> }
> inline void NoBarrier_Store(volatile Atomic32* ptr, Atomic32 value) {
>   *ptr = value;
> }
> 
> I’m perfectly happy if the reason this is considered safe by tsan is that the
> variable is only ever accessed as volatile, I just want to know what the
> conditions for being considered safe are.

 You will have to ask the tsan guys about this. I do not know how NoBarrier_Load
and NoBarrier_Store works under the cover. Like you, I guess that the magic is
in the volatile keyword.

[Alexander Potapenko, 2013-12-12 15:26:49]

Mark, for #if defined(THREAD_SANITIZER) we just use a different implementation
of atomic functions, which notify TSan of each operation, see
base/atomicops_internals_tsan.h
We don't make any assumptions about the variables' volatility, and simply adding
"volatile" to your code doesn't fix anything, because volatile != atomic.
There's a couple of links that describe the difference at
https://sites.google.com/site/kjellhedstrom2/stay-away-from-volatile-in-threa...

[Mark Mentovai, 2013-12-12 16:17:06]

“volatile ≠ atomic,” that’s kind of what I was getting at…

So the non-tsan object code winds up approximately equivalent to what it would
be if you hadn’t used atomicops in this case, and having to use atomicops is
simply a crutch to placate tsan.

Like I said, I’m fine with whatever signal tsan wants to use to determine
whether an operation is safe or not (volatile or otherwise). It does seems kind
of bogus to have barked “the code is incorrect!” to Ben when your solution, for
the non-tsan environment, is practically identical to what he had written in the
first place.

[Alexander Potapenko, 2013-12-12 17:53:59]

On 2013/12/12 16:17:06, Mark Mentovai wrote:
> “volatile ≠ atomic,” that’s kind of what I was getting at…
> 
> So the non-tsan object code winds up approximately equivalent to what it would
> be if you hadn’t used atomicops in this case, and having to use atomicops is
> simply a crutch to placate tsan.
> 
> Like I said, I’m fine with whatever signal tsan wants to use to determine
> whether an operation is safe or not (volatile or otherwise). It does seems
kind
> of bogus to have barked “the code is incorrect!” to Ben when your solution,
for
> the non-tsan environment, is practically identical to what he had written in
the
> first place.

Not that I myself barked on anyone this time, but I indeed think that using
base::subtle for the cases like this increases the code's readability and
portability.
IIUC (the folks in CC know this far better than I) there's no notion of a data
race in C++03 and thus there's no undefined behavior here as long as we stick to
that standard. So, yeah, "incorrect" may be a too strong word.
However the compiler is free to assume that there are no races and do whatever
it wants with the memory accesses to the shared variable (and IIUC the
base::subtle nobarrier atomics are at least guaranteed they're not going to be
moved by the compiler), so the original code could be broken by such
optimizations.
Now consider we move to C++11 someday, which clearly states that non-atomic
concurrent accesses to the same variable are UB. The base::subtle primitives
should be easy enough to translate into std::atomic primitives (or GCC/Clang
atomic builtins) without adding new synchronization errors (not sure there's a
performance-wise optimal 1:1 correspondence between them, but at least every use
of base::subtle can be checked individually). But we won't be able to check all
the existing occurrences of "volatile" or look through the comments to see if
there's something about thread safety in them.

[commit-bot: I haz the power, 2013-12-12 19:29:13]

Change committed as 240385

[dvyukov, 2013-12-13 06:53:44]

On Thu, Dec 12, 2013 at 8:17 PM,  <mark@chromium.org> wrote:
> “volatile ≠ atomic,” that’s kind of what I was getting at…
>
> So the non-tsan object code winds up approximately equivalent to what it
> would
> be if you hadn’t used atomicops in this case, and having to use atomicops is
> simply a crutch to placate tsan.
>
> Like I said, I’m fine with whatever signal tsan wants to use to determine
> whether an operation is safe or not (volatile or otherwise). It does seems
> kind
> of bogus to have barked “the code is incorrect!” to Ben when your solution,
> for
> the non-tsan environment, is practically identical to what he had written in
> the
> first place.


There is a significant difference.
Don't confuse interface and implementation details. If ENOMEM happens
to be 42 right now on your machine, it's not a reason to use 42 in
source code saying that it's bogus to require changing code to
practically identical (while it is indeed completely identical).
Atomic functions not only help tsan and other tools to understand
code. They equally help developers to correctly read the code and
understand the intention. If our platforms or compilers will change
(which happens roughly every 2 weeks), it will be trivially simple to
update only the implementation of atomic functions (as compared to
reviewing and fixing 100'000'000 lines of code).
Program against interface.

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