Use a spinlock in SkOnce.

Use a spinlock in SkOnce.

SkOnceFlag is now statically initializable on all platforms.

Also adds sk_atomic_cas, used to implement new SkSpinlock.

Going to punt on making SkOnceFlag any smaller (for now, it's 8 bytes).  We could conceivably get it down to two bits, one for done and one for a one-bit spinlock (we'd need atomic-& and atomic-| to make that work, but they appear to be available everywhere).

BUG=skia:1929
Committed: http://code.google.com/p/skia/source/detail?r=12968

[reed1, 2014-01-02 13:53:33]

api LGTM, deferring to bungeman

https://codereview.chromium.org/123093002/diff/1/include/core/SkThread.h
File include/core/SkThread.h (right):

https://codereview.chromium.org/123093002/diff/1/include/core/SkThread.h#newc...
include/core/SkThread.h:53: // This is POD and must be zero-initialized.
Is it possible to add another field, just for debugging, that we assert upon in
SkAutoSpinLock, to ensure that someone zero-initialized the SkSpinLock?
Basically, this comment looks scary.

[mtklein, 2014-01-02 16:28:16]

https://codereview.chromium.org/123093002/diff/1/include/core/SkThread.h
File include/core/SkThread.h (right):

https://codereview.chromium.org/123093002/diff/1/include/core/SkThread.h#newc...
include/core/SkThread.h:53: // This is POD and must be zero-initialized.
On 2014/01/02 13:53:34, reed1 wrote:
> Is it possible to add another field, just for debugging, that we assert upon
in
> SkAutoSpinLock, to ensure that someone zero-initialized the SkSpinLock?
> Basically, this comment looks scary.

Yes indeed.  Done.

[bungeman-skia, 2014-01-07 22:05:41]

Other than the comments below on the implementation, I'm concerned about the use
of pure unyeilding spinlocks, especially on systems with a small number of cpus
(like 2, obviously uniprocessor systems should use 'none', though I'm not sure
that will always happen anyway). There are some places where the 'init' may be
very heavy, for example in the default font case we may end up doing disk io.

I think it would be easier and more correct to just use the mutexes, and remove
the base class (with the TODO) in SkMutex_win.h and clean up the resulting
errors on Windows. If we ever stop supporting WinXP we can use the Windows
native InitOnceExecuteOnce. It would be nice to use std::mutex since it's
supposed to be constexpr, but we'll see if vc++ ever gets around to honoring
that (and it's c++11).

https://codereview.chromium.org/123093002/diff/110001/include/core/SkThread.h
File include/core/SkThread.h (right):

https://codereview.chromium.org/123093002/diff/110001/include/core/SkThread.h...
include/core/SkThread.h:37: *  If *addr == before, set *addr to after and return
true, otherwise return false.
This needs to state the memory barriers provided. Note that this often comes in
_acquire, _release, and _none versions.

https://codereview.chromium.org/123093002/diff/110001/include/core/SkThread.h...
include/core/SkThread.h:57: while (!sk_atomic_cas(&thisIsPrivate, 0, 1)) {
This doesn't need a barrier.

(It doesn't look like you've annotated the barriers provided by sk_atomic_cas,
and there should be comments here about what barriers are required for the
algorithm to work.)

https://codereview.chromium.org/123093002/diff/110001/include/core/SkThread.h...
include/core/SkThread.h:64: SkAssertResult(sk_atomic_cas(&thisIsPrivate, 1, 0));
This needs a store:store/load:store (release) barrier before the store.

[mtklein, 2014-01-08 17:02:17]

I'd like to submit this now with the understanding we will continue to evolve
how we handle SkOnce, locks, etc.

I agree completely that once we drop XP, we should switch SkOnce to
InitOnceExecuteOnce.  And if we're splitting the code anyway, I'm also happy to
switch to pthread_once for everyone else then too.  Heck, maybe C++11 arrives
first and we can skip std::mutex and jump right to std::call_once.

I understand your concern about the spinlock but for now I'd like to see how it
fares.  We're not currently multithreaded in production code, so there's minor
risk there, and it'll be very easy for us to see spinlock contention on any CPU
profile of tests, DM, etc.  We may find we want to parameterize SkOnce on the
lock type, so we can decide which to use case-by-case: spinlocks for quick
new-and-return type stuff, mutexes for things like loading fonts off the disk.

https://codereview.chromium.org/123093002/diff/110001/include/core/SkThread.h
File include/core/SkThread.h (right):

https://codereview.chromium.org/123093002/diff/110001/include/core/SkThread.h...
include/core/SkThread.h:37: *  If *addr == before, set *addr to after and return
true, otherwise return false.
On 2014/01/07 22:05:41, bungeman1 wrote:
> This needs to state the memory barriers provided. Note that this often comes
in
> _acquire, _release, and _none versions.

Thanks for reminding me.  Now done.

I updated _android to use android_atomic_release_cas.  android_atomic_cas wasn't
actually part of the public API, but because it's all header it got sucked in. 
Turns they also have a bizarre return type, returning int 0 on success and
something else otherwise, so I fixed my inverted logic.

_sync and _win already provide a full memory barrier, so no changes needed
there.  _none of course needed no changes.

https://codereview.chromium.org/123093002/diff/110001/include/core/SkThread.h...
include/core/SkThread.h:57: while (!sk_atomic_cas(&thisIsPrivate, 0, 1)) {
On 2014/01/07 22:05:41, bungeman1 wrote:
> This doesn't need a barrier.
> 
> (It doesn't look like you've annotated the barriers provided by sk_atomic_cas,
> and there should be comments here about what barriers are required for the
> algorithm to work.)

Done.  Conceivably this might need a store barrier, right?  Otherwise if it
happened to be the only process running on some CPU, it might never actually see
some other thread on another CPU drop the lock.

sk_atomic_cas actually performs a full barrier in all implementations now, so in
practice it's moot.

https://codereview.chromium.org/123093002/diff/110001/include/core/SkThread.h...
include/core/SkThread.h:64: SkAssertResult(sk_atomic_cas(&thisIsPrivate, 1, 0));
On 2014/01/07 22:05:41, bungeman1 wrote:
> This needs a store:store/load:store (release) barrier before the store.

Done.

https://codereview.chromium.org/123093002/diff/110001/include/core/SkThread.h...
include/core/SkThread.h:64: SkAssertResult(sk_atomic_cas(&thisIsPrivate, 1, 0));
On 2014/01/07 22:05:41, bungeman1 wrote:
> This needs a store:store/load:store (release) barrier before the store.

Done.  Now guaranteed by sk_atomic_cas.

[mtklein, 2014-01-08 17:14:24]

On 2014/01/08 17:02:17, mtklein wrote:
> I'd like to submit this now with the understanding we will continue to evolve
> how we handle SkOnce, locks, etc.
> 
> I agree completely that once we drop XP, we should switch SkOnce to
> InitOnceExecuteOnce.  And if we're splitting the code anyway, I'm also happy
to
> switch to pthread_once for everyone else then too.  Heck, maybe C++11 arrives
> first and we can skip std::mutex and jump right to std::call_once.
> 
> I understand your concern about the spinlock but for now I'd like to see how
it
> fares.  We're not currently multithreaded in production code, so there's minor
> risk there, and it'll be very easy for us to see spinlock contention on any
CPU
> profile of tests, DM, etc.  We may find we want to parameterize SkOnce on the
> lock type, so we can decide which to use case-by-case: spinlocks for quick
> new-and-return type stuff, mutexes for things like loading fonts off the disk.
> 
> https://codereview.chromium.org/123093002/diff/110001/include/core/SkThread.h
> File include/core/SkThread.h (right):
> 
>
https://codereview.chromium.org/123093002/diff/110001/include/core/SkThread.h...
> include/core/SkThread.h:37: *  If *addr == before, set *addr to after and
return
> true, otherwise return false.
> On 2014/01/07 22:05:41, bungeman1 wrote:
> > This needs to state the memory barriers provided. Note that this often comes
> in
> > _acquire, _release, and _none versions.
> 
> Thanks for reminding me.  Now done.
> 
> I updated _android to use android_atomic_release_cas.  android_atomic_cas
wasn't
> actually part of the public API, but because it's all header it got sucked in.

> Turns they also have a bizarre return type, returning int 0 on success and
> something else otherwise, so I fixed my inverted logic.
> 
> _sync and _win already provide a full memory barrier, so no changes needed
> there.  _none of course needed no changes.
> 
>
https://codereview.chromium.org/123093002/diff/110001/include/core/SkThread.h...
> include/core/SkThread.h:57: while (!sk_atomic_cas(&thisIsPrivate, 0, 1)) {
> On 2014/01/07 22:05:41, bungeman1 wrote:
> > This doesn't need a barrier.
> > 
> > (It doesn't look like you've annotated the barriers provided by
sk_atomic_cas,
> > and there should be comments here about what barriers are required for the
> > algorithm to work.)
> 
> Done.  Conceivably this might need a store barrier, right?  Otherwise if it
> happened to be the only process running on some CPU, it might never actually
see
> some other thread on another CPU drop the lock.
> 
> sk_atomic_cas actually performs a full barrier in all implementations now, so
in
> practice it's moot.
> 
>
https://codereview.chromium.org/123093002/diff/110001/include/core/SkThread.h...
> include/core/SkThread.h:64: SkAssertResult(sk_atomic_cas(&thisIsPrivate, 1,
0));
> On 2014/01/07 22:05:41, bungeman1 wrote:
> > This needs a store:store/load:store (release) barrier before the store.
> 
> Done.
> 
>
https://codereview.chromium.org/123093002/diff/110001/include/core/SkThread.h...
> include/core/SkThread.h:64: SkAssertResult(sk_atomic_cas(&thisIsPrivate, 1,
0));
> On 2014/01/07 22:05:41, bungeman1 wrote:
> > This needs a store:store/load:store (release) barrier before the store.
> 
> Done.  Now guaranteed by sk_atomic_cas.

FYI, the Android toolchain had a warning (-Werrored even) that didn't like my {
/*zero initialize everything*/ }, so I just updated SK_INIT_ONCE to be explicit.

[bungeman-skia, 2014-01-08 18:35:43]

With the documentation comments I'll give this a lgtm (for now). I'm still
rather grumpy about the spinlocks. All this back-porting of boost/c++11 stuff is
getting a bit old. Skipping std::mutex is probably a good idea anyway when
possible, as it looks like it's still violating constexpr on vc++.

https://codereview.chromium.org/123093002/diff/190001/include/core/SkThread.h
File include/core/SkThread.h (right):

https://codereview.chromium.org/123093002/diff/190001/include/core/SkThread.h...
include/core/SkThread.h:58: while (!sk_atomic_cas(&thisIsPrivate, 0, 1)) {
I'd still like a comment here that this doesn't require a memory barrier.

https://codereview.chromium.org/123093002/diff/190001/include/core/SkThread.h...
include/core/SkThread.h:65: SkAssertResult(sk_atomic_cas(&thisIsPrivate, 1, 0));
I'd still like a comment here that this requires a release memory barrier.

This is mostly to explicitly document the spinlock algorithm, so that if we end
up changing _cas (to have _none, _release, _acquire, or whatever) we don't lose
this otherwise implicit information which is easy to overlook. This request is
mostly due to frustration I've had looking at other code which did not document
the barriers required.

[mtklein, 2014-01-08 18:48:19]

On 2014/01/08 18:35:43, bungeman1 wrote:
> With the documentation comments I'll give this a lgtm (for now). I'm still
> rather grumpy about the spinlocks. All this back-porting of boost/c++11 stuff
is
> getting a bit old. Skipping std::mutex is probably a good idea anyway when
> possible, as it looks like it's still violating constexpr on vc++.
> 
> https://codereview.chromium.org/123093002/diff/190001/include/core/SkThread.h
> File include/core/SkThread.h (right):
> 
>
https://codereview.chromium.org/123093002/diff/190001/include/core/SkThread.h...
> include/core/SkThread.h:58: while (!sk_atomic_cas(&thisIsPrivate, 0, 1)) {
> I'd still like a comment here that this doesn't require a memory barrier.
> 
>
https://codereview.chromium.org/123093002/diff/190001/include/core/SkThread.h...
> include/core/SkThread.h:65: SkAssertResult(sk_atomic_cas(&thisIsPrivate, 1,
0));
> I'd still like a comment here that this requires a release memory barrier.
> 
> This is mostly to explicitly document the spinlock algorithm, so that if we
end
> up changing _cas (to have _none, _release, _acquire, or whatever) we don't
lose
> this otherwise implicit information which is easy to overlook. This request is
> mostly due to frustration I've had looking at other code which did not
document
> the barriers required.

Sorry.  Hadn't realized you were asking for comments.  Done.

You'll be happy to know that std::once_flag::once_flag() is constexpr.

[commit-bot: I haz the power, 2014-01-08 18:48:39]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/mtklein@google.com/123093002/270001

[commit-bot: I haz the power, 2014-01-08 21:16:00]

Change committed as 12968