Make leak counters thread-safe

Make leak counters thread-safe and turn them on by default for Debug

Make leak counters implemented with SK_DECLARE_INST_COUNT thread-safe.
Enable the leak counting for Debug builds when Skia is built as a
static library. Having SK_DECLARE_INST_COUNT without
SK_DEFINE_INST_COUNT relies on static variables in member functions
declared in the header files. These might be duplicated in the clients
of the library when Skia is built as a dynamic library, producing
incorrect operation.

Protect the instance counter initialization step (initStep) by
using SkOnce.

Makes SkOnce.h part of the public API, since SkInstCnt is public.

Protect the per-class child list shared variable with a per-class mutex.

Changes the behavior in the way that if the child list has been
"cleaned up", it will still try to create subsequent child lists.

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

[Kimmo Kinnunen, 2013-12-04 13:09:19]

Would not need the problems with the cleanup if the list would be linked list..
Not sure if that's worth it.

[Kimmo Kinnunen, 2013-12-04 13:10:01]

Depends on https://codereview.chromium.org/98703002/

[robertphillips, 2013-12-04 14:01:22]

This looks good to me (and preferable to ripping it all out). I am adding the
threading experts to get their two cents.

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

https://codereview.chromium.org/99483003/diff/1/include/core/SkInstCnt.h#newc...
include/core/SkInstCnt.h:113: if (NULL == SkInstanceCountHelper::GetChildren())
{             \
Why not use PFCheckInstCnt here?

[bsalomon, 2013-12-04 15:04:18]

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

https://codereview.chromium.org/99483003/diff/1/include/core/SkInstCnt.h#newc...
include/core/SkInstCnt.h:47: static bool gInited;                               
        \
Perhaps we should use SK_DECLARE_STATIC_ONCE()/SkOnce() here.

[robertphillips, 2013-12-04 15:17:29]

I retract the "(and preferable to ripping it all out)" comment earlier - I
hadn't looked at the other patch and had the wrong idea about what it was doing.

[bungeman-skia, 2013-12-04 15:24:44]

1) Use SkOnce, it's just as fast as this, but correct.

2) I don't see any changes in this CL around decrementing?

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

https://codereview.chromium.org/99483003/diff/1/include/core/SkInstCnt.h#newc...
include/core/SkInstCnt.h:47: static bool gInited;                               
        \
On 2013/12/04 15:04:19, bsalomon wrote:
> Perhaps we should use SK_DECLARE_STATIC_ONCE()/SkOnce() here.

Seconded.

[Kimmo Kinnunen, 2013-12-05 08:03:22]

> 2) I don't see any changes in this CL around decrementing?

That's intentional? Both inc and dec should be thread-safe. Only non-safe was
adding the check function to the function list?

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

https://codereview.chromium.org/99483003/diff/1/include/core/SkInstCnt.h#newc...
include/core/SkInstCnt.h:47: static bool gInited;                               
        \
On 2013/12/04 15:04:19, bsalomon wrote:
> Perhaps we should use SK_DECLARE_STATIC_ONCE()/SkOnce() here.

Done.

https://codereview.chromium.org/99483003/diff/1/include/core/SkInstCnt.h#newc...
include/core/SkInstCnt.h:113: if (NULL == SkInstanceCountHelper::GetChildren())
{             \
On 2013/12/04 14:01:22, robertphillips wrote:
> Why not use PFCheckInstCnt here?

I was confused with the previous need for separate _TEMPLATE macro, eg. I
assumed typename wouldn't work for non-templated classes. You're right, it
works. 
Done.

[mtklein, 2013-12-05 16:06:53]

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

https://codereview.chromium.org/99483003/diff/40001/include/core/SkInstCnt.h#...
include/core/SkInstCnt.h:112: SkAutoMutexAcquire
ama(SkGetInstCntAddInstChildMutex());        \
Does this one really need to be a global mutex?  Seems like each
SkInstanceCountHelper class could have its own static mutex() method?

[Kimmo Kinnunen, 2013-12-09 07:54:51]

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

https://codereview.chromium.org/99483003/diff/40001/include/core/SkInstCnt.h#...
include/core/SkInstCnt.h:112: SkAutoMutexAcquire
ama(SkGetInstCntAddInstChildMutex());        \
On 2013/12/05 16:06:54, mtklein wrote:
> Does this one really need to be a global mutex?  Seems like each
> SkInstanceCountHelper class could have its own static mutex() method?

Well, they could.. I didn't think they should.

Changed so that each helper has their own mutex.
Done.

[mtklein, 2013-12-10 14:48:41]

On 2013/12/09 07:54:51, kkinnunen wrote:
> https://codereview.chromium.org/99483003/diff/40001/include/core/SkInstCnt.h
> File include/core/SkInstCnt.h (right):
> 
>
https://codereview.chromium.org/99483003/diff/40001/include/core/SkInstCnt.h#...
> include/core/SkInstCnt.h:112: SkAutoMutexAcquire
> ama(SkGetInstCntAddInstChildMutex());        \
> On 2013/12/05 16:06:54, mtklein wrote:
> > Does this one really need to be a global mutex?  Seems like each
> > SkInstanceCountHelper class could have its own static mutex() method?
> 
> Well, they could.. I didn't think they should.
> 
> Changed so that each helper has their own mutex.
> Done.

Thanks.  lgtm

I'm curious why you preferred the global mutex.  Was it to save memory, assuming
the mutex would be mostly uncontested?

[Kimmo Kinnunen, 2013-12-11 07:03:30]

On 2013/12/10 14:48:41, mtklein wrote:
> On 2013/12/09 07:54:51, kkinnunen wrote:
> > On 2013/12/05 16:06:54, mtklein wrote:
> > > Does this one really need to be a global mutex?  Seems like each
> > > SkInstanceCountHelper class could have its own static mutex() method?
> > 
> > Well, they could.. I didn't think they should.
> > 
> > Changed so that each helper has their own mutex.
> > Done.
> 
> Thanks.  lgtm
> I'm curious why you preferred the global mutex.  Was it to save memory,
assuming
> the mutex would be mostly uncontested?

Simpler to reason about, perf/resources usage being the same.

[commit-bot: I haz the power, 2013-12-11 07:03:49]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/kkinnunen@nvidia.com/99483003/60001

[commit-bot: I haz the power, 2013-12-11 07:03:53]

Presubmit check for 99483003-60001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Since the CL is editing public API, you must have an LGTM from one of:
('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org',
'bsalomon@google.com')

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Kimmo Kinnunen, 2013-12-11 07:21:44]

Mike, would it be possible for you to look this one last time? I made a mistake
in the previous patch and posted a version that had the SkPostConfig.h turning
on the inst count for debug. This was not intended, as it probably has some
side-effects in testing chrome..

[robertphillips, 2013-12-11 12:47:12]

Chrome explicitly sets SK_ENABLE_INST_COUNT to 0 so leaving it on in debug won't
impact them. I'm actually in favor of leaving it on. It used to be on all the
time in debug and was only disabled when we moved to running GM with multiple
threads (and it didn't work).

[Kimmo Kinnunen, 2013-12-11 12:59:57]

On 2013/12/11 12:47:12, robertphillips wrote:
> Chrome explicitly sets SK_ENABLE_INST_COUNT to 0 so leaving it on in debug
won't
> impact them. I'm actually in favor of leaving it on. It used to be on all the
> time in debug and was only disabled when we moved to running GM with multiple
> threads (and it didn't work).

Ok. Re-enabled. The patch still needs blessing from public API owner.

[bsalomon, 2013-12-11 15:46:30]

On 2013/12/11 12:59:57, kkinnunen wrote:
> On 2013/12/11 12:47:12, robertphillips wrote:
> > Chrome explicitly sets SK_ENABLE_INST_COUNT to 0 so leaving it on in debug
> won't
> > impact them. I'm actually in favor of leaving it on. It used to be on all
the
> > time in debug and was only disabled when we moved to running GM with
multiple
> > threads (and it didn't work).
> 
> Ok. Re-enabled. The patch still needs blessing from public API owner.

lgtm

[commit-bot: I haz the power, 2013-12-12 06:15:59]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/kkinnunen@nvidia.com/99483003/90001

[commit-bot: I haz the power, 2013-12-12 06:24:33]

Failed to apply the patch.
svn: Commit failed (details follow):
svn: Temporarily unavailable

[commit-bot: I haz the power, 2013-12-12 13:25:24]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/kkinnunen@nvidia.com/99483003/90001

[commit-bot: I haz the power, 2013-12-12 14:00:18]

Change committed as 12635

[robertphillips, 2013-12-12 14:27:40]

This was reverted in r12637 due to compile errors on Mac 10.6 and in Chrome.

Mac 10.6

[09:10:05.291672] ../include/core/SkFlattenable.h: In static member function
'static int SkFlattenable::CheckInstanceCount(int, bool)':
[09:10:05.291727] ../include/core/SkFlattenable.h:68: error: using 'typename'
outside of template
[09:10:05.291785] ../include/core/SkFlattenable.h: In static member function
'static void SkFlattenable::AddInstChild(int (*)(int, bool))':
[09:10:05.291841] ../include/core/SkFlattenable.h:68: error: using 'typename'
outside of template
[09:10:05.291896] In file included from ../include/core/SkBitmap.h:15,
[09:10:05.291951]                  from ../include/core/SkShader.h:12,
[09:10:05.292004]                  from
/Users/chrome-bot/buildbot/skiabot-mac-10_6-compile-005/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_6-GCC-x86-Debug/build/skia/gyp/../include/gpu/GrTextureAccess.h:12,
[09:10:05.292065]                  from
/Users/chrome-bot/buildbot/skiabot-mac-10_6-compile-005/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_6-GCC-x86-Debug/build/skia/gyp/../include/gpu/GrEffect.h:14,
[09:10:05.292122]                  from
/Users/chrome-bot/buildbot/skiabot-mac-10_6-compile-005/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_6-GCC-x86-Debug/build/skia/gyp/../include/gpu/GrEffectStage.h:15,
[09:10:05.292183]                  from
/Users/chrome-bot/buildbot/skiabot-mac-10_6-compile-005/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_6-GCC-x86-Debug/build/skia/gyp/../include/gpu/GrPaint.h:14,
[09:10:05.292240]                  from
/Users/chrome-bot/buildbot/skiabot-mac-10_6-compile-005/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_6-GCC-x86-Debug/build/skia/gyp/../include/gpu/GrContext.h:13,
[09:10:05.292300]                  from
/Users/chrome-bot/buildbot/skiabot-mac-10_6-compile-005/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_6-GCC-x86-Debug/build/skia/gyp/../src/gpu/GrClipMaskCache.h:11,
[09:10:05.292356]                  from
/Users/chrome-bot/buildbot/skiabot-mac-10_6-compile-005/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_6-GCC-x86-Debug/build/skia/gyp/../src/gpu/GrClipMaskManager.h:11,
[09:10:05.292417]                  from
/Users/chrome-bot/buildbot/skiabot-mac-10_6-compile-005/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_6-GCC-x86-Debug/build/skia/gyp/../src/gpu/GrClipMaskManager.cpp:9:
[09:10:05.292474] ../include/core/SkColorTable.h: In static member function
'static int SkColorTable::CheckInstanceCount(int, bool)':
[09:10:05.292531] ../include/core/SkColorTable.h:24: error: using 'typename'
outside of template
[09:10:05.292586] ../include/core/SkColorTable.h: In static member function
'static void SkColorTable::AddInstChild(int (*)(int, bool))':
[09:10:05.292642] ../include/core/SkColorTable.h:24: error: using 'typename'
outside of template
[09:10:05.292698] In file included from ../include/core/SkShader.h:12,
[09:10:05.292751]                  from
/Users/chrome-bot/buildbot/skiabot-mac-10_6-compile-005/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_6-GCC-x86-Debug/build/skia/gyp/../include/gpu/GrTextureAccess.h:12,
[09:10:05.292811]                  from
/Users/chrome-bot/buildbot/skiabot-mac-10_6-compile-005/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_6-GCC-x86-Debug/build/skia/gyp/../include/gpu/GrEffect.h:14,
[09:10:05.292867]                  from
/Users/chrome-bot/buildbot/skiabot-mac-10_6-compile-005/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_6-GCC-x86-Debug/build/skia/gyp/../include/gpu/GrEffectStage.h:15,
[09:10:05.292929]                  from
/Users/chrome-bot/buildbot/skiabot-mac-10_6-compile-005/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_6-GCC-x86-Debug/build/skia/gyp/../include/gpu/GrPaint.h:14,
[09:10:05.292985]                  from
/Users/chrome-bot/buildbot/skiabot-mac-10_6-compile-005/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_6-GCC-x86-Debug/build/skia/gyp/../include/gpu/GrContext.h:13,
[09:10:05.293044]                  from
/Users/chrome-bot/buildbot/skiabot-mac-10_6-compile-005/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_6-GCC-x86-Debug/build/skia/gyp/../src/gpu/GrClipMaskCache.h:11,
[09:10:05.293102]                  from
/Users/chrome-bot/buildbot/skiabot-mac-10_6-compile-005/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_6-GCC-x86-Debug/build/skia/gyp/../src/gpu/GrClipMaskManager.h:11,
[09:10:05.293161]                  from
/Users/chrome-bot/buildbot/skiabot-mac-10_6-compile-005/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_6-GCC-x86-Debug/build/skia/gyp/../src/gpu/GrClipMaskManager.cpp:9:

NaCL

[14:20:10.418558] In file included from ../../include/core/SkPaint.h:18,
[14:20:10.418578]                  from
../../include/core/SkFlattenableBuffers.h:15,
[14:20:10.418603]                  from ../../src/core/SkAnnotation.cpp:11:
[14:20:10.418622] ../../include/core/SkXfermode.h: In static member function
'static int SkXfermode::CheckInstanceCount(int, bool)':
[14:20:10.418643] ../../include/core/SkXfermode.h:33: error: using 'typename'
outside of template
[14:20:10.418669] ../../include/core/SkXfermode.h: In static member function
'static void SkXfermode::AddInstChild(int (*)(int, bool))':
[14:20:10.418689] ../../include/core/SkXfermode.h:33: error: using 'typename'
outside of template

Chromium

[06:13:17.223233] ../../third_party/skia/include/core/SkTypes.h: In copy
constructor ‘SkMutex::SkMutex(const SkMutex&)’:
[06:13:17.223263] ../../third_party/skia/include/core/SkTypes.h:417:5: error:
‘SkNoncopyable::SkNoncopyable(const SkNoncopyable&)’ is private
[06:13:17.223293]
../../third_party/skia/include/core/SkThread_platform.h:170:14: error: within
this context
[06:13:17.223323] In file included from
../../third_party/skia/include/core/SkFlattenable.h:13:0,
[06:13:17.223354]                  from
../../third_party/skia/include/core/SkColorTable.h:14,
[06:13:17.223386]                  from
../../third_party/skia/include/core/SkBitmap.h:15,
[06:13:17.223415]                  from
../../third_party/skia/src/core/SkBitmapProcState.h:13,
[06:13:17.223444]                  from
../../third_party/skia/src/opts/SkBitmapProcState_opts_SSE2.h:10,
[06:13:17.223651]                  from
../../third_party/skia/src/opts/SkBitmapProcState_opts_SSE2.cpp:11:
[06:13:17.223782] ../../third_party/skia/include/core/SkRefCnt.h: In constructor
‘SkRefCntBase::SkInstanceCountHelper::SkInstanceCountHelper()’:
[06:13:17.223911] ../../third_party/skia/include/core/SkRefCnt.h:29:5: note:
synthesized method ‘SkMutex::SkMutex(const SkMutex&)’ first required here 
[06:13:17.224042] FAILED: c++ -MMD -MF
obj/third_party/skia/src/opts/skia_opts.SkXfermode_opts_none.o.d -DANGLE_DX11
-DV8_DEPRECATION_WARNINGS -D_FILE_OFFSET_BITS=64 -DCHROMIUM_BUILD -DUSE_CAIRO=1
-DUSE_GLIB=1 -DUSE_DEFAULT_RENDER_THEME=1 -DUSE_LIBJPEG_TURBO=1 -DUSE_NSS=1
-DUSE_X11=1 -DENABLE_ONE_CLICK_SIGNIN -DGTK_DISABLE_SINGLE_INCLUDES=1
-DUSE_XI2_MT=2 -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_PEPPER_CDMS
-DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH -DENABLE_NOTIFICATIONS
-DUSE_UDEV -DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_STATIC -DENABLE_EGLIMAGE=1
-DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1
-DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1
-DENABLE_BACKGROUND=1 -DENABLE_AUTOMATION=1 -DENABLE_GOOGLE_NOW=1
-DCLD_VERSION=2 -DENABLE_FULL_PRINTING=1 -DENABLE_PRINTING=1
-DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1
-DENABLE_MANAGED_USERS=1 -DENABLE_MDNS=1 -DDYNAMIC_ANNOTATIONS_ENABLED=1
-DWTF_USE_DYNAMIC_ANNOTATIONS=1 -D_DEBUG -D_GLIBCXX_DEBUG=1
-I../../third_party/skia/include/core -I../../third_party/skia/src/core
-I../../third_party/skia/src/opts -I../.. -I../../skia/config -fstack-protector
--param=ssp-buffer-size=4 -pthread -fno-exceptions -fno-strict-aliasing
-Wno-unused-parameter -Wno-missing-field-initializers -fvisibility=hidden -pipe
-fPIC -msse2 -Wno-format -Wno-unused-result -O0 -g -fno-rtti
-fno-threadsafe-statics -fvisibility-inlines-hidden -Wno-deprecated  -c
../../third_party/skia/src/opts/SkXfermode_opts_none.cpp -o
obj/third_party/skia/src/opts/skia_opts.SkXfermode_opts_none.o
[06:13:46.757337] In file included from
../../third_party/skia/include/core/SkThread.h:14:0,
[06:13:46.757474]                  from
../../third_party/skia/include/core/SkRefCnt.h:13,
[06:13:46.757509]                  from
../../third_party/skia/include/core/SkFlattenable.h:13,
[06:13:46.757541]                  from
../../third_party/skia/include/core/SkXfermode.h:13,
[06:13:46.757570]                  from
../../third_party/skia/src/opts/SkXfermode_opts_none.cpp:1:
[06:13:46.757598] ../../third_party/skia/include/core/SkTypes.h: In copy
constructor ‘SkMutex::SkMutex(const SkMutex&)’:
[06:13:46.757627] ../../third_party/skia/include/core/SkTypes.h:417:5: error:
‘SkNoncopyable::SkNoncopyable(const SkNoncopyable&)’ is private
[06:13:46.757653]
../../third_party/skia/include/core/SkThread_platform.h:170:14: error: within
this context
[06:13:46.757679] In file included from
../../third_party/skia/include/core/SkFlattenable.h:13:0,
[06:13:46.757706]                  from
../../third_party/skia/include/core/SkXfermode.h:13,
[06:13:46.757731]                  from
../../third_party/skia/src/opts/SkXfermode_opts_none.cpp:1:
[06:13:46.757755] ../../third_party/skia/include/core/SkRefCnt.h: In constructor
‘SkRefCntBase::SkInstanceCountHelper::SkInstanceCountHelper()’:
[06:13:46.757781] ../../third_party/skia/include/core/SkRefCnt.h:29:5: note:
synthesized method ‘SkMutex::SkMutex(const SkMutex&)’ first required here

[Kimmo Kinnunen, 2013-12-13 14:00:30]

> This was reverted in r12637 due to compile errors on Mac 10.6 and in Chrome.

Thanks for the revert. Sorry for the trouble..

> Mac 10.6
...
> [14:20:10.418669] ../../include/core/SkXfermode.h: In static member function
> 'static void SkXfermode::AddInstChild(int (*)(int, bool))':
> [14:20:10.418689] ../../include/core/SkXfermode.h:33: error: using 'typename'
> outside of template

Fixed these in the update to this patch..

> Chromium
> [06:13:46.757627] ../../third_party/skia/include/core/SkTypes.h:417:5: error:
> ‘SkNoncopyable::SkNoncopyable(const SkNoncopyable&)’ is private
> [06:13:46.757653]
> ../../third_party/skia/include/core/SkThread_platform.h:170:14: error: within
> this context

As far as I can see, the current threading code for Windows only compiles due to
some VC++ bug. 
This change causes the bug to not trigger, and thus compile fails. 

So this fix cannot go in. :(


Could somebody re-open this issue?

It will take some time to resolve the root cause of the blocking issue, though.
The issue is that SkOnce cannot be implemented with non-static mutexes.
Currently it appears as if Windows implementation of the static mutexes is not
thread safe, as the code races to construct the SkBaseMutex.

[robertphillips, 2013-12-13 14:01:51]

should be re-opened

[robertphillips, 2013-12-13 14:02:48]

Let's see if the threading/SkOnce guys have any insight into the Windows issue.

[bungeman-skia, 2013-12-13 16:34:40]

On 2013/12/13 14:02:48, robertphillips wrote:
> Let's see if the threading/SkOnce guys have any insight into the Windows
issue.

This all seems to build and run fine on my machine. It appears the failure on
the Chromium build
http://108.170.217.252:10117/builders/Canary-Chrome-Ubuntu12-Ninja-x86_64-Def...
is due to SK_USE_POSIX_THREADS not being set. In fact, there don't appear to be
any -DSK* on the command line to compile these files, which seems odd.

[bungeman-skia, 2013-12-13 17:23:35]

On 2013/12/13 16:34:40, bungeman1 wrote:
> On 2013/12/13 14:02:48, robertphillips wrote:
> > Let's see if the threading/SkOnce guys have any insight into the Windows
> issue.
> 
> This all seems to build and run fine on my machine. It appears the failure on
> the Chromium build
>
http://108.170.217.252:10117/builders/Canary-Chrome-Ubuntu12-Ninja-x86_64-Def...
> is due to SK_USE_POSIX_THREADS not being set. In fact, there don't appear to
be
> any -DSK* on the command line to compile these files, which seems odd.

The issue here is that the SK_USE_POSIX_THREADS define (as well as a number of
others) are being set in skia_library.gypi instead of skia_common.gypi. This
means that these defines are not seen by the 'opts'. This will need to be fixed
first (and has been an issue for some time, this just shows it up).

[mtklein, 2013-12-13 17:33:02]

On 2013/12/13 17:23:35, bungeman1 wrote:
> On 2013/12/13 16:34:40, bungeman1 wrote:
> > On 2013/12/13 14:02:48, robertphillips wrote:
> > > Let's see if the threading/SkOnce guys have any insight into the Windows
> > issue.
> > 
> > This all seems to build and run fine on my machine. It appears the failure
on
> > the Chromium build
> >
>
http://108.170.217.252:10117/builders/Canary-Chrome-Ubuntu12-Ninja-x86_64-Def...
> > is due to SK_USE_POSIX_THREADS not being set. In fact, there don't appear to
> be
> > any -DSK* on the command line to compile these files, which seems odd.
> 
> The issue here is that the SK_USE_POSIX_THREADS define (as well as a number of
> others) are being set in skia_library.gypi instead of skia_common.gypi. This
> means that these defines are not seen by the 'opts'. This will need to be
fixed
> first (and has been an issue for some time, this just shows it up).

I'm ~halfway through this.  Will hopefully have the CL out for review later
today.

[mtklein, 2013-12-13 19:13:13]

On 2013/12/13 17:33:02, mtklein wrote:
> On 2013/12/13 17:23:35, bungeman1 wrote:
> > On 2013/12/13 16:34:40, bungeman1 wrote:
> > > On 2013/12/13 14:02:48, robertphillips wrote:
> > > > Let's see if the threading/SkOnce guys have any insight into the Windows
> > > issue.
> > > 
> > > This all seems to build and run fine on my machine. It appears the failure
> on
> > > the Chromium build
> > >
> >
>
http://108.170.217.252:10117/builders/Canary-Chrome-Ubuntu12-Ninja-x86_64-Def...
> > > is due to SK_USE_POSIX_THREADS not being set. In fact, there don't appear
to
> > be
> > > any -DSK* on the command line to compile these files, which seems odd.
> > 
> > The issue here is that the SK_USE_POSIX_THREADS define (as well as a number
of
> > others) are being set in skia_library.gypi instead of skia_common.gypi. This
> > means that these defines are not seen by the 'opts'. This will need to be
> fixed
> > first (and has been an issue for some time, this just shows it up).
> 
> I'm ~halfway through this.  Will hopefully have the CL out for review later
> today.

Up at https://codereview.chromium.org/98503006

[Kimmo Kinnunen, 2013-12-16 06:19:03]

On 2013/12/13 17:23:35, bungeman1 wrote:
> On 2013/12/13 16:34:40, bungeman1 wrote:
> > On 2013/12/13 14:02:48, robertphillips wrote:
> > > Let's see if the threading/SkOnce guys have any insight into the Windows
> > issue.
> > 
> > This all seems to build and run fine on my machine. It appears the failure
on
> > the Chromium build
> >
>
http://108.170.217.252:10117/builders/Canary-Chrome-Ubuntu12-Ninja-x86_64-Def...
> > is due to SK_USE_POSIX_THREADS not being set. In fact, there don't appear to
> be
> > any -DSK* on the command line to compile these files, which seems odd.
> 
> The issue here is that the SK_USE_POSIX_THREADS define (as well as a number of
> others) are being set in skia_library.gypi instead of skia_common.gypi. This
> means that these defines are not seen by the 'opts'. This will need to be
fixed
> first (and has been an issue for some time, this just shows it up).

Yes, all this is fixable for unixish builds. Still, the Windows build is the
actual problematic part.

For easier repro, you can comment SK_USE_POSIX_THREADS from gyp in a local skia
build, apply this patch and see the windows build problem on Linux, for example.
(This is the same thing that happened by accident due to the .gypi problems
mentiened above)

[Kimmo Kinnunen, 2013-12-16 12:37:42]

On 2013/12/16 06:19:03, kkinnunen wrote:
> Yes, all this is fixable for unixish builds. Still, the Windows build is the
> actual problematic part.
> For easier repro, you can comment SK_USE_POSIX_THREADS from gyp in a local
skia
> build, apply this patch and see the windows build problem on Linux, for
example.

Hmm, apparently I only _imagined_ the same compile error on Windows. It does not
seem to really happen. So after mtklein's fix to gypi files, maybe this patch
will compile.

There's still something I do not understand with the threads when using
!SK_USE_POSIX_THREADS, but that is a separate issue.

[bungeman-skia, 2014-01-10 19:01:13]

I believe that all of the issues which came up out of this have now been
resolved. Does it make sense to give this another go?

[Kimmo Kinnunen, 2014-01-13 16:54:01]

On 2014/01/10 19:01:13, bungeman1 wrote:
> I believe that all of the issues which came up out of this have now been
> resolved. Does it make sense to give this another go?

Rebased. I still fail to run the trybots, though. Working on that..

[bungeman-skia, 2014-01-13 21:22:02]

On 2014/01/13 16:54:01, kkinnunen wrote:
> On 2014/01/10 19:01:13, bungeman1 wrote:
> > I believe that all of the issues which came up out of this have now been
> > resolved. Does it make sense to give this another go?
> 
> Rebased. I still fail to run the trybots, though. Working on that..

I kicked off the trybots at a more opportune time. Seems like everything is
green.

[commit-bot: I haz the power, 2014-01-14 18:17:59]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/kkinnunen@nvidia.com/99483003/250001

[commit-bot: I haz the power, 2014-01-14 18:42:31]

Change committed as 13068

[djsollen, 2014-01-14 21:55:26]

reverted with the following CL as it was breaking the Android bots.

https://codereview.chromium.org/138683006

[Kimmo Kinnunen, 2014-01-16 21:07:45]

On 2014/01/14 21:55:26, djsollen wrote:
> reverted with the following CL as it was breaking the Android bots.
> 
> https://codereview.chromium.org/138683006

djsollen,
We talked about enabling this only for the static build. Was this what you had
in mind?
Btw, would it be possible to reopen this?

[djsollen, 2014-01-16 21:14:13]

Yes, that is what I was thinking. I think we should file a bug about changing
that behavior and putting a link to it in the SkPostConfig.h where you do that
check.

[Kimmo Kinnunen, 2014-01-17 16:45:10]

Brian, you lgtm'd this before. I wonder if it'd make sense to integrate this
now? The problem that caused the revert (failures on arm) was fixed.

Btw: this changes the BUILDTYPE=Debug test run output with default
SkPostConfig.h settings. 
Changing the output has caused problems with Chromium build before. 
Also, even though the SkPostConfig.h settings was thought to be overridden by
Chromium, they were not before..

[commit-bot: I haz the power, 2014-01-17 17:37:44]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/kkinnunen@nvidia.com/99483003/490001

[commit-bot: I haz the power, 2014-01-17 17:55:06]

Change committed as 13120