Clean up SkDynamicAnnotations.

Clean up SkDynamicAnnotations.

    Unprotected reads -> relaxed reads.
    Unprotected write -> relaxed write.

The only unprotected write we had was in SkTraceEvent, which it looks like we nabbed from Chrome at some point and changed only to silence TSAN.  Chrome's version uses AtomicWord / NoBarrier_Load / NoBarrier_Store, which boils down to the same as here, intptr_t / relaxed load / relaxed store.

This leaves one place where we're lying a bit to TSAN, in include/core/SkLazyPtr.h where we're doing a data-dependent consume load.  We're telling TSAN it's consume, but telling any other compiler to compile it as relaxed, given how they all upgrade consume to acquire.  This eliminates a barrier for us on ARM.  How do you guys deal with this?  Just use a consume memory order, take the hit, and hope compilers get smarter one day?

BUG=chromium:465721
No public API changes.
TBR=reed@google.com

Committed: https://skia.googlesource.com/skia/+/172b45518a6a2c9c924cce124dcf27de88b03788

[mtklein_C, 2015-03-10 15:42:55]

mtklein@chromium.org changed reviewers:
+ glider@chromium.org

[Alexander Potapenko, 2015-03-11 09:09:45]

https://codereview.chromium.org/996763002/diff/20001/include/core/SkOnce.h
File include/core/SkOnce.h (right):

https://codereview.chromium.org/996763002/diff/20001/include/core/SkOnce.h#ne...
include/core/SkOnce.h:86: if (!*done) {
For the sake of uniformity this needs to be an atomic relaxed load.

https://codereview.chromium.org/996763002/diff/20001/include/core/SkOnce.h#ne...
include/core/SkOnce.h:118: SkAssertResult(sk_acquire_load(done));
|done| is always true at this point, so it looks like you're just doing an extra
memory load for no reason (unless it's optimized away in the release build, in
which case it should be a relaxed load).

I suggest you remove this line and do an acquire load at line 105 - guess it
should be at least not less correct.

https://codereview.chromium.org/996763002/diff/20001/src/core/SkLazyFnPtr.h
File src/core/SkLazyFnPtr.h (right):

https://codereview.chromium.org/996763002/diff/20001/src/core/SkLazyFnPtr.h#n...
src/core/SkLazyFnPtr.h:50: fn = Choose();
It's possible for Choose() to be called several times from different threads, is
that the contract?

https://codereview.chromium.org/996763002/diff/20001/src/core/SkTraceEvent.h
File src/core/SkTraceEvent.h (right):

https://codereview.chromium.org/996763002/diff/20001/src/core/SkTraceEvent.h#...
src/core/SkTraceEvent.h:764: #define TRACE_EVENT_API_ATOMIC_LOAD(var)
sk_atomic_load(&var, sk_memory_order_relaxed)
Please mind the line length (not sure what is the policy in Skia)

[Alexander Potapenko, 2015-03-11 09:15:41]

Adding dvyukov@ for the consume_load question.

Dima, WDYT about SkStaticLazyPtr implementation in
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i...
The guys want to use a consume load, but it's being promoted to acquire load by
the compilers, so they're using a relaxed load and only tell TSan that it's a
consume one.

[Alexander Potapenko, 2015-03-11 09:25:45]

On 2015/03/11 09:15:41, Alexander Potapenko wrote:
> Adding dvyukov@ for the consume_load question.
> 
> Dima, WDYT about SkStaticLazyPtr implementation in
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i...
> The guys want to use a consume load, but it's being promoted to acquire load
by
> the compilers, so they're using a relaxed load and only tell TSan that it's a
> consume one.

For what it's worth, here's a comparison of acquire and consume loads on a
microbenchmark on x86_64, POWERPC and ARMv7:
http://preshing.com/20140709/the-purpose-of-memory_order_consume-in-cpp11/
The author also claims that the current compilers are unable to produce code for
consume loads efficiently.

[Alexander Potapenko, 2015-03-11 11:32:59]

In an offline discussion Dima told me he's ok with the current implementation
that provides a single consume_load() API function.
If/when this breaks, we'll only need to fix that function, not all the instances
of consume_load replaced with relaxed_load.

Mike, may I ask you to use "#if defined(THREAD_SANITIZER)" to detect you're
running under TSan, instead of checking for "DYNAMIC_ANNOTATIONS_ENABLED"?

[mtklein, 2015-03-11 15:31:20]

mtklein@google.com changed reviewers:
+ mtklein@google.com

[mtklein, 2015-03-11 15:31:21]

All done I think.

https://codereview.chromium.org/996763002/diff/20001/include/core/SkOnce.h
File include/core/SkOnce.h (right):

https://codereview.chromium.org/996763002/diff/20001/include/core/SkOnce.h#ne...
include/core/SkOnce.h:86: if (!*done) {
On 2015/03/11 09:09:44, Alexander Potapenko wrote:
> For the sake of uniformity this needs to be an atomic relaxed load.

Done.

https://codereview.chromium.org/996763002/diff/20001/include/core/SkOnce.h#ne...
include/core/SkOnce.h:118: SkAssertResult(sk_acquire_load(done));
On 2015/03/11 09:09:44, Alexander Potapenko wrote:
> |done| is always true at this point, so it looks like you're just doing an
extra
> memory load for no reason (unless it's optimized away in the release build, in
> which case it should be a relaxed load).
> 
> I suggest you remove this line and do an acquire load at line 105 - guess it
> should be at least not less correct.

Done.  I think we were worried about the case when more than one thread enters
sk_once_slow(), that threads which lost the race to grab the lock might not have
also passed through an acquire barrier before they leave SkOnce.  But clearly
they do pass through an acquire barrier, the lock acquire() operation itself. 
I've updated the comments to explain this.

https://codereview.chromium.org/996763002/diff/20001/src/core/SkLazyFnPtr.h
File src/core/SkLazyFnPtr.h (right):

https://codereview.chromium.org/996763002/diff/20001/src/core/SkLazyFnPtr.h#n...
src/core/SkLazyFnPtr.h:50: fn = Choose();
On 2015/03/11 09:09:44, Alexander Potapenko wrote:
> It's possible for Choose() to be called several times from different threads,
is
> that the contract?

Yes, that's exactly the tradeoff we want.  When you're trying to initialize a
pointer and you can afford to do it multiple times in a race, SkLazyPtr is the
way to go.  If it really must happen exactly once, or the initialization you're
doing can't be encapsulated by a pointer, use SkOnce.

https://codereview.chromium.org/996763002/diff/20001/src/core/SkTraceEvent.h
File src/core/SkTraceEvent.h (right):

https://codereview.chromium.org/996763002/diff/20001/src/core/SkTraceEvent.h#...
src/core/SkTraceEvent.h:764: #define TRACE_EVENT_API_ATOMIC_LOAD(var)
sk_atomic_load(&var, sk_memory_order_relaxed)
On 2015/03/11 09:09:45, Alexander Potapenko wrote:
> Please mind the line length (not sure what is the policy in Skia)

Yep.  We go to 100.  Sadly TRACE_EVENT_API_ATOMIC_STORE did overrun that.  :(

[Alexander Potapenko, 2015-03-12 07:03:59]

On 2015/03/11 15:31:21, mtklein wrote:
> All done I think.
> 
> https://codereview.chromium.org/996763002/diff/20001/include/core/SkOnce.h
> File include/core/SkOnce.h (right):
> 
>
https://codereview.chromium.org/996763002/diff/20001/include/core/SkOnce.h#ne...
> include/core/SkOnce.h:86: if (!*done) {
> On 2015/03/11 09:09:44, Alexander Potapenko wrote:
> > For the sake of uniformity this needs to be an atomic relaxed load.
> 
> Done.
> 
>
https://codereview.chromium.org/996763002/diff/20001/include/core/SkOnce.h#ne...
> include/core/SkOnce.h:118: SkAssertResult(sk_acquire_load(done));
> On 2015/03/11 09:09:44, Alexander Potapenko wrote:
> > |done| is always true at this point, so it looks like you're just doing an
> extra
> > memory load for no reason (unless it's optimized away in the release build,
in
> > which case it should be a relaxed load).
> > 
> > I suggest you remove this line and do an acquire load at line 105 - guess it
> > should be at least not less correct.
> 
> Done.  I think we were worried about the case when more than one thread enters
> sk_once_slow(), that threads which lost the race to grab the lock might not
have
> also passed through an acquire barrier before they leave SkOnce.  But clearly
> they do pass through an acquire barrier, the lock acquire() operation itself. 
> I've updated the comments to explain this.
> 
> https://codereview.chromium.org/996763002/diff/20001/src/core/SkLazyFnPtr.h
> File src/core/SkLazyFnPtr.h (right):
> 
>
https://codereview.chromium.org/996763002/diff/20001/src/core/SkLazyFnPtr.h#n...
> src/core/SkLazyFnPtr.h:50: fn = Choose();
> On 2015/03/11 09:09:44, Alexander Potapenko wrote:
> > It's possible for Choose() to be called several times from different
threads,
> is
> > that the contract?
> 
> Yes, that's exactly the tradeoff we want.  When you're trying to initialize a
> pointer and you can afford to do it multiple times in a race, SkLazyPtr is the
> way to go.  If it really must happen exactly once, or the initialization
you're
> doing can't be encapsulated by a pointer, use SkOnce.
> 
> https://codereview.chromium.org/996763002/diff/20001/src/core/SkTraceEvent.h
> File src/core/SkTraceEvent.h (right):
> 
>
https://codereview.chromium.org/996763002/diff/20001/src/core/SkTraceEvent.h#...
> src/core/SkTraceEvent.h:764: #define TRACE_EVENT_API_ATOMIC_LOAD(var)
> sk_atomic_load(&var, sk_memory_order_relaxed)
> On 2015/03/11 09:09:45, Alexander Potapenko wrote:
> > Please mind the line length (not sure what is the policy in Skia)
> 
> Yep.  We go to 100.  Sadly TRACE_EVENT_API_ATOMIC_STORE did overrun that.  :(

LGTM

[mtklein, 2015-03-12 12:21:18]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2015-03-12 12:21:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/996763002/40001

[commit-bot: I haz the power, 2015-03-12 12:27:53]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/172b45518a6a2c9c924cce124dcf27de88b03788