Modernize and trim down SkOnce.

Modernize and trim down SkOnce.

The API and implementation are very much simplified.
You may not want to bother reading the diff.

As is our trend, SkOnce now uses <atomic> directly.

Member initialization means we don't need SK_DECLARE_STATIC_ONCE.
SkSpinlock already works this same way.

All uses of the old API taking an external bool* and Lock* were pessimal,
so I have not carried this sort of API forward.  It's simpler, faster,
and more space-efficient to always use this single SkOnce class interface.

SkOnce weighs 2 bytes: a done bool and an SkSpinlock, also a bool internally.
This API refactoring opens up the opportunity to fuse those into a single
three-state byte if we'd like.

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

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1894893002

Committed: https://skia.googlesource.com/skia/+/d9dd4282118fc14eac735e6fa0b3ec53047b457f

[mtklein_C, 2016-04-17 15:05:30]

Description was changed from

==========
Modernize and trim down SkOnce.

The API and implementation are very much simplified.

Member initialization means we don't need SK_DECLARE_STATIC_ONCE.
SkSpinlock already works the same way.

All uses of the old API taking an external bool* and Lock* were pessimal,
so I have not carried this sort of API forward.  It's simpler, faster,
and more space-efficient to always use this single SkOnce class interface.

BUG=skia:
==========

to

==========
Modernize and trim down SkOnce.

The API and implementation are very much simplified.

Member initialization means we don't need SK_DECLARE_STATIC_ONCE.
SkSpinlock already works the same way.

All uses of the old API taking an external bool* and Lock* were pessimal,
so I have not carried this sort of API forward.  It's simpler, faster,
and more space-efficient to always use this single SkOnce class interface.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[mtklein, 2016-04-17 15:12:27]

Description was changed from

==========
Modernize and trim down SkOnce.

The API and implementation are very much simplified.

Member initialization means we don't need SK_DECLARE_STATIC_ONCE.
SkSpinlock already works the same way.

All uses of the old API taking an external bool* and Lock* were pessimal,
so I have not carried this sort of API forward.  It's simpler, faster,
and more space-efficient to always use this single SkOnce class interface.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Modernize and trim down SkOnce.

The API and implementation are very much simplified.
As is our trend, it now uses <atomic> directly.

Member initialization means we don't need SK_DECLARE_STATIC_ONCE.
SkSpinlock already works the same way.

All uses of the old API taking an external bool* and Lock* were pessimal,
so I have not carried this sort of API forward.  It's simpler, faster,
and more space-efficient to always use this single SkOnce class interface.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[mtklein, 2016-04-17 15:16:26]

Description was changed from

==========
Modernize and trim down SkOnce.

The API and implementation are very much simplified.
As is our trend, it now uses <atomic> directly.

Member initialization means we don't need SK_DECLARE_STATIC_ONCE.
SkSpinlock already works the same way.

All uses of the old API taking an external bool* and Lock* were pessimal,
so I have not carried this sort of API forward.  It's simpler, faster,
and more space-efficient to always use this single SkOnce class interface.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Modernize and trim down SkOnce.

The API and implementation are very much simplified.
As is our trend, it now uses <atomic> directly.
You may not want to bother reading the diff; the file's almost completely
different.

Member initialization means we don't need SK_DECLARE_STATIC_ONCE.
SkSpinlock already works the same way.

All uses of the old API taking an external bool* and Lock* were pessimal,
so I have not carried this sort of API forward.  It's simpler, faster,
and more space-efficient to always use this single SkOnce class interface.

SkOnce weighs 2 bytes: a done bool and an SkSpinlock, also a bool internally.
This API refactoring opens up the opportunity to fuse those into a single
three-state byte if we'd like.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[mtklein, 2016-04-17 15:17:20]

Description was changed from

==========
Modernize and trim down SkOnce.

The API and implementation are very much simplified.
As is our trend, it now uses <atomic> directly.
You may not want to bother reading the diff; the file's almost completely
different.

Member initialization means we don't need SK_DECLARE_STATIC_ONCE.
SkSpinlock already works the same way.

All uses of the old API taking an external bool* and Lock* were pessimal,
so I have not carried this sort of API forward.  It's simpler, faster,
and more space-efficient to always use this single SkOnce class interface.

SkOnce weighs 2 bytes: a done bool and an SkSpinlock, also a bool internally.
This API refactoring opens up the opportunity to fuse those into a single
three-state byte if we'd like.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Modernize and trim down SkOnce.

The API and implementation are very much simplified.
You may not want to bother reading the diff.

As is our trend, SkOnce now uses <atomic> directly.

Member initialization means we don't need SK_DECLARE_STATIC_ONCE.
SkSpinlock already works this same way.

All uses of the old API taking an external bool* and Lock* were pessimal,
so I have not carried this sort of API forward.  It's simpler, faster,
and more space-efficient to always use this single SkOnce class interface.

SkOnce weighs 2 bytes: a done bool and an SkSpinlock, also a bool internally.
This API refactoring opens up the opportunity to fuse those into a single
three-state byte if we'd like.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[mtklein_C, 2016-04-17 15:17:39]

The CQ bit was checked by mtklein@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-17 15:17:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1894893002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1894893002/20001

[mtklein_C, 2016-04-17 15:24:00]

Description was changed from

==========
Modernize and trim down SkOnce.

The API and implementation are very much simplified.
You may not want to bother reading the diff.

As is our trend, SkOnce now uses <atomic> directly.

Member initialization means we don't need SK_DECLARE_STATIC_ONCE.
SkSpinlock already works this same way.

All uses of the old API taking an external bool* and Lock* were pessimal,
so I have not carried this sort of API forward.  It's simpler, faster,
and more space-efficient to always use this single SkOnce class interface.

SkOnce weighs 2 bytes: a done bool and an SkSpinlock, also a bool internally.
This API refactoring opens up the opportunity to fuse those into a single
three-state byte if we'd like.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Modernize and trim down SkOnce.

The API and implementation are very much simplified.
You may not want to bother reading the diff.

As is our trend, SkOnce now uses <atomic> directly.

Member initialization means we don't need SK_DECLARE_STATIC_ONCE.
SkSpinlock already works this same way.

All uses of the old API taking an external bool* and Lock* were pessimal,
so I have not carried this sort of API forward.  It's simpler, faster,
and more space-efficient to always use this single SkOnce class interface.

SkOnce weighs 2 bytes: a done bool and an SkSpinlock, also a bool internally.
This API refactoring opens up the opportunity to fuse those into a single
three-state byte if we'd like.

No public API changes.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[mtklein_C, 2016-04-17 15:25:49]

The CQ bit was unchecked by mtklein@chromium.org

[mtklein_C, 2016-04-17 15:28:18]

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

[herb_g, 2016-04-18 14:29:51]

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

https://codereview.chromium.org/1894893002/diff/20001/include/private/SkOnce....
include/private/SkOnce.h:29: fDone.store(true, std::memory_order_release);
Should the memory release from the spinlock cover this release?

[mtklein, 2016-04-18 14:41:22]

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

[mtklein, 2016-04-18 14:41:23]

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

https://codereview.chromium.org/1894893002/diff/20001/include/private/SkOnce....
include/private/SkOnce.h:29: fDone.store(true, std::memory_order_release);
On 2016/04/18 at 14:29:51, herb_g wrote:
> Should the memory release from the spinlock cover this release?

As I understand it, the synchronizes-with edge here is only guaranteed if we
acquire and release on the same atomic object, here fDone.  The fast path reader
does not read or write the atomic fLock object, so there's no way to use that to
create the synchronizes-with edge.

Just keep in mind everything inside the first if is done approximately once, and
everything inside the second if is done exactly once.  There's not much benefit
to tweaking the stock double-checked locking formula except if we can find a way
to remove the outermost acquire.

[herb_g, 2016-04-18 14:53:10]

lgtm

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

https://codereview.chromium.org/1894893002/diff/20001/include/private/SkOnce....
include/private/SkOnce.h:29: fDone.store(true, std::memory_order_release);
On 2016/04/18 14:41:23, mtklein wrote:
> On 2016/04/18 at 14:29:51, herb_g wrote:
> > Should the memory release from the spinlock cover this release?
> 
> As I understand it, the synchronizes-with edge here is only guaranteed if we
> acquire and release on the same atomic object, here fDone.  The fast path
reader
> does not read or write the atomic fLock object, so there's no way to use that
to
> create the synchronizes-with edge.
> 
> Just keep in mind everything inside the first if is done approximately once,
and
> everything inside the second if is done exactly once.  There's not much
benefit
> to tweaking the stock double-checked locking formula except if we can find a
way
> to remove the outermost acquire.

Acknowledged.

[mtklein, 2016-04-18 15:07:47]

Description was changed from

==========
Modernize and trim down SkOnce.

The API and implementation are very much simplified.
You may not want to bother reading the diff.

As is our trend, SkOnce now uses <atomic> directly.

Member initialization means we don't need SK_DECLARE_STATIC_ONCE.
SkSpinlock already works this same way.

All uses of the old API taking an external bool* and Lock* were pessimal,
so I have not carried this sort of API forward.  It's simpler, faster,
and more space-efficient to always use this single SkOnce class interface.

SkOnce weighs 2 bytes: a done bool and an SkSpinlock, also a bool internally.
This API refactoring opens up the opportunity to fuse those into a single
three-state byte if we'd like.

No public API changes.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Modernize and trim down SkOnce.

The API and implementation are very much simplified.
You may not want to bother reading the diff.

As is our trend, SkOnce now uses <atomic> directly.

Member initialization means we don't need SK_DECLARE_STATIC_ONCE.
SkSpinlock already works this same way.

All uses of the old API taking an external bool* and Lock* were pessimal,
so I have not carried this sort of API forward.  It's simpler, faster,
and more space-efficient to always use this single SkOnce class interface.

SkOnce weighs 2 bytes: a done bool and an SkSpinlock, also a bool internally.
This API refactoring opens up the opportunity to fuse those into a single
three-state byte if we'd like.

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

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[mtklein, 2016-04-18 15:07:50]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2016-04-18 15:07:55]

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

[commit-bot: I haz the power, 2016-04-18 15:09:13]

Description was changed from

==========
Modernize and trim down SkOnce.

The API and implementation are very much simplified.
You may not want to bother reading the diff.

As is our trend, SkOnce now uses <atomic> directly.

Member initialization means we don't need SK_DECLARE_STATIC_ONCE.
SkSpinlock already works this same way.

All uses of the old API taking an external bool* and Lock* were pessimal,
so I have not carried this sort of API forward.  It's simpler, faster,
and more space-efficient to always use this single SkOnce class interface.

SkOnce weighs 2 bytes: a done bool and an SkSpinlock, also a bool internally.
This API refactoring opens up the opportunity to fuse those into a single
three-state byte if we'd like.

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

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Modernize and trim down SkOnce.

The API and implementation are very much simplified.
You may not want to bother reading the diff.

As is our trend, SkOnce now uses <atomic> directly.

Member initialization means we don't need SK_DECLARE_STATIC_ONCE.
SkSpinlock already works this same way.

All uses of the old API taking an external bool* and Lock* were pessimal,
so I have not carried this sort of API forward.  It's simpler, faster,
and more space-efficient to always use this single SkOnce class interface.

SkOnce weighs 2 bytes: a done bool and an SkSpinlock, also a bool internally.
This API refactoring opens up the opportunity to fuse those into a single
three-state byte if we'd like.

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

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/d9dd4282118fc14eac735e6fa0b3ec53047b457f
==========

[commit-bot: I haz the power, 2016-04-18 15:09:14]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/d9dd4282118fc14eac735e6fa0b3ec53047b457f

[bungeman-skia, 2016-04-18 15:19:00]

https://codereview.chromium.org/1894893002/diff/20001/src/core/SkTaskGroup.cpp
File src/core/SkTaskGroup.cpp (right):

https://codereview.chromium.org/1894893002/diff/20001/src/core/SkTaskGroup.cp...
src/core/SkTaskGroup.cpp:32: static SkOnce once;
It is not entirely clear to me how using member initializers solves the issue of
initializing this value.

[mtklein, 2016-04-18 15:27:44]

https://codereview.chromium.org/1894893002/diff/20001/src/core/SkTaskGroup.cpp
File src/core/SkTaskGroup.cpp (right):

https://codereview.chromium.org/1894893002/diff/20001/src/core/SkTaskGroup.cp...
src/core/SkTaskGroup.cpp:32: static SkOnce once;
On 2016/04/18 at 15:19:00, bungeman1 wrote:
> It is not entirely clear to me how using member initializers solves the issue
of initializing this value.

It makes the default constructor for SkOnce constexpr.  The SkOnce comes
pre-initialized.

[bungeman-skia, 2016-04-18 15:44:33]

On 2016/04/18 15:27:44, mtklein wrote:
> https://codereview.chromium.org/1894893002/diff/20001/src/core/SkTaskGroup.cpp
> File src/core/SkTaskGroup.cpp (right):
> 
>
https://codereview.chromium.org/1894893002/diff/20001/src/core/SkTaskGroup.cp...
> src/core/SkTaskGroup.cpp:32: static SkOnce once;
> On 2016/04/18 at 15:19:00, bungeman1 wrote:
> > It is not entirely clear to me how using member initializers solves the
issue
> of initializing this value.
> 
> It makes the default constructor for SkOnce constexpr.  The SkOnce comes
> pre-initialized.

Hmmm... yes. Can we state that constraint explicitly both here and in
SkSpinlock, like

constexpr SkOnce() = default;

We're relying on this for correctness, so it would be nice to be explicit about
it and have the compiler yell at anyone who violates it.

Of course, we can't while supporting vs2013 because this won't compile there (I
think). So on vs2013 this is racy if thread safe statics are turned off, but we
claim not to have any customers that need that anymore? Once we don't care to
build on vs2013 at all, we can mark the default constructor explicitly as
constexpr though.

[mtklein, 2016-04-18 16:40:18]

On 2016/04/18 at 15:44:33, bungeman wrote:
> On 2016/04/18 15:27:44, mtklein wrote:
> >
https://codereview.chromium.org/1894893002/diff/20001/src/core/SkTaskGroup.cpp
> > File src/core/SkTaskGroup.cpp (right):
> > 
> >
https://codereview.chromium.org/1894893002/diff/20001/src/core/SkTaskGroup.cp...
> > src/core/SkTaskGroup.cpp:32: static SkOnce once;
> > On 2016/04/18 at 15:19:00, bungeman1 wrote:
> > > It is not entirely clear to me how using member initializers solves the
issue
> > of initializing this value.
> > 
> > It makes the default constructor for SkOnce constexpr.  The SkOnce comes
> > pre-initialized.
> 
> Hmmm... yes. Can we state that constraint explicitly both here and in
SkSpinlock, like
> 
> constexpr SkOnce() = default;
> 
> We're relying on this for correctness, so it would be nice to be explicit
about it and have the compiler yell at anyone who violates it.
> 
> Of course, we can't while supporting vs2013 because this won't compile there
(I think). So on vs2013 this is racy if thread safe statics are turned off, but
we claim not to have any customers that need that anymore? Once we don't care to
build on vs2013 at all, we can mark the default constructor explicitly as
constexpr though.

Yep, generally agreed.  I would very much like to write constexpr SkOnce() =
default, and constexpr SkSpinlock() = default, and one day constexpr SkMutex() =
default, constexpr SkSemaphore() = default, etc.  I don't even particularly care
so much about the = default once we can sanely write constexpr.

I have not confirmed one way or the other if vs2013 compiles this as racy
runtime code or as pre-intialized.  At this point we're just catching up with
customers already on 2015.