Port SkRefCnt.h to new SkAtomics.h

Port SkRefCnt.h to new SkAtomics.h

This adds sk_memory_barrier(), implemented using sk_atomic_fetch_add() on an uninitialized variable.  If that becomes a problem we can drop this to the porting layer, using std::atomic_thread_fence() / __atomic_thread_fence() / __sync_synchronize().

The big win is that ref() doesn't generate a memory barrier any more on ARM.
This is an instance of SkSafeRef() in SkPaint(const SkPaint&) after this CL:
     4d0:       684a            ldr     r2, [r1, #4]
     4d2:       6018            str     r0, [r3, #0]
     4d4:       b13a            cbz     r2, 4e6 <_ZN7SkPaintC1ERKS_+0x2e>
     4d6:       1d10            adds    r0, r2, #4
     4d8:       e850 4f00       ldrex   r4, [r0]
     4dc:       3401            adds    r4, #1
     4de:       e840 4500       strex   r5, r4, [r0]
     4e2:       2d00            cmp     r5, #0
     4e4:       d1f8            bne.n   4d8 <_ZN7SkPaintC1ERKS_+0x20>

Here's the before,  pretty much the same with two memory barriers surrounding the ref():
     4d8:       684a            ldr     r2, [r1, #4]
     4da:       6018            str     r0, [r3, #0]
     4dc:       b15a            cbz     r2, 4f6 <_ZN7SkPaintC1ERKS_+0x3e>
     4de:       1d10            adds    r0, r2, #4
     4e0:       f3bf 8f5f       dmb     sy
     4e4:       e850 4f00       ldrex   r4, [r0]
     4e8:       3401            adds    r4, #1
     4ea:       e840 4500       strex   r5, r4, [r0]
     4ee:       2d00            cmp     r5, #0
     4f0:       d1f8            bne.n   4e4 <_ZN7SkPaintC1ERKS_+0x2c>
     4f2:       f3bf 8f5f       dmb     sy

The miscellaneous files in here are just fixups to explicitly include SkMutex.h,
instead of leeching it off SkRefCnt.h.

No public API changes.
TBR=reed@google.com

Build trybots seem hosed.
NOTRY=true

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/7b274c78fbeefa3818af68099545f2839c854847

[mtklein_C, 2015-02-03 16:50:30]

mtklein@chromium.org changed reviewers:
+ bungeman@google.com

[bungeman-skia, 2015-02-03 17:14:47]

https://codereview.chromium.org/896803002/diff/40001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.chromium.org/896803002/diff/40001/include/core/SkRefCnt.h#...
include/core/SkRefCnt.h:76: // An acquire makes sure code in internal_dispose()
doen't happen before the decrement.
nit: doen't?

I liked the way it was clear before that the algorithm itself only needs the
acquire inside this block, mostly because it's needed for exactly the same
reasons the acquire in "unique" is needed. I know it doesn't make any practical
difference, but I'd like this comment to somehow point out more explicitly
somehow that the acquire logically is only needed inside the block, and that
this is the same situation as "unique".

https://codereview.chromium.org/896803002/diff/40001/include/core/SkRefCnt.h#...
include/core/SkRefCnt.h:274: void deref() const { this->unref(); }  // Chrome
prefers to call deref().
I know it has nothing to do with this CL, but I still cringe every time I see
this line. Yes, it's the minimum to magically make this work with Blink, but
Chromium actually wants AddRef/Release (so the comment is wrong, it's Blink that
want's deref). Also, while this is the minimal amount to make things work, Blink
also wants adopted and requireAdoption (see
https://code.google.com/p/chromium/codesearch#chromium/src/skia/config/sk_ref...
).

[mtklein, 2015-02-03 17:27:56]

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

[mtklein, 2015-02-03 17:27:57]

https://codereview.chromium.org/896803002/diff/40001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.chromium.org/896803002/diff/40001/include/core/SkRefCnt.h#...
include/core/SkRefCnt.h:76: // An acquire makes sure code in internal_dispose()
doen't happen before the decrement.
On 2015/02/03 17:14:47, bungeman1 wrote:
> nit: doen't?
> 
> I liked the way it was clear before that the algorithm itself only needs the
> acquire inside this block, mostly because it's needed for exactly the same
> reasons the acquire in "unique" is needed. I know it doesn't make any
practical
> difference, but I'd like this comment to somehow point out more explicitly
> somehow that the acquire logically is only needed inside the block, and that
> this is the same situation as "unique".

Done.

https://codereview.chromium.org/896803002/diff/40001/include/core/SkRefCnt.h#...
include/core/SkRefCnt.h:274: void deref() const { this->unref(); }  // Chrome
prefers to call deref().
On 2015/02/03 17:14:47, bungeman1 wrote:
> I know it has nothing to do with this CL, but I still cringe every time I see
> this line. Yes, it's the minimum to magically make this work with Blink, but
> Chromium actually wants AddRef/Release (so the comment is wrong, it's Blink
that
> want's deref). Also, while this is the minimal amount to make things work,
Blink
> also wants adopted and requireAdoption (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/skia/config/sk_ref...
> ).

Done.

[bungeman-skia, 2015-02-03 18:48:47]

This one comment aside, it seems good this caught what may have been an issue
with unique.

https://codereview.chromium.org/896803002/diff/60001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.chromium.org/896803002/diff/60001/include/core/SkRefCnt.h#...
include/core/SkRefCnt.h:76: if (1 == sk_atomic_fetch_add(&fRefCnt, -1,
sk_memory_order_release)) {
I was actually fine with the old acq_rel here, I was just asking for the comment
to state the actual algorithm. On x86 it shouldn't make a difference (these both
turn into no barriers) but on ARM I'd like to be sure there isn't an extra set
of dmb in here. Do we end up with an extra unwanted dmb here?

[mtklein, 2015-02-03 19:08:27]

https://codereview.chromium.org/896803002/diff/60001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.chromium.org/896803002/diff/60001/include/core/SkRefCnt.h#...
include/core/SkRefCnt.h:76: if (1 == sk_atomic_fetch_add(&fRefCnt, -1,
sk_memory_order_release)) {
On 2015/02/03 18:48:47, bungeman1 wrote:
> I was actually fine with the old acq_rel here, I was just asking for the
comment
> to state the actual algorithm. On x86 it shouldn't make a difference (these
both
> turn into no barriers) but on ARM I'd like to be sure there isn't an extra set
> of dmb in here. Do we end up with an extra unwanted dmb here?

Yep.  :(

I've moved back to acquire / acq_rel now with updated comments.  We'll see how
TSAN likes that.

[mtklein, 2015-02-03 19:36:59]

On 2015/02/03 19:08:27, mtklein wrote:
> https://codereview.chromium.org/896803002/diff/60001/include/core/SkRefCnt.h
> File include/core/SkRefCnt.h (right):
> 
>
https://codereview.chromium.org/896803002/diff/60001/include/core/SkRefCnt.h#...
> include/core/SkRefCnt.h:76: if (1 == sk_atomic_fetch_add(&fRefCnt, -1,
> sk_memory_order_release)) {
> On 2015/02/03 18:48:47, bungeman1 wrote:
> > I was actually fine with the old acq_rel here, I was just asking for the
> comment
> > to state the actual algorithm. On x86 it shouldn't make a difference (these
> both
> > turn into no barriers) but on ARM I'd like to be sure there isn't an extra
set
> > of dmb in here. Do we end up with an extra unwanted dmb here?
> 
> Yep.  :(
> 
> I've moved back to acquire / acq_rel now with updated comments.  We'll see how
> TSAN likes that.

TSAN seems happy now.  Have a look?

[bungeman-skia, 2015-02-03 19:41:20]

If TSAN is happy, everything must be correct, right? lgtm

[mtklein, 2015-02-03 19:42:07]

On 2015/02/03 19:41:20, bungeman1 wrote:
> If TSAN is happy, everything must be correct, right? lgtm

Ha.

[mtklein, 2015-02-03 19:42:29]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2015-02-03 19:42:50]

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

[mtklein, 2015-02-03 20:27:50]

The CQ bit was unchecked by mtklein@google.com

[mtklein, 2015-02-03 20:28:01]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2015-02-03 20:29:09]

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

[mtklein_C, 2015-02-03 20:29:20]

New patchsets have been uploaded after l-g-t-m from bungeman@google.com

[mtklein, 2015-02-03 20:29:26]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2015-02-03 20:29:29]

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

[mtklein, 2015-02-03 21:03:44]

The CQ bit was unchecked by mtklein@google.com

[mtklein, 2015-02-03 21:03:45]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2015-02-03 21:03:49]

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

[mtklein, 2015-02-03 21:38:28]

The CQ bit was unchecked by mtklein@google.com

[mtklein, 2015-02-03 21:38:49]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2015-02-03 21:38:51]

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

[commit-bot: I haz the power, 2015-02-03 21:39:03]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/7b274c78fbeefa3818af68099545f2839c854847

[mtklein, 2015-02-03 22:58:17]

On 2015/02/03 21:39:03, I haz the power (commit-bot) wrote:
> Committed patchset #8 (id:140001) as
> https://skia.googlesource.com/skia/+/7b274c78fbeefa3818af68099545f2839c854847

Early numbers look like this will be a ~10% recording speedup on ARM:

https://perf.skia.org/#2150

[reed1, 2015-02-03 23:00:27]

On 2015/02/03 22:58:17, mtklein wrote:
> On 2015/02/03 21:39:03, I haz the power (commit-bot) wrote:
> > Committed patchset #8 (id:140001) as
> >
https://skia.googlesource.com/skia/+/7b274c78fbeefa3818af68099545f2839c854847
> 
> Early numbers look like this will be a ~10% recording speedup on ARM:
> 
> https://perf.skia.org/#2150

Kick!