Sketch of SK_ONCE

Sketch of SK_ONCE

BUG=
R=bungeman@google.com

Committed: https://code.google.com/p/skia/source/detail?r=11674

[mtklein, 2013-10-08 22:03:49]

Please take an early look?

[bungeman-skia, 2013-10-08 22:34:58]

https://codereview.chromium.org/26563002/diff/7001/src/core/SkOnce.h
File src/core/SkOnce.h (right):

https://codereview.chromium.org/26563002/diff/7001/src/core/SkOnce.h#newcode10
src/core/SkOnce.h:10: static bool sk_once##name##ready = false;      \
nit: can we put some '__' before and after name? I just like readable symbols,
even if they're auto generated.

https://codereview.chromium.org/26563002/diff/7001/src/core/SkOnce.h#newcode15
src/core/SkOnce.h:15: inline static void compiler_barrier() {
note that on Windows this is _ReadWriteBarrier, not that tsan seems to be there
yet.

https://codereview.chromium.org/26563002/diff/7001/src/core/SkOnce.h#newcode21
src/core/SkOnce.h:21: asm volatile("dmb" ::: "memory");
note that android has android_memory_barrier and android_compiler_barrier in
cutils/atomic-inline.h (of course, these can only be used with the framework).

https://codereview.chromium.org/26563002/diff/7001/src/core/SkOnce.h#newcode37
src/core/SkOnce.h:37: inline static void acquire_barrier() {
of course, all of this goes in some future cleaned up SkThread.h :-)

https://codereview.chromium.org/26563002/diff/7001/src/core/SkOnce.h#newcode48
src/core/SkOnce.h:48: release_barrier();
I know that we know why these barriers are where they are, but it would be nice
to have a short comment here about what the barrier is doing (ensuring ready is
set to true after once) and maybe a larger comment above with the entire thing,
since we're splitting this into two for the speed. I suppose that's part of the
TODO: docs.

https://codereview.chromium.org/26563002/diff/7001/src/core/SkOnce.h#newcode58
src/core/SkOnce.h:58: #if SK_HAS_COMPILER_FEATURE(thread_sanitizer)
SK_HAS_COMPILER_FEATURE is only available on clang, and newer gccs support tsan.
I think we need an actual flag for this.

https://codereview.chromium.org/26563002/diff/7001/src/core/SkOnce.h#newcode70
src/core/SkOnce.h:70: acquire_barrier();
A comment like "Ensure that we see the effects of 'once' as having run" or
something like that.

[mtklein, 2013-10-09 15:22:25]

PTAL?

https://codereview.chromium.org/26563002/diff/7001/src/core/SkOnce.h
File src/core/SkOnce.h (right):

https://codereview.chromium.org/26563002/diff/7001/src/core/SkOnce.h#newcode10
src/core/SkOnce.h:10: static bool sk_once##name##ready = false;      \
On 2013/10/08 22:34:59, bungeman1 wrote:
> nit: can we put some '__' before and after name? I just like readable symbols,
> even if they're auto generated.

You betcha.  Done.  Took the opportunity to rename ready to done, as it sort of
makes more sense in the do-this-once context.

https://codereview.chromium.org/26563002/diff/7001/src/core/SkOnce.h#newcode15
src/core/SkOnce.h:15: inline static void compiler_barrier() {
On 2013/10/08 22:34:59, bungeman1 wrote:
> note that on Windows this is _ReadWriteBarrier, not that tsan seems to be
there
> yet.

Done.

https://codereview.chromium.org/26563002/diff/7001/src/core/SkOnce.h#newcode21
src/core/SkOnce.h:21: asm volatile("dmb" ::: "memory");
On 2013/10/08 22:34:59, bungeman1 wrote:
> note that android has android_memory_barrier and android_compiler_barrier in
> cutils/atomic-inline.h (of course, these can only be used with the framework).

Yeah.  Given that ARM doesn't quite imply Android, and especially that Android
definitely doesn't imply ARM, I want to keep this CPU specific for now rather
than going the roundabout way of checking the OS then having that code check the
CPU.  I'm happy to let SkThread handle this once that's suitably refactored.

https://codereview.chromium.org/26563002/diff/7001/src/core/SkOnce.h#newcode37
src/core/SkOnce.h:37: inline static void acquire_barrier() {
On 2013/10/08 22:34:59, bungeman1 wrote:
> of course, all of this goes in some future cleaned up SkThread.h :-)

Agreed.  I've added a note to remind us.

https://codereview.chromium.org/26563002/diff/7001/src/core/SkOnce.h#newcode48
src/core/SkOnce.h:48: release_barrier();
On 2013/10/08 22:34:59, bungeman1 wrote:
> I know that we know why these barriers are where they are, but it would be
nice
> to have a short comment here about what the barrier is doing (ensuring ready
is
> set to true after once) and maybe a larger comment above with the entire
thing,
> since we're splitting this into two for the speed. I suppose that's part of
the
> TODO: docs.

Yeah, that was the TODO.  How's this read to you now?

https://codereview.chromium.org/26563002/diff/7001/src/core/SkOnce.h#newcode58
src/core/SkOnce.h:58: #if SK_HAS_COMPILER_FEATURE(thread_sanitizer)
On 2013/10/08 22:34:59, bungeman1 wrote:
> SK_HAS_COMPILER_FEATURE is only available on clang, and newer gccs support
tsan.
> I think we need an actual flag for this.

Done.

https://codereview.chromium.org/26563002/diff/7001/src/core/SkOnce.h#newcode70
src/core/SkOnce.h:70: acquire_barrier();
On 2013/10/08 22:34:59, bungeman1 wrote:
> A comment like "Ensure that we see the effects of 'once' as having run" or
> something like that.

Done.

[robertphillips, 2013-10-09 15:31:13]

nits

https://codereview.chromium.org/26563002/diff/16001/src/core/SkOnce.h
File src/core/SkOnce.h (right):

https://codereview.chromium.org/26563002/diff/16001/src/core/SkOnce.h#newcode1
src/core/SkOnce.h:1: #ifndef SkOnce_DEFINED
Header?

https://codereview.chromium.org/26563002/diff/16001/tests/OnceTest.cpp
File tests/OnceTest.cpp (right):

https://codereview.chromium.org/26563002/diff/16001/tests/OnceTest.cpp#newcode1
tests/OnceTest.cpp:1: #include "SkOnce.h"
Header

[mtklein, 2013-10-09 15:33:57]

https://codereview.chromium.org/26563002/diff/16001/src/core/SkOnce.h
File src/core/SkOnce.h (right):

https://codereview.chromium.org/26563002/diff/16001/src/core/SkOnce.h#newcode1
src/core/SkOnce.h:1: #ifndef SkOnce_DEFINED
On 2013/10/09 15:31:13, robertphillips wrote:
> Header?

Done.

https://codereview.chromium.org/26563002/diff/16001/tests/OnceTest.cpp
File tests/OnceTest.cpp (right):

https://codereview.chromium.org/26563002/diff/16001/tests/OnceTest.cpp#newcode1
tests/OnceTest.cpp:1: #include "SkOnce.h"
On 2013/10/09 15:31:13, robertphillips wrote:
> Header

Done.

[bungeman-skia, 2013-10-09 15:57:28]

lgtm, except for one formatting thing

https://codereview.chromium.org/26563002/diff/26001/gyp/common_conditions.gypi
File gyp/common_conditions.gypi (right):

https://codereview.chromium.org/26563002/diff/26001/gyp/common_conditions.gyp...
gyp/common_conditions.gypi:289: 'conditions' : [[
could we put this second '[' at the beginning of the next line? I think that's
more the style of the rest of the file, and alos lines up with how the
brackets/braces are closed below.

[mtklein, 2013-10-09 16:04:12]

https://codereview.chromium.org/26563002/diff/26001/gyp/common_conditions.gypi
File gyp/common_conditions.gypi (right):

https://codereview.chromium.org/26563002/diff/26001/gyp/common_conditions.gyp...
gyp/common_conditions.gypi:289: 'conditions' : [[
On 2013/10/09 15:57:28, bungeman1 wrote:
> could we put this second '[' at the beginning of the next line? I think that's
> more the style of the rest of the file, and alos lines up with how the
> brackets/braces are closed below.

Done.

[mtklein, 2013-10-09 16:11:35]

On 2013/10/09 16:04:12, mtklein wrote:
> https://codereview.chromium.org/26563002/diff/26001/gyp/common_conditions.gypi
> File gyp/common_conditions.gypi (right):
> 
>
https://codereview.chromium.org/26563002/diff/26001/gyp/common_conditions.gyp...
> gyp/common_conditions.gypi:289: 'conditions' : [[
> On 2013/10/09 15:57:28, bungeman1 wrote:
> > could we put this second '[' at the beginning of the next line? I think
that's
> > more the style of the rest of the file, and alos lines up with how the
> > brackets/braces are closed below.
> 
> Done.

Works great on my desktop and phone, and the fussy Mac and Windows trybots are
green (or red for other reasons)... here we go!

[mtklein, 2013-10-09 16:12:27]

Committed patchset #11 manually as r11674 (presubmit successful).