Let DM run unit tests.

Let DM run unit tests.
  - refactor GYPs and a few flags
  - make GPU tests grab a thread-local GrContextFactory when needed as we do in DM for GMs
  - add a few more UI features to make DM more like tests

I believe this makes the program 'tests' obsolete.

It should be somewhat faster to run the two sets together than running the old binaries serially: 
  - serial: tests 20s (3m18s CPU), dm 21s (3m01s CPU)
  - together: 27s (6m21s CPU)

Next up is to incorporate benches.  I'm only planning there on a single-pass sanity check, so that won't obsolete the program 'bench' just yet.

Tested: out/Debug/tests && out/Debug/dm && echo ok
BUG=skia:

Committed: http://code.google.com/p/skia/source/detail?r=13586

Committed: http://code.google.com/p/skia/source/detail?r=13592

[bsalomon, 2014-02-24 20:23:14]

I think to really replace tests on the bots with this we'll have to come up with
an alternate solution for this:

- make GPU tests grab a thread-local GrContextFactory when needed as we do in DM
for GMs 


due to the issues with TLS ordering we've discussed.

https://codereview.chromium.org/178273002/diff/150001/tests/Test.cpp
File tests/Test.cpp (right):

https://codereview.chromium.org/178273002/diff/150001/tests/Test.cpp#newcode25
tests/Test.cpp:25: DEFINE_string2(resourcePath, i, "resources", "directory for
test resources.");
Does this mean the same flag is used for test and gm resources? Do we actually
use the same dir to hold them today?

[mtklein, 2014-02-24 20:36:12]

Does this make the status quo worse for thread-locals?

https://codereview.chromium.org/178273002/diff/150001/tests/Test.cpp
File tests/Test.cpp (right):

https://codereview.chromium.org/178273002/diff/150001/tests/Test.cpp#newcode25
tests/Test.cpp:25: DEFINE_string2(resourcePath, i, "resources", "directory for
test resources.");
On 2014/02/24 20:23:14, bsalomon wrote:
> Does this mean the same flag is used for test and gm resources? Do we actually
> use the same dir to hold them today?

Sort of.  Right now one is --resources and the other is --resourcePath.  They
both default to "resources", which is where we store all resources for both GMs
and tests.

Let me see if I can merge these two flags cleanly.

[bsalomon, 2014-02-24 20:38:16]

On 2014/02/24 20:36:12, mtklein wrote:
> Does this make the status quo worse for thread-locals?
> 


For DM, no, but IIRC tests currently runs all the GPU tests on the main thread
where there is a static, non-TLS GrContextFactory.

[mtklein, 2014-02-24 20:44:31]

On 2014/02/24 20:38:16, bsalomon wrote:
> On 2014/02/24 20:36:12, mtklein wrote:
> > Does this make the status quo worse for thread-locals?
> > 
> 
> 
> For DM, no, but IIRC tests currently runs all the GPU tests on the main thread
> where there is a static, non-TLS GrContextFactory.

Sorry for double post.  Forgot to put the first one on review thread.

Right, but that's only because I hadn't figured out how to use the thread-locals
to run them off main thread when I wrote that part of tests.  Is there something
specific to tests that we should be worried beyond all tests passing?

PS 8 now uses a single --resources/-i flag for test and GM resources.

[mtklein, 2014-02-24 20:51:17]

>> I was thinking that the potential for exit time crashes would block using DM
in place of tests on the test bots.

Exit time crashes aren't special?  You can still get stack traces, debug them in
GDB, etc.

For debugging, dm --nogms --gpuThreads 0 is pretty much identical in execution
to current mode of running tests, except that the GrContextFactory's destructor
is delayed to just after main returns instead of just before main returns.  I'm
not sure I see that that's a big difference.

[bsalomon, 2014-02-24 21:02:39]

On 2014/02/24 20:51:17, mtklein wrote:
> >> I was thinking that the potential for exit time crashes would block using
DM
> in place of tests on the test bots.
> 
> Exit time crashes aren't special?  You can still get stack traces, debug them
in
> GDB, etc.
> 
> For debugging, dm --nogms --gpuThreads 0 is pretty much identical in execution
> to current mode of running tests, except that the GrContextFactory's
destructor
> is delayed to just after main returns instead of just before main returns. 
I'm
> not sure I see that that's a big difference.


All I'm saying is that the crashes will make the bots turn red if moved to using
dm for the unit test step on the builders. I'm not trying to say we shouldn't
land this change, just that tests can't be fully replaced by this since the bots
won't be able to run it yet.

I think the TLS order will still cause crashes in the single threaded case since
ANGLE's TLS var will be killed before GrContextFactory is destroyed.

[mtklein, 2014-02-24 21:05:28]

On 2014/02/24 21:02:39, bsalomon wrote:
> On 2014/02/24 20:51:17, mtklein wrote:
> > >> I was thinking that the potential for exit time crashes would block using
> DM
> > in place of tests on the test bots.
> > 
> > Exit time crashes aren't special?  You can still get stack traces, debug
them
> in
> > GDB, etc.
> > 
> > For debugging, dm --nogms --gpuThreads 0 is pretty much identical in
execution
> > to current mode of running tests, except that the GrContextFactory's
> destructor
> > is delayed to just after main returns instead of just before main returns. 
> I'm
> > not sure I see that that's a big difference.
> 
> 
> All I'm saying is that the crashes will make the bots turn red if moved to
using
> dm for the unit test step on the builders. I'm not trying to say we shouldn't
> land this change, just that tests can't be fully replaced by this since the
bots
> won't be able to run it yet.
> 
> I think the TLS order will still cause crashes in the single threaded case
since
> ANGLE's TLS var will be killed before GrContextFactory is destroyed.

Oh, I see.  I've actually updated skiatest::Test to use threadlocal
GrContextFactories everywhere, so we should be able to see tests crash with
trybots.  I should see the ANGLE bot fail?

[bsalomon, 2014-02-24 21:09:47]

On 2014/02/24 21:05:28, mtklein wrote:
> On 2014/02/24 21:02:39, bsalomon wrote:
> > On 2014/02/24 20:51:17, mtklein wrote:
> > > >> I was thinking that the potential for exit time crashes would block
using
> > DM
> > > in place of tests on the test bots.
> > > 
> > > Exit time crashes aren't special?  You can still get stack traces, debug
> them
> > in
> > > GDB, etc.
> > > 
> > > For debugging, dm --nogms --gpuThreads 0 is pretty much identical in
> execution
> > > to current mode of running tests, except that the GrContextFactory's
> > destructor
> > > is delayed to just after main returns instead of just before main returns.

> > I'm
> > > not sure I see that that's a big difference.
> > 
> > 
> > All I'm saying is that the crashes will make the bots turn red if moved to
> using
> > dm for the unit test step on the builders. I'm not trying to say we
shouldn't
> > land this change, just that tests can't be fully replaced by this since the
> bots
> > won't be able to run it yet.
> > 
> > I think the TLS order will still cause crashes in the single threaded case
> since
> > ANGLE's TLS var will be killed before GrContextFactory is destroyed.
> 
> Oh, I see.  I've actually updated skiatest::Test to use threadlocal
> GrContextFactories everywhere, so we should be able to see tests crash with
> trybots.  I should see the ANGLE bot fail?

I think so... I was seeing crashes in ANGLE builds of DM when I ran them on my
windows z620.

The problem was we'd make a gl* call under ~GrContext(). Angle's impl of the gl
call would try to get the current context from a TLS var that had already been
unregistered.

[mtklein, 2014-02-24 23:44:14]

On 2014/02/24 21:09:47, bsalomon wrote:
> On 2014/02/24 21:05:28, mtklein wrote:
> > On 2014/02/24 21:02:39, bsalomon wrote:
> > > On 2014/02/24 20:51:17, mtklein wrote:
> > > > >> I was thinking that the potential for exit time crashes would block
> using
> > > DM
> > > > in place of tests on the test bots.
> > > > 
> > > > Exit time crashes aren't special?  You can still get stack traces, debug
> > them
> > > in
> > > > GDB, etc.
> > > > 
> > > > For debugging, dm --nogms --gpuThreads 0 is pretty much identical in
> > execution
> > > > to current mode of running tests, except that the GrContextFactory's
> > > destructor
> > > > is delayed to just after main returns instead of just before main
returns.
> 
> > > I'm
> > > > not sure I see that that's a big difference.
> > > 
> > > 
> > > All I'm saying is that the crashes will make the bots turn red if moved to
> > using
> > > dm for the unit test step on the builders. I'm not trying to say we
> shouldn't
> > > land this change, just that tests can't be fully replaced by this since
the
> > bots
> > > won't be able to run it yet.
> > > 
> > > I think the TLS order will still cause crashes in the single threaded case
> > since
> > > ANGLE's TLS var will be killed before GrContextFactory is destroyed.
> > 
> > Oh, I see.  I've actually updated skiatest::Test to use threadlocal
> > GrContextFactories everywhere, so we should be able to see tests crash with
> > trybots.  I should see the ANGLE bot fail?
> 
> I think so... I was seeing crashes in ANGLE builds of DM when I ran them on my
> windows z620.
> 
> The problem was we'd make a gl* call under ~GrContext(). Angle's impl of the
gl
> call would try to get the current context from a TLS var that had already been
> unregistered.

So, ANGLE bot failed but not for this CL.  Bad GM comparisons (running gm, not
dm).

In any case, I've uploaded PS9.  It's me giving up for now, making all GPU code
single threaded, though not necessarily on the main thread.  DM runs GPU GMs and
GPU tests on a separate thread; tests still runs them on the main thread.  The
GrContextFactory is destroyed before main ends either way: it's on the stack in
tests, and in DM it's tied to the lifetime of the TaskRunner.  No new TLS.

[tfarina, 2014-02-25 02:47:07]

https://codereview.chromium.org/178273002/diff/220001/gyp/tests.gyp
File gyp/tests.gyp (right):

https://codereview.chromium.org/178273002/diff/220001/gyp/tests.gyp#newcode12
gyp/tests.gyp:12: 'tests.gypi',
why? why have a separate gyp include file? what problem this solves?

[mtklein, 2014-02-25 15:05:00]

https://codereview.chromium.org/178273002/diff/220001/gyp/tests.gyp
File gyp/tests.gyp (right):

https://codereview.chromium.org/178273002/diff/220001/gyp/tests.gyp#newcode12
gyp/tests.gyp:12: 'tests.gypi',
On 2014/02/25 02:47:08, tfarina wrote:
> why? why have a separate gyp include file? what problem this solves? 

This lets us compile and link this list of object files into two or more
different binaries.

Ordinarily this sort of thing would be solved by making a static library that's
linked into the binaries that need it, but that interacts poorly with global
constructors, which we use to register tests, GMs, and benchmarks.  It looks to
the linker like no code from that library is used, so it drops it all.

We have an analogous setup for GMs, which are linked into GM, DM, and SampleApp.

[mtklein, 2014-02-25 15:05:44]

On 2014/02/25 15:05:00, mtklein wrote:
> https://codereview.chromium.org/178273002/diff/220001/gyp/tests.gyp
> File gyp/tests.gyp (right):
> 
> https://codereview.chromium.org/178273002/diff/220001/gyp/tests.gyp#newcode12
> gyp/tests.gyp:12: 'tests.gypi',
> On 2014/02/25 02:47:08, tfarina wrote:
> > why? why have a separate gyp include file? what problem this solves? 
> 
> This lets us compile and link this list of object files into two or more
> different binaries.
> 
> Ordinarily this sort of thing would be solved by making a static library
that's
> linked into the binaries that need it, but that interacts poorly with global
> constructors, which we use to register tests, GMs, and benchmarks.  It looks
to
> the linker like no code from that library is used, so it drops it all.
> 
> We have an analogous setup for GMs, which are linked into GM, DM, and
SampleApp.

I forgot: actually they're linked into GM, DM, SampleApp, and bench.

[tfarina, 2014-02-25 16:29:57]

https://codereview.chromium.org/178273002/diff/220001/gyp/tests.gyp
File gyp/tests.gyp (right):

https://codereview.chromium.org/178273002/diff/220001/gyp/tests.gyp#newcode12
gyp/tests.gyp:12: 'tests.gypi',
On 2014/02/25 15:05:01, mtklein wrote:
> On 2014/02/25 02:47:08, tfarina wrote:
> > why? why have a separate gyp include file? what problem this solves? 
> 
> This lets us compile and link this list of object files into two or more
> different binaries.
> 
> Ordinarily this sort of thing would be solved by making a static library
that's
> linked into the binaries that need it, but that interacts poorly with global
> constructors, which we use to register tests, GMs, and benchmarks.
doesn't gtest has this too (global constructors)?

>  It looks to
> the linker like no code from that library is used, so it drops it all.
> 
> We have an analogous setup for GMs, which are linked into GM, DM, and
SampleApp.

for me this is just creating a mess (imo), you might have a different view
though, since you take that route.

For me explicit deps (listed on 'dependencies' list) are much easier to reason
about than playing with gyp includes and building different source files list in
other libraries.

[mtklein, 2014-02-25 16:47:00]

https://codereview.chromium.org/178273002/diff/220001/gyp/tests.gyp
File gyp/tests.gyp (right):

https://codereview.chromium.org/178273002/diff/220001/gyp/tests.gyp#newcode12
gyp/tests.gyp:12: 'tests.gypi',
On 2014/02/25 16:29:58, tfarina wrote:
> On 2014/02/25 15:05:01, mtklein wrote:
> > On 2014/02/25 02:47:08, tfarina wrote:
> > > why? why have a separate gyp include file? what problem this solves? 
> > 
> > This lets us compile and link this list of object files into two or more
> > different binaries.
> > 
> > Ordinarily this sort of thing would be solved by making a static library
> that's
> > linked into the binaries that need it, but that interacts poorly with global
> > constructors, which we use to register tests, GMs, and benchmarks.
> doesn't gtest has this too (global constructors)?
> 
> >  It looks to
> > the linker like no code from that library is used, so it drops it all.
> > 
> > We have an analogous setup for GMs, which are linked into GM, DM, and
> SampleApp.
> 
> for me this is just creating a mess (imo), you might have a different view
> though, since you take that route.
> 
> For me explicit deps (listed on 'dependencies' list) are much easier to reason
> about than playing with gyp includes and building different source files list
in
> other libraries.

Yes, gtest works the same way.

We agree that this is a mess.  For now, this works, and no other approach we
know does yet.

If you'd like to follow along with discussion of this general problem, you'll
find it at http://code.google.com/p/skia/issues/detail?id=2142.

[bsalomon, 2014-02-25 16:49:37]

lgtm

[mtklein, 2014-02-25 18:16:49]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-02-25 18:16:55]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/mtklein@chromium.org/178273002/220001

[tfarina, 2014-02-25 19:03:59]

On 2014/02/25 16:47:00, mtklein wrote:
> https://codereview.chromium.org/178273002/diff/220001/gyp/tests.gyp
> File gyp/tests.gyp (right):
> 
> https://codereview.chromium.org/178273002/diff/220001/gyp/tests.gyp#newcode12
> gyp/tests.gyp:12: 'tests.gypi',
> On 2014/02/25 16:29:58, tfarina wrote:
> > On 2014/02/25 15:05:01, mtklein wrote:
> > > On 2014/02/25 02:47:08, tfarina wrote:
> > > > why? why have a separate gyp include file? what problem this solves? 
> > > 
> > > This lets us compile and link this list of object files into two or more
> > > different binaries.
> > > 
> > > Ordinarily this sort of thing would be solved by making a static library
> > that's
> > > linked into the binaries that need it, but that interacts poorly with
global
> > > constructors, which we use to register tests, GMs, and benchmarks.
> > doesn't gtest has this too (global constructors)?
> > 
> > >  It looks to
> > > the linker like no code from that library is used, so it drops it all.
> > > 
> > > We have an analogous setup for GMs, which are linked into GM, DM, and
> > SampleApp.
> > 
> > for me this is just creating a mess (imo), you might have a different view
> > though, since you take that route.
> > 
> > For me explicit deps (listed on 'dependencies' list) are much easier to
reason
> > about than playing with gyp includes and building different source files
list
> in
> > other libraries.
> 
> Yes, gtest works the same way.
> 
Then why we use it as a static_library and include it in the dependencies list
as testing/gtest/gtest.gyp:gtest (rather than including its source files list)?

https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest.gyp&...
https://code.google.com/p/chromium/codesearch#chromium/src/cc/cc_tests.gyp&l=231

> We agree that this is a mess.  For now, this works, and no other approach we
> know does yet.
> 
> If you'd like to follow along with discussion of this general problem, you'll
> find it at http://code.google.com/p/skia/issues/detail?id=2142.
Sure, I copied myself there. Thanks for the link!

[commit-bot: I haz the power, 2014-02-25 19:08:12]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/mtklein@chromium.org/178273002/220001

[commit-bot: I haz the power, 2014-02-25 19:32:23]

Change committed as 13586

[reed1, 2014-02-25 20:01:48]

A revert of this CL has been created in
https://codereview.chromium.org/179403010/ by reed@google.com.

The reason for reverting is: broke tests.

[mtklein, 2014-02-26 16:11:09]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-02-26 16:11:17]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/mtklein@chromium.org/178273002/260001

[commit-bot: I haz the power, 2014-02-26 16:31:30]

Change committed as 13592